Memory management

[AirisX]

Hi everyone,

it seems that memory manipulations like below are gives some memory leak:
https://github.com/SpiderLabs/ModSecurity-nginx/blob/a2a5858d249222938c2f5e48087a922c63d7f9d8/src/ngx_http_modsecurity_rewrite.c#L184-L186

I tried to comment out lines 184, 186 (except 185) and the difference between using memory by Nginx was more than 100 MB (less than before) after a little load test.

But in the following two cases, the same actions do not give the same result:
https://github.com/SpiderLabs/ModSecurity-nginx/blob/a2a5858d249222938c2f5e48087a922c63d7f9d8/src/ngx_http_modsecurity_rewrite.c#L86-L90

https://github.com/SpiderLabs/ModSecurity-nginx/blob/a2a5858d249222938c2f5e48087a922c63d7f9d8/src/ngx_http_modsecurity_rewrite.c#L135-L137

[AirisX]

I continued my investigations and did a little experiment. For clarity, I commented out all the handlers except ngx_http_modsecurity_rewrite_handler, and inside this handler I commented out all the steps, except for the last operations - msc_process_request_headers and ngx_http_modsecurity_process_intervention.
Here is the full diff:

diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c
index 6d6ee7a..f1c432d 100644
--- a/src/ngx_http_modsecurity_module.c
+++ b/src/ngx_http_modsecurity_module.c
@@ -453,6 +453,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
      * TODO: check if hook into separated phases is the best thing to do.
      *
      */
+    /*
     h_preaccess = ngx_array_push(&cmcf->phases[NGX_HTTP_PREACCESS_PHASE].handlers);
     if (h_preaccess == NULL)
     {
@@ -460,6 +461,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
         return NGX_ERROR;
     }
     *h_preaccess = ngx_http_modsecurity_pre_access_handler;
+    */

     /**
      * Process the log phase.
@@ -468,6 +470,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
      *       check if last phase will not hold the request.
      *
      */
+    /*
     h_log = ngx_array_push(&cmcf->phases[NGX_HTTP_LOG_PHASE].handlers);
     if (h_log == NULL)
     {
@@ -486,6 +489,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
     if (rc != NGX_OK) {
         return rc;
     }
+    */

     return NGX_OK;
 }
diff --git a/src/ngx_http_modsecurity_rewrite.c b/src/ngx_http_modsecurity_rewrite.c
index a0d9196..af10e71 100644
--- a/src/ngx_http_modsecurity_rewrite.c
+++ b/src/ngx_http_modsecurity_rewrite.c
@@ -83,6 +83,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
             return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
         const char *server_addr = inet_ntoa(((struct sockaddr_in *) connection->sockaddr)->sin_addr);
+        /*
         old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
         ret = msc_process_connection(ctx->modsec_transaction,
             client_addr, client_port,
@@ -91,6 +92,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
         if (ret != 1){
             dd("Was not able to extract connection information.");
         }
+        */
         /**
          *
          * FIXME: Check how we can finalize a request without crash nginx.
@@ -100,11 +102,13 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
          * and try to use it later.
          *
          */
+        /*
         dd("Processing intervention with the connection information filled in");
         ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
         if (ret > 0) {
             return ret;
         }
+        */

         const char *http_version;
         switch (r->http_version) {
@@ -132,6 +136,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
         if (n_uri == (char*)-1 || n_method == (char*)-1) {
             return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
+        /*
         old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
         msc_process_uri(ctx->modsec_transaction, n_uri, n_method, http_version);
         ngx_http_modsecurity_pcre_malloc_done(old_pool);
@@ -141,6 +146,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
         if (ret > 0) {
             return ret;
         }
+        */

         /**
          * Since incoming request headers are already in place, lets send it to ModSecurity

After this I did load testing (simple download request from the upstream, 1000 threads) and measured memory usage by the nginx before and after this. Here is the results.
Before:

56.8125 MB      root    63303 nginx: master process
78.1836 MB      root    63305 nginx: worker process

After:

56.8125 MB      root    63303 nginx: master process
162.504 MB      root    63305 nginx: worker process

As you can see the memory usage increased significantly.

After this I moved msc_transaction_cleanup directly to the end of ngx_http_modsecurity_rewrite_handler. Here is the final diff:

diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c
index 6d6ee7a..e205a1e 100644
--- a/src/ngx_http_modsecurity_module.c
+++ b/src/ngx_http_modsecurity_module.c
@@ -213,7 +213,7 @@ ngx_http_modsecurity_cleanup(void *data)

     ctx = (ngx_http_modsecurity_ctx_t *) data;

-    msc_transaction_cleanup(ctx->modsec_transaction);
+    //msc_transaction_cleanup(ctx->modsec_transaction);

 #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS)
     /*
@@ -453,6 +453,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
      * TODO: check if hook into separated phases is the best thing to do.
      *
      */
+    /*
     h_preaccess = ngx_array_push(&cmcf->phases[NGX_HTTP_PREACCESS_PHASE].handlers);
     if (h_preaccess == NULL)
     {
@@ -460,6 +461,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
         return NGX_ERROR;
     }
     *h_preaccess = ngx_http_modsecurity_pre_access_handler;
+    */

     /**
      * Process the log phase.
@@ -468,6 +470,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
      *       check if last phase will not hold the request.
      *
      */
+    /*
     h_log = ngx_array_push(&cmcf->phases[NGX_HTTP_LOG_PHASE].handlers);
     if (h_log == NULL)
     {
@@ -486,6 +489,7 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)
     if (rc != NGX_OK) {
         return rc;
     }
+    */

     return NGX_OK;
 }
diff --git a/src/ngx_http_modsecurity_rewrite.c b/src/ngx_http_modsecurity_rewrite.c
index a0d9196..6b1bdc3 100644
--- a/src/ngx_http_modsecurity_rewrite.c
+++ b/src/ngx_http_modsecurity_rewrite.c
@@ -83,6 +83,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
             return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
         const char *server_addr = inet_ntoa(((struct sockaddr_in *) connection->sockaddr)->sin_addr);
+        /*
         old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
         ret = msc_process_connection(ctx->modsec_transaction,
             client_addr, client_port,
@@ -91,6 +92,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
         if (ret != 1){
             dd("Was not able to extract connection information.");
         }
+        */
         /**
          *
          * FIXME: Check how we can finalize a request without crash nginx.
@@ -100,11 +102,13 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
          * and try to use it later.
          *
          */
+        /*
         dd("Processing intervention with the connection information filled in");
         ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
         if (ret > 0) {
             return ret;
         }
+        */

         const char *http_version;
         switch (r->http_version) {
@@ -132,6 +136,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
         if (n_uri == (char*)-1 || n_method == (char*)-1) {
             return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
+        /*
         old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
         msc_process_uri(ctx->modsec_transaction, n_uri, n_method, http_version);
         ngx_http_modsecurity_pcre_malloc_done(old_pool);
@@ -141,6 +146,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
         if (ret > 0) {
             return ret;
         }
+        */

         /**
          * Since incoming request headers are already in place, lets send it to ModSecurity
@@ -189,6 +195,8 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
         if (ret > 0) {
             return ret;
         }
+
+        msc_transaction_cleanup(ctx->modsec_transaction);
     }

And repeated the measurements. Here is the results.
Before:

56.832 MB       root    12726 nginx: master process
78.2031 MB      root    12728 nginx: worker process

After:

56.832 MB       root    12726 nginx: master process
87.4375 MB      root    12728 nginx: worker process

The memory usage increased not significantly. I can't explain this behaviour yet.
@defanator maybe you faced with memory leaks during requests?

[defanator]

@AirisX if you have any reasons to suspect possible memory leaks, why don't you use tools like valgrind/LSAN/ASAN for checking? What's the goal of measuring RSS?

nginx uses pools [1] for memory allocations, as well as existing PCRE malloc/free handlers in current version of the connector module. Given the nature of pools, you may observe some spikes in memory usage during load testing.

[1] http://nginx.org/en/docs/dev/development_guide.html#pool

[AirisX]

@defanator, such tools like Valgrind or ASAN don't detect any memory leaks in this case. But the fact is that memory usage by the worker process increases during load test and after some time isn't freed.

[AirisX]

I suppose that in case of invoking msc_transaction_cleanup in the callback ngx_http_modsecurity_cleanup results in memory fragmentation (during load test with many concurrent handlers) and the freed blocks can't be returned to the OS.

In case of invoking msc_transaction_cleanup directly at the end of each phase handler, this problem doesn't appear.

[zimmerle]

Hi @AirisX,

The memory footprint used in current v3/master is way small that what was used back in 2017. I am closing this issue because I am assuming that it is no longer a problem. Please re-open in case you still have problems with the most recent code.