Memory leak in modsecurity_request_body_to_stream()

[marcstern]

msr->stream_input_data buffer is never unallocated, leading to a memory leak on each request.
We should use the request memory pool.
This needs passing the pool as parameter in modsecurity_request_body_to_stream() and in read_request_body(). The request memory pool is available when calling read_request_body() (in hook_request_late() ).

[marcstern]

first malloc() in msre_op_rsub_execute() should also be replaced

[marcstern]

I found a better solution: free the memory in modsecurity_tx_cleanup():
/* Streams cleanup. */
if (msr->stream_input_data != NULL) {
free(msr->stream_input_data);
msr->stream_input_data = NULL;
msr->stream_input_length = 0;
msr->stream_input_allocated_length = 0;
}
if (msr->stream_output_data != NULL) {
free(msr->stream_output_data);
msr->stream_output_data = NULL;
msr->stream_output_length = 0;
}

[vloup]

Hi Marc,

I just tested your fix against modsecurity 2.9.3 (Debian buster, I created a new patched deb file with quilt), and it seems you code compiles only if you run configure with --enable-large-stream-input. So far the results do look promising.

A proper fix would be to ifdef the assignation of msr->stream_input_allocated_length like that:

Index: modsecurity-apache-2.9.3/apache2/modsecurity.c
===================================================================
--- modsecurity-apache-2.9.3.orig/apache2/modsecurity.c
+++ modsecurity-apache-2.9.3/apache2/modsecurity.c
@@ -325,6 +325,21 @@ static apr_status_t modsecurity_tx_cleanup(void *data) {
     #endif
 #endif

+    /* Streams cleanup. */
+    if (msr->stream_input_data != NULL) {
+        free(msr->stream_input_data);
+        msr->stream_input_data = NULL;
+        msr->stream_input_length = 0;
+#ifdef MSC_LARGE_STREAM_INPUT
+        msr->stream_input_allocated_length = 0;
+#endif
+    }
+    if (msr->stream_output_data != NULL) {
+        free(msr->stream_output_data);
+        msr->stream_output_data = NULL;
+        msr->stream_output_length = 0;
+    }
+
     return APR_SUCCESS;
 }

[marcstern]

Correct

[vloup]

I pushed this code change on our environment on the 22nd at 12:00.

Here is the memory plot that confirms that things got way better now. Instead of having a daily reboot of our modsec container due to memory leaks, we're seeing it way more stable with no restarts for the last 3 days.

@zimmerle Do you wish to have a merge request for this?

[JamesColeman-LW]

I think having a pull request would be great @vloup as in my testing it solves the issue. I've made a patch for our internal builds of mod_security2, which we will be deploying to our customers shortly.

[vloup]

@JamesColeman-LW a PR has been opened.

[martinhsv]

Closed via #2715 .

Thanks all.