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!