memory leak in msc_rules_add_file / msc_rules_cleanup #2710

liudongmiao · 2022-03-22T10:54:05Z

There are reports on memory leak on nginx -s reload, #2381 #2502 #2552 #2636 and many others .
~~Of course, #2580 doesn't solve problems.~~ (Update: #2580 should have fixed this problem.)

There is a simple poc:

#include <stdio.h>
#include <unistd.h>
#include "modsecurity/rules_set.h"

int main(int argc, char **argv) {
    int i;
    char *file;
    const char *error;
#ifdef __APPLE__
    printf("top -pid %d\n", getpid());
#else
    printf("top -p %d\n", getpid());
#endif
    file = argc > 1 ? argv[1] : "memory-leak.conf";
    printf("rules file: %s\n", file);
    for (i = 0; i < 100; ++i) {
        RulesSet *rules_set = msc_create_rules_set();
        if (msc_rules_add_file(rules_set, file, &error) < 0) {
            fprintf(stderr, "error: %s\n", error);
            break;
        }
        msc_rules_cleanup(rules_set);
    }
    printf("100 iter completed\n");
    sleep(100);
    return 0;
}

After run 100 times on CRS rules, the memory grow to 1.2G, about 12M for rules.

Include modsecurity-v3.0.6/modsecurity.conf-recommended
Include coreruleset-3.3.2/crs-setup.conf.example
Include coreruleset-3.3.2/rules/*.conf

Of course, for nginx -s reload, it may be solved by moving msc_rules_add_file to working process, then it doesn't affect master process, and sovle the problem.

However, IMO, it should be fixed in ModSecurity, as we have shared_ptr in c++11.

The text was updated successfully, but these errors were encountered:

martinhsv · 2022-03-22T16:32:17Z

@liudongmiao ,

As you yourself have pointed out, there already is an issue open for this problem (indeed, more than one).

What is your purpose in opening yet another issue for the same problem?

From the perspective of issue management, tracking, (as well as for other users to search for), it is almost never advantageous to open duplicate issues.

aaishere · 2022-03-22T17:49:42Z

There are reports on memory leak on nginx -s reload, #2381 #2502 #2552 #2636 and many others . Of course, #2580 doesn't solve problems.

There is a simple poc:

#include <stdio.h>
#include <unistd.h>
#include "modsecurity/modsecurity.h"
#include "modsecurity/rules_set.h"

int main(int argc, char **argv) {
    int i;
    char *file;
    const char *error;
    printf("watch -n 1 ps -o rss= --pid %d\n", getpid());
    file = argc > 1 ? argv[1] : "memory-leak.conf";
    printf("rules file: %s\n", file);
    for (i = 0; i < 100; ++i) {
        RulesSet *rules_set = msc_create_rules_set();
        if (msc_rules_add_file(rules_set, file, &error) < 0) {
            fprintf(stderr, "error: %s\n", error);
            break;
        }
        msc_rules_cleanup(rules_set);
    }
    printf("100 iter completed\n");
    sleep(100);
    return 0;
}

After run 100 times on CRS rules, the memory grow to 1.2G, about 12M for rules.

Include modsecurity-v3.0.6/modsecurity.conf-recommended
Include coreruleset-3.3.2/crs-setup.conf.example
Include coreruleset-3.3.2/rules/*.conf

Of course, for nginx -s reload, it may be solved by moving msc_rules_add_file to working process, then it doesn't affect master process, and sovle the problem.

However, IMO, it should be fixed in ModSecurity, as we have shared_ptr in c++11.

Hi @liudongmiao

Thanks for the suggestion regarding the memory leak problem.
I have the same problem with ModSecurity and Nginx.
How can I move msc_rules_add_file to working process to fix the nginx -s reload problem?

liudongmiao · 2022-03-23T01:18:34Z

@aaishere I have a pr owasp-modsecurity/ModSecurity-nginx#277

liudongmiao · 2022-03-23T01:22:08Z

@martinhsv As previous issue doesn't address the real issue, so I opened a new issue.
And attach the benchmark codes for ModSecurity team and contributors to check.

And finally, make a workaround pr on ModSecurity-nginx.
owasp-modsecurity/ModSecurity-nginx#277
It will lazy load modseucrity rules in worker process.

liudongmiao · 2022-04-25T01:26:41Z

@martinhsv The main memory leak is this:

Class Rules has std::vector<std::shared_ptr<Rule> > m_rules
Class Rule, no destructor
- SubClass RuleWithAction, destructor without virtual
  - SubSubClass RuleWithOperator, destructor with virtual

Then, when Rules is destructed, ~RuleWithOperator wouldn't be called.
Fix should be easy, make ~RuleWithAction virtual, or add a virtual ~Rule.

liudongmiao · 2022-04-25T05:11:33Z

@martinhsv I have make a pr at #2728

labanana34 · 2022-05-04T06:46:17Z

Hello Liudongmiao,

I'm interested to test your patch, but I have limited knowledge of how GIT work.
Any tips or documentation I can read to try your fix ?

EDIT::
Ok I think I figured it out, here what I did:

I download every thing from here:
https://github.com/liudongmiao/ModSecurity-nginx/tree/patch-lazy-load-rules
and here:
https://github.com/SpiderLabs/ModSecurity-nginx

I extract and merged the two forlder and recompile the "ngx_http_modsecurity_module.so"

:~/XXX/nginx-1.14.0$ sudo ./configure --with-cc-opt='-g -O2 -fdebug-prefix-map=/build/nginx-H4cN7P/nginx-1.14.0=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -Wdate-time -D_FORTIFY_SOURCE=2' --with-ld-opt='-Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now -fPIC' --prefix=/usr/share/nginx --conf-path=/etc/nginx/nginx.conf --http-log-path=/var/log/nginx/access.log --error-log-path=/var/log/nginx/error.log --lock-path=/var/lock/nginx.lock --pid-path=/run/nginx.pid --modules-path=/usr/lib/nginx/modules --http-client-body-temp-path=/var/lib/nginx/body --http-fastcgi-temp-path=/var/lib/nginx/fastcgi --http-proxy-temp-path=/var/lib/nginx/proxy --http-scgi-temp-path=/var/lib/nginx/scgi --http-uwsgi-temp-path=/var/lib/nginx/uwsgi --with-debug --with-pcre-jit --with-http_ssl_module --with-http_stub_status_module --with-http_realip_module --with-http_auth_request_module --with-http_v2_module --with-http_dav_module --with-http_slice_module --with-threads --with-http_addition_module --with-http_geoip_module=dynamic --with-http_gunzip_module --with-http_gzip_static_module --with-http_image_filter_module=dynamic --with-http_sub_module --with-http_xslt_module=dynamic --with-stream=dynamic --with-stream_ssl_module --with-mail=dynamic --with-mail_ssl_module --add-dynamic-module=../ModSecurity-nginx-lazy-patch
[sudo] Mot de passe de administrateur :
checking for OS
 + Linux 4.15.0-167-generic x86_64
checking for C compiler ... found
 + using GNU C compiler
 + gcc version: 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
checking for gcc -pipe switch ... found
checking for --with-ld-opt="-Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now -fPIC" ... found
checking for -Wl,-E switch ... found
checking for gcc builtin atomic operations ... found
checking for C99 variadic macros ... found
checking for gcc variadic macros ... found
checking for gcc builtin 64 bit byteswap ... found
checking for unistd.h ... found
checking for inttypes.h ... found
checking for limits.h ... found
checking for sys/filio.h ... not found
checking for sys/param.h ... found
checking for sys/mount.h ... found
checking for sys/statvfs.h ... found
checking for crypt.h ... found
checking for Linux specific features
checking for epoll ... found
checking for EPOLLRDHUP ... found
checking for EPOLLEXCLUSIVE ... found
checking for O_PATH ... found
checking for sendfile() ... found
checking for sendfile64() ... found
checking for sys/prctl.h ... found
checking for prctl(PR_SET_DUMPABLE) ... found
checking for prctl(PR_SET_KEEPCAPS) ... found
checking for capabilities ... found
checking for crypt_r() ... found
checking for sys/vfs.h ... found
checking for nobody group ... not found
checking for nogroup group ... found
checking for poll() ... found
checking for /dev/poll ... not found
checking for kqueue ... not found
checking for crypt() ... not found
checking for crypt() in libcrypt ... found
checking for F_READAHEAD ... not found
checking for posix_fadvise() ... found
checking for O_DIRECT ... found
checking for F_NOCACHE ... not found
checking for directio() ... not found
checking for statfs() ... found
checking for statvfs() ... found
checking for dlopen() ... not found
checking for dlopen() in libdl ... found
checking for sched_yield() ... found
checking for sched_setaffinity() ... found
checking for SO_SETFIB ... not found
checking for SO_REUSEPORT ... found
checking for SO_ACCEPTFILTER ... not found
checking for SO_BINDANY ... not found
checking for IP_TRANSPARENT ... found
checking for IP_BINDANY ... not found
checking for IP_BIND_ADDRESS_NO_PORT ... found
checking for IP_RECVDSTADDR ... not found
checking for IP_SENDSRCADDR ... not found
checking for IP_PKTINFO ... found
checking for IPV6_RECVPKTINFO ... found
checking for TCP_DEFER_ACCEPT ... found
checking for TCP_KEEPIDLE ... found
checking for TCP_FASTOPEN ... found
checking for TCP_INFO ... found
checking for accept4() ... found
checking for eventfd() ... found
checking for int size ... 4 bytes
checking for long size ... 8 bytes
checking for long long size ... 8 bytes
checking for void * size ... 8 bytes
checking for uint32_t ... found
checking for uint64_t ... found
checking for sig_atomic_t ... found
checking for sig_atomic_t size ... 4 bytes
checking for socklen_t ... found
checking for in_addr_t ... found
checking for in_port_t ... found
checking for rlim_t ... found
checking for uintptr_t ... uintptr_t found
checking for system byte ordering ... little endian
checking for size_t size ... 8 bytes
checking for off_t size ... 8 bytes
checking for time_t size ... 8 bytes
checking for AF_INET6 ... found
checking for setproctitle() ... not found
checking for pread() ... found
checking for pwrite() ... found
checking for pwritev() ... found
checking for sys_nerr ... found
checking for localtime_r() ... found
checking for clock_gettime(CLOCK_MONOTONIC) ... found
checking for posix_memalign() ... found
checking for memalign() ... found
checking for mmap(MAP_ANON|MAP_SHARED) ... found
checking for mmap("/dev/zero", MAP_SHARED) ... found
checking for System V shared memory ... found
checking for POSIX semaphores ... not found
checking for POSIX semaphores in libpthread ... found
checking for struct msghdr.msg_control ... found
checking for ioctl(FIONBIO) ... found
checking for struct tm.tm_gmtoff ... found
checking for struct dirent.d_namlen ... not found
checking for struct dirent.d_type ... found
checking for sysconf(_SC_NPROCESSORS_ONLN) ... found
checking for sysconf(_SC_LEVEL1_DCACHE_LINESIZE) ... found
checking for openat(), fstatat() ... found
checking for getaddrinfo() ... found
configuring additional dynamic modules
adding module in ../ModSecurity-nginx-lazy-patch
checking for ModSecurity library ... not found
checking for ModSecurity library in /usr/local/modsecurity ... found
 + ngx_http_modsecurity_module was configured
checking for PCRE library ... found
checking for PCRE JIT support ... found
checking for OpenSSL library ... found
checking for zlib library ... found
checking for libxslt ... found
checking for libexslt ... found
checking for GD library ... found
checking for GD WebP support ... found
checking for GeoIP library ... found
checking for GeoIP IPv6 support ... found
creating objs/Makefile

Configuration summary
  + using threads
  + using system PCRE library
  + using system OpenSSL library
  + using system zlib library

  nginx path prefix: "/usr/share/nginx"
  nginx binary file: "/usr/share/nginx/sbin/nginx"
  nginx modules path: "/usr/lib/nginx/modules"
  nginx configuration prefix: "/etc/nginx"
  nginx configuration file: "/etc/nginx/nginx.conf"
  nginx pid file: "/run/nginx.pid"
  nginx error log file: "/var/log/nginx/error.log"
  nginx http access log file: "/var/log/nginx/access.log"
  nginx http client request body temporary files: "/var/lib/nginx/body"
  nginx http proxy temporary files: "/var/lib/nginx/proxy"
  nginx http fastcgi temporary files: "/var/lib/nginx/fastcgi"
  nginx http uwsgi temporary files: "/var/lib/nginx/uwsgi"
  nginx http scgi temporary files: "/var/lib/nginx/scgi"

$ ls -lah /usr/share/nginx/modules/
total 628K
drwxr-xr-x 2 root root 4,0K avril 14 06:08 .
drwxr-xr-x 3 root root 4,0K juin  23  2020 ..
-rw-r--r-- 1 root root  20K avril 12 11:00 ngx_http_geoip_module.so
-rw-r--r-- 1 root root  23K avril 12 11:00 ngx_http_image_filter_module.so
-rw-r--r-- 1 root root 289K mai    4 09:08 ngx_http_modsecurity_module.so
-rw-r--r-- 1 root root  19K avril 12 11:00 ngx_http_xslt_filter_module.so
-rw-r--r-- 1 root root 102K avril 12 11:00 ngx_mail_module.so
-rw-r--r-- 1 root root 160K avril 12 11:00 ngx_stream_module.so

Then restart my Nginx.

I will leave it on my test server to see if I have the memory leak.

Best regards

labanana34 · 2022-05-05T06:49:50Z

@liudongmiao Hey it seems that I still have my memory leak. May be I do something wrong ?

Best regards

liudongmiao · 2022-05-05T10:08:46Z

@labanana34

If you don't use patch in #2728, there are still memory leak in ModSecurity.

For https://github.com/liudongmiao/ModSecurity-nginx/tree/patch-lazy-load-rules, there are still memory leak.
However, as the rules is loaded in separate worker process, then when it's exited (like nginx -s reload), it's meaningless.
Of course, if you don't use nginx worker process, ...., the path-lazy-load-rules is meaningless.

labanana34 · 2022-05-05T12:18:42Z

Oh ok !

So I did the same thing, take this:
https://github.com/liudongmiao/ModSecurity/tree/patch-2
and this:
https://github.com/SpiderLabs/ModSecurity-nginx

Merged the two directory and recompile my "ngx_http_modsecurity_module.so" with it.

I'll keep you inform if I still see the memory leak.

Best regards

labanana34 · 2022-05-06T06:52:18Z

Hello @liudongmiao,

I reproduce the step to recompile my "ngx_http_modsecurity_module.so" with your last patch https://github.com/liudongmiao/ModSecurity/tree/patch-2
But I still experience a memory leak.

Can you confirm me that I do the right thing concerning the compilation of my "ngx_http_modsecurity_module.so" ?

Best regards

liudongmiao · 2022-05-06T07:01:18Z

@labanana34 Please describe the memory leak types.

modsecurity instance.
As I see, no leak at all. However, I have move it to worker process too.
modsecurity rules.
If modsecurity rules is loaded in nginx's worker process, then leak is ok. (The memory would be reclaimed after worker process exits.) You can use example codes in memory leak in msc_rules_add_file / msc_rules_cleanup #2710 (comment)
modsecurity transactions.
There are some little leaks, and may increase. In my cases, we don't use global memory pool, and use nginx memory pool to replase system's malloc / free, and get a 10+% performance and without any memory leaks.

labanana34 · 2022-05-06T07:07:21Z

@liudongmiao
Thank you for your help !

I think I have the 2nd, worker process leak. When I look at my HTOP, "nginx: worker process" grow on memory until there is nothing left.
I'm gonna take a look at the 2710#issue you point me.
EDIT::
So I have to make a try with : https://github.com/Abce/ModSecurity ?

Best regards.

liudongmiao · 2022-05-06T07:17:41Z

@labanana34 Please email me.

mmckenna-yottaa · 2022-08-23T15:14:35Z

rule.h needs a virtual ~Rule destructor to fix the rule memory leak. A virtual ~RuleWithAction destructor is not enough.

If rule.h does not explicitly define a virtual ~Rule destructor, then the Rule destructor is implicitly non-virtual, so that in rules.h the destruction of vector m_rules does not call the destructors for the Rule subclasses, thereby failing to delete the objects owned by the Rule subclasses.

liudongmiao · 2022-08-26T16:20:33Z

rule.h needs a virtual ~Rule destructor to fix the rule memory leak. A virtual ~RuleWithAction destructor is not enough.

It's shared_ptr. In my test, it doesn't require. I will figure it out. (update: explain at #2728 (comment)).

liudongmiao · 2022-09-16T05:20:14Z

Would be fixed in #2801, partially.

martinhsv · 2022-12-21T19:07:06Z

Closing as duplicate.

liudongmiao changed the title ~~memory leak in msc_create_rules_set / msc_rules_add_file / msc_rules_cleanup~~ memory leak in msc_rules_add_file / msc_rules_cleanup Mar 22, 2022

liudongmiao mentioned this issue Mar 22, 2022

lazy loading rules set owasp-modsecurity/ModSecurity-nginx#277

Closed

martinhsv closed this as completed Mar 22, 2022

martinhsv reopened this Mar 22, 2022

This was referenced Mar 23, 2022

ModSecurity memory leak owasp-modsecurity/ModSecurity-nginx#236

Closed

fix some memory leaks for parsing & cleaning up rules #2580

Closed

liudongmiao mentioned this issue Apr 24, 2022

fix memory patch, should close #2710 #2728

Closed

martinhsv closed this as completed Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

memory leak in msc_rules_add_file / msc_rules_cleanup #2710

memory leak in msc_rules_add_file / msc_rules_cleanup #2710

liudongmiao commented Mar 22, 2022 •

edited

Loading

martinhsv commented Mar 22, 2022 •

edited

Loading

aaishere commented Mar 22, 2022

liudongmiao commented Mar 23, 2022

liudongmiao commented Mar 23, 2022

liudongmiao commented Apr 25, 2022

liudongmiao commented Apr 25, 2022

labanana34 commented May 4, 2022 •

edited

Loading

labanana34 commented May 5, 2022

liudongmiao commented May 5, 2022

labanana34 commented May 5, 2022

labanana34 commented May 6, 2022

liudongmiao commented May 6, 2022

labanana34 commented May 6, 2022 •

edited

Loading

liudongmiao commented May 6, 2022

mmckenna-yottaa commented Aug 23, 2022 •

edited

Loading

liudongmiao commented Aug 26, 2022 •

edited

Loading

liudongmiao commented Sep 16, 2022

martinhsv commented Dec 21, 2022

memory leak in msc_rules_add_file / msc_rules_cleanup #2710

memory leak in msc_rules_add_file / msc_rules_cleanup #2710

Comments

liudongmiao commented Mar 22, 2022 • edited Loading

martinhsv commented Mar 22, 2022 • edited Loading

aaishere commented Mar 22, 2022

liudongmiao commented Mar 23, 2022

liudongmiao commented Mar 23, 2022

liudongmiao commented Apr 25, 2022

liudongmiao commented Apr 25, 2022

labanana34 commented May 4, 2022 • edited Loading

labanana34 commented May 5, 2022

liudongmiao commented May 5, 2022

labanana34 commented May 5, 2022

labanana34 commented May 6, 2022

liudongmiao commented May 6, 2022

labanana34 commented May 6, 2022 • edited Loading

liudongmiao commented May 6, 2022

mmckenna-yottaa commented Aug 23, 2022 • edited Loading

liudongmiao commented Aug 26, 2022 • edited Loading

liudongmiao commented Sep 16, 2022

martinhsv commented Dec 21, 2022

liudongmiao commented Mar 22, 2022 •

edited

Loading

martinhsv commented Mar 22, 2022 •

edited

Loading

labanana34 commented May 4, 2022 •

edited

Loading

labanana34 commented May 6, 2022 •

edited

Loading

mmckenna-yottaa commented Aug 23, 2022 •

edited

Loading

liudongmiao commented Aug 26, 2022 •

edited

Loading