Fixed memory leak

[AirisX]

I compiled the nginx-connector with the flag "-fsanitize=leak" and found that the "ModSecurity" object from the "ngx_http_modsecurity_conf_t" structure is not freed.

I tried to fix it according to the examples.

[defanator]

Hi @AirisX, have you checked this change with ASAN as well?

I'm seeing the following with your patch applied:

2017/12/22 10:22:55 [notice] 16066#16066: using the "epoll" event method
2017/12/22 10:22:55 [notice] 16066#16066: nginx/1.13.7
2017/12/22 10:22:55 [notice] 16066#16066: built by gcc 6.3.0 20170406 (Ubuntu 6.3.0-12ubuntu2) 
2017/12/22 10:22:55 [notice] 16066#16066: OS: Linux 4.10.0-30-generic
2017/12/22 10:22:55 [notice] 16066#16066: getrlimit(RLIMIT_NOFILE): 131072:131072
2017/12/22 10:22:55 [notice] 16067#16067: start worker processes
2017/12/22 10:22:55 [notice] 16067#16067: start worker process 16068
2017/12/22 10:23:01 [notice] 16067#16067: signal 15 (SIGTERM) received from 26525, exiting
2017/12/22 10:23:01 [notice] 16068#16068: exiting
2017/12/22 10:23:01 [notice] 16067#16067: signal 14 (SIGALRM) received
ASAN:DEADLYSIGNAL
=================================================================
==16068==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fbfcf57139f bp 0x7fbfcf87cfe8 sp 0x7fffaa467870 T0)
    #0 0x7fbfcf57139e in modsecurity::ModSecurity::~ModSecurity() /home/test/ModSecurity/src/modsecurity.cc:92
    #1 0x7fbfcf57170d in msc_cleanup /home/test/ModSecurity/src/modsecurity.cc:465
    #2 0x56297fc9ffc5 in ngx_http_modsecurity_config_cleanup ../ModSecurity-nginx/src/ngx_http_modsecurity_module.c:654
    #3 0x56297fa87149 in ngx_destroy_pool src/core/ngx_palloc.c:57
    #4 0x56297faec20c in ngx_worker_process_exit src/os/unix/ngx_process_cycle.c:997
    #5 0x56297faec540 in ngx_worker_process_cycle src/os/unix/ngx_process_cycle.c:743
    #6 0x56297fae8b91 in ngx_spawn_process src/os/unix/ngx_process.c:198
    #7 0x56297faec84d in ngx_start_worker_processes src/os/unix/ngx_process_cycle.c:358
    #8 0x56297faee46c in ngx_master_process_cycle src/os/unix/ngx_process_cycle.c:130
    #9 0x56297fa7f0fd in main src/core/nginx.c:381
    #10 0x7fbfce5833f0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x203f0)
    #11 0x56297fa815d9 in _start (/home/test/nginx-static+0x875d9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/test/ModSecurity/src/modsecurity.cc:92 in modsecurity::ModSecurity::~ModSecurity()
==16068==ABORTING
2017/12/22 10:23:02 [notice] 16067#16067: signal 14 (SIGALRM) received
2017/12/22 10:23:02 [notice] 16067#16067: signal 17 (SIGCHLD) received from 16068
2017/12/22 10:23:02 [notice] 16067#16067: worker process 16068 exited with code 1
2017/12/22 10:23:02 [notice] 16067#16067: exit
ASAN:DEADLYSIGNAL
=================================================================
==16067==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fbfcf57139f bp 0x7fbfcf87cfe8 sp 0x7fffaa467a80 T0)
    #0 0x7fbfcf57139e in modsecurity::ModSecurity::~ModSecurity() /home/test/ModSecurity/src/modsecurity.cc:92
    #1 0x7fbfcf57170d in msc_cleanup /home/test/ModSecurity/src/modsecurity.cc:465
    #2 0x56297fc9ffc5 in ngx_http_modsecurity_config_cleanup ../ModSecurity-nginx/src/ngx_http_modsecurity_module.c:654
    #3 0x56297fa87149 in ngx_destroy_pool src/core/ngx_palloc.c:57
    #4 0x56297faead36 in ngx_master_process_exit src/os/unix/ngx_process_cycle.c:720
    #5 0x56297faeef8c in ngx_master_process_cycle src/os/unix/ngx_process_cycle.c:178
    #6 0x56297fa7f0fd in main src/core/nginx.c:381
    #7 0x7fbfce5833f0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x203f0)
    #8 0x56297fa815d9 in _start (/home/test/nginx-static+0x875d9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/test/ModSecurity/src/modsecurity.cc:92 in modsecurity::ModSecurity::~ModSecurity()
==16067==ABORTING

Trying to figure out what is happening there, my current assumption is that ngx_http_modsecurity_config_cleanup() can definitely be called multiple times, and it's not clear whether we should call msc_cleanup() at the very first cleanup call. I'd like to have @zimmerle comment here though, as I'm not sure about any possible issues in libmodsecurity caused by such behavior of the connector.

[defanator]

Further investigation showed that t->modsec in cleanup handler contains invalid pointer since it's not being passed while merging configuration.

Currently applied the following patch and investigating deeper:

@@ -620,6 +622,7 @@ ngx_http_modsecurity_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child)
 
     ngx_conf_merge_value(c->enable, p->enable, 0);
     ngx_conf_merge_value(c->sanity_checks_enabled, p->sanity_checks_enabled, 0);
+    c->modsec = p->modsec;
 
 #if defined(MODSECURITY_DDEBUG) && (MODSECURITY_DDEBUG)
     dd("PARENT RULES");

[AirisX]

Hi @defanator,

just yesterday i found a core-dump originated from the pre-production build with this patch. This dump appeared after the stopping the nginx process. After tracing this dump i got the same you are point on. In my case it was line 94 in modsecurity.cc (I am using slightly modified libmodsecurity). Here is the line itself:

delete m_global_collection;

But my other experimental build with this patch and without all the extra modules has no this problem. I still do not understand what it depends on.

By the way, what libmodsecurity brunch are you using?

As for ngx_http_modsecurity_config_cleanup() you right, it is invoked multiple times. But only the last invoke contains ngx_http_modsecurity_conf_t structure with not null modsec pointer. This structure was filled at the first invoke of the ngx_http_modsecurity_create_conf in ngx_http_modsecurity_create_main_conf. Thus to ngx_http_modsecurity_config_cleanup applied the principle of LIFO.

[AirisX]

I think it is necessary explicitly initialize t->modsec pointer with NULL to prevent invoke msc_cleanup with address 0x000000000000 among multiple invokes of the ngx_http_modsecurity_config_cleanup. Like this:

--- a/src/ngx_http_modsecurity_module.c
+++ b/src/ngx_http_modsecurity_module.c
@@ -545,6 +545,7 @@ static void *ngx_http_modsecurity_create_conf(ngx_conf_t *cf)
     conf->enable = NGX_CONF_UNSET;
     conf->sanity_checks_enabled = NGX_CONF_UNSET;
     conf->rules_set = msc_create_rules_set();
+    conf->modsec = NULL;

     cln = ngx_pool_cleanup_add(cf->pool, 0);
     if (cln == NULL) {

[defanator]

@AirisX I'm using v3/master branch for ModSecurity, and master branch for ModSecurity-nginx. I do all my testing in isolated Vagrant environment created from this project:

https://github.com/defanator/modsecurity-performance

What "extra" modules you were using? How did you build your nginx with modules - statically (--add-module) or dynamically (--add-dynamic-module)?

[defanator]

@AirisX I have tested your patch with addition from #80 (comment) and all seems to work fine.

Full diff was as follows:

diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c
index 6d6ee7a..4fae30c 100644
--- a/src/ngx_http_modsecurity_module.c
+++ b/src/ngx_http_modsecurity_module.c
@@ -545,6 +545,7 @@ static void *ngx_http_modsecurity_create_conf(ngx_conf_t *cf)
     conf->enable = NGX_CONF_UNSET;
     conf->sanity_checks_enabled = NGX_CONF_UNSET;
     conf->rules_set = msc_create_rules_set();
+    conf->modsec = NULL;
 
     cln = ngx_pool_cleanup_add(cf->pool, 0);
     if (cln == NULL) {
@@ -647,13 +648,15 @@ ngx_http_modsecurity_config_cleanup(void *data)
     ngx_pool_t *old_pool;
     ngx_http_modsecurity_conf_t *t = (ngx_http_modsecurity_conf_t *) data;
 
-    dd("deleting a loc conf -- RuleSet is: \"%p\"", t->rules_set);
+    dd("deleting a loc conf -- RuleSet is: \"%p\", modsec is: \"%p\"", t->rules_set, t->modsec);
 
     old_pool = ngx_http_modsecurity_pcre_malloc_init(NULL);
     msc_rules_cleanup(t->rules_set);
+    msc_cleanup(t->modsec);
     ngx_http_modsecurity_pcre_malloc_done(old_pool);
 
     t->rules_set = NULL;
+    t->modsec = NULL;
 }
 

Could you please add that change to the branch linked with this PR?

(dd part is not needed obviously; I have long-awaiting changesets to remove unnecessary dd's and turn the rest into ngx_log_debugX)

[AirisX]

Hi @defanator, I also have tested patch mentioned in comment #80 (comment) and there are no longer core-dumps. As it turned out, the composition of the modules used doesn't matter, and the fact is that the modsec pointer was initialized with the garbage value.

I added this change to the brunch.

[defanator]

@zimmerle I'm fine with merging this one - would you like to take a look as well?

[defanator]

@zimmerle @victorhora guys, could we finally merge this one? I'm happy to do this myself. I suppose we'd like to keep CHANGES here as well? (this one definitely worth to mention there)

[zimmerle]

Sorry for the long delay.

Thank you @AirisX, @defanator and @victorhora. Merged!