Memory Leak Issue in Nginx PCRE2
Maxim Dounin
mdounin at mdounin.ru
Tue Oct 17 04:01:30 UTC 2023
Hello!
On Mon, Oct 16, 2023 at 06:19:48PM +0400, Sergey Kandaurov wrote:
>
> > On 11 Oct 2023, at 02:04, Maxim Dounin <mdounin at mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Wed, Sep 27, 2023 at 01:13:44AM +0800, 上勾拳 wrote:
> >
> >> Dear Nginx Developers,
> >>
> >> I hope this email finds you well. I am reaching out to the mailing list for
> >> the first time to report and discuss an issue I encountered while working
> >> on supporting PCRE2 in OpenResty. If I have made any errors in my reporting
> >> or discussion, please do not hesitate to provide feedback. Your guidance is
> >> greatly appreciated.
> >>
> >> During my recent work, I used the sanitizer to inspect potential issues,
> >> and I identified a small memory leak in the PCRE2 code section of Nginx.
> >> While this issue does not seem to be critical, it could potentially disrupt
> >> memory checking tools. To help you reproduce the problem, I have included a
> >> minimal configuration below. Please note that this issue occurs when Nginx
> >> is configured to use PCRE2, and the version is 1.22.1 or higher.
> >>
> >> *Minimal Configuration for Reproduction:*
> >> worker_processes 1;
> >> daemon off;
> >> master_process off;
> >> error_log
> >> /home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/servroot/logs/error.log
> >> debug;
> >> pid
> >> /home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/servroot/logs/nginx.pid;
> >>
> >> http {
> >> access_log
> >> /home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/servroot/logs/access.log;
> >> #access_log off;
> >> default_type text/plain;
> >> keepalive_timeout 68000ms;
> >> server {
> >> listen 1984;
> >> #placeholder
> >> server_name 'localhost';
> >>
> >> client_max_body_size 30M;
> >> #client_body_buffer_size 4k;
> >>
> >> # Begin preamble config...
> >>
> >> # End preamble config...
> >>
> >> # Begin test case config...
> >>
> >> location ~ '^/[a-d]$' {
> >> return 200;
> >> }
> >> }
> >> }
> >> events {
> >> accept_mutex off;
> >>
> >> worker_connections 64;
> >> }
> >>
> >> *nginx -V :*
> >> nginx version: nginx/1.25.1 (no pool)
> >> built by gcc 11.4.1 20230605 (Red Hat 11.4.1-2) (GCC)
> >> built with OpenSSL 1.1.1u 30 May 2023
> >> TLS SNI support enabled
> >> configure arguments:
> >> --prefix=/home/zhenzhongw/code/pcre_pr/lua-nginx-module/work/nginx
> >> --with-threads --with-pcre-jit --with-ipv6
> >> --with-cc-opt='-fno-omit-frame-pointer -fsanitize=address
> >> -DNGX_LUA_USE_ASSERT -I/opt/pcre2/include -I/opt/ssl/include'
> >> --with-http_v2_module --with-http_v3_module --with-http_realip_module
> >> --with-http_ssl_module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/ndk-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/set-misc-nginx-module
> >> --with-ld-opt='-fsanitize=address -L/opt/pcre2/lib -L/opt/ssl/lib
> >> -Wl,-rpath,/opt/pcre2/lib:/opt/drizzle/lib:/opt/ssl/lib'
> >> --without-mail_pop3_module --without-mail_imap_module
> >> --with-http_image_filter_module --without-mail_smtp_module --with-stream
> >> --with-stream_ssl_module --without-http_upstream_ip_hash_module
> >> --without-http_memcached_module --without-http_auth_basic_module
> >> --without-http_userid_module --with-http_auth_request_module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/echo-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/memc-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/srcache-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-upstream-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/headers-more-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/drizzle-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/rds-json-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/coolkit-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/redis2-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/stream-lua-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/data/fake-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/data/fake-shm-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/data/fake-delayed-load-module
> >> --with-http_gunzip_module --with-http_dav_module --with-select_module
> >> --with-poll_module --with-debug --with-poll_module --with-cc=gcc
> >>
> >> *The sanitizer tool reported the following error message: *
> >> =================================================================
> >> ==555798==ERROR: LeakSanitizer: detected memory leaks
> >>
> >> Direct leak of 72 byte(s) in 1 object(s) allocated from:
> >> #0 0x7f502f6b4a07 in __interceptor_malloc (/lib64/libasan.so.6+0xb4a07)
> >> #1 0x4a1737 in ngx_alloc src/os/unix/ngx_alloc.c:22
> >> #2 0x525796 in ngx_regex_malloc src/core/ngx_regex.c:509
> >> #3 0x7f502f3e745e in _pcre2_memctl_malloc_8
> >> (/opt/pcre2/lib/libpcre2-8.so.0+0x1145e)
> >> #4 0x5771ad in ngx_http_regex_compile src/http/ngx_http_variables.c:2555
> >> #5 0x536088 in ngx_http_core_regex_location
> >> src/http/ngx_http_core_module.c:3263
> >> #6 0x537f94 in ngx_http_core_location
> >> src/http/ngx_http_core_module.c:3115
> >> #7 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> >> #8 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> >> #9 0x5391ec in ngx_http_core_server src/http/ngx_http_core_module.c:2991
> >> #10 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> >> #11 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> >> #12 0x528e4c in ngx_http_block src/http/ngx_http.c:239
> >> #13 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> >> #14 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> >> #15 0x463f74 in ngx_init_cycle src/core/ngx_cycle.c:284
> >> #12 0x528e4c in ngx_http_block src/http/ngx_http.c:239
> >> #13 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> >> #14 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> >> #15 0x463f74 in ngx_init_cycle src/core/ngx_cycle.c:284
> >> #16 0x4300c7 in main src/core/nginx.c:295
> >> #17 0x7ff31a43feaf in __libc_start_call_main (/lib64/libc.so.6+0x3feaf)
> >>
> >> SUMMARY: AddressSanitizer: 72 byte(s) leaked in 1 allocation(s).
> >>
> >> *I have created a patch to address this memory leak issue, which I am
> >> sharing below:*
> >> diff --git a/src/core/ngx_regex.c b/src/core/ngx_regex.c
> >> index 91381f499..71f583789 100644
> >> --- a/src/core/ngx_regex.c
> >> +++ b/src/core/ngx_regex.c
> >> @@ -600,6 +600,8 @@ ngx_regex_cleanup(void *data)
> >> * the new cycle, these will be re-allocated.
> >> */
> >>
> >> + ngx_regex_malloc_init(NULL);
> >> +
> >> if (ngx_regex_compile_context) {
> >> pcre2_compile_context_free(ngx_regex_compile_context);
> >> ngx_regex_compile_context = NULL;
> >> @@ -611,6 +613,8 @@ ngx_regex_cleanup(void *data)
> >> ngx_regex_match_data_size = 0;
> >> }
> >>
> >> + ngx_regex_malloc_done();
> >> +
> >> #endif
> >> }
> >>
> >> @@ -706,7 +710,13 @@ ngx_regex_module_init(ngx_cycle_t *cycle)
> >> ngx_regex_malloc_done();
> >>
> >> ngx_regex_studies = NULL;
> >> +
> >> #if (NGX_PCRE2)
> >> + if (ngx_regex_compile_context) {
> >> + ngx_regex_malloc_init(NULL);
> >> + pcre2_compile_context_free(ngx_regex_compile_context);
> >> + ngx_regex_malloc_done();
> >> + }
> >> ngx_regex_compile_context = NULL;
> >> #endif
> >>
> >> I kindly request your assistance in reviewing this matter and considering
> >> the patch for inclusion in Nginx. If you have any questions or need further
> >> information, please feel free to reach out to me. Your expertise and
> >> feedback are highly valuable in resolving this issue.
> >
> > Thank you for the report.
> >
> > Indeed, this looks like a small leak which manifests itself during
> > reconfiguration when nginx is compiled with PCRE2.
> >
> > The patch looks correct to me, though I think it would be better
> > to don't do anything with ngx_regex_compile_context in
> > ngx_regex_module_init(). Please take a look if the following
> > patch looks good to you:
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1696950530 -10800
> > # Tue Oct 10 18:08:50 2023 +0300
> > # Node ID 0ceb96f57592b77618fba4200797df977241ec9b
> > # Parent cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc
> > Core: fixed memory leak on configuration reload with PCRE2.
> >
> > In ngx_regex_cleanup() allocator wasn't configured when calling
> > pcre2_compile_context_free() and pcre2_match_data_free(), resulting
> > in no ngx_free() call and leaked memory. Fix is ensure that allocator
> > is configured for global allocations, so that ngx_free() is actually
> > called to free memory.
> >
> > Additionally, ngx_regex_compile_context was cleared in
> > ngx_regex_module_init(). It should be either not cleared, so it will
> > be freed by ngx_regex_cleanup(), or properly freed. Fix is to
> > not clear it, so ngx_regex_cleanup() will be able to free it.
> >
> > Reported by ZhenZhong Wu,
> > https://mailman.nginx.org/pipermail/nginx-devel/2023-September/3Z5FIKUDRN2WBSL3JWTZJ7SXDA6YIWPB.html
> >
> > diff --git a/src/core/ngx_regex.c b/src/core/ngx_regex.c
> > --- a/src/core/ngx_regex.c
> > +++ b/src/core/ngx_regex.c
> > @@ -600,6 +600,8 @@ ngx_regex_cleanup(void *data)
> > * the new cycle, these will be re-allocated.
> > */
> >
> > + ngx_regex_malloc_init(NULL);
> > +
> > if (ngx_regex_compile_context) {
> > pcre2_compile_context_free(ngx_regex_compile_context);
> > ngx_regex_compile_context = NULL;
> > @@ -611,6 +613,8 @@ ngx_regex_cleanup(void *data)
> > ngx_regex_match_data_size = 0;
> > }
> >
> > + ngx_regex_malloc_done();
> > +
> > #endif
> > }
> >
> > @@ -706,9 +710,6 @@ ngx_regex_module_init(ngx_cycle_t *cycle
> > ngx_regex_malloc_done();
> >
> > ngx_regex_studies = NULL;
> > -#if (NGX_PCRE2)
> > - ngx_regex_compile_context = NULL;
> > -#endif
> >
> > return NGX_OK;
> > }
> >
>
> Note that direct leaks are also reported when
> pcre2_compile_context_create() is called in runtime from a worker
> process, such as in ngx_http_ssi_regex_match(). The patch stands
> correct in this regard though. Anyway, leaking a compile context
> in a worker process doesn't seem to be harmful as they do not
> accumulate: it's only possible to allocate up to two contexts
> in a worker process: the 1st borrowed from the master process,
> and the 2nd allocated in runtime (replacing the COW'ed one).
In ngx_http_ssi_regex_match(), compile context is only allocated
(via ngx_regex_compile()) if ngx_regex_compile_context is NULL,
that is, no additional contexts will be allocated if COW'ed one is
available. And with the patch, if there is an allocated compile
context, it will be properly freed on cycle destruction in
ngx_regex_cleanup().
> Currently, nginx configuration doesn't seem to provide a way to
> alter the default values in the compile context (per man pcre2api).
> So the proposed change does look correct. But potentially, if we'd
> want to change this behaviour, the patch has a subtle downside,
> which may prevent to apply such configuration changes:
> - on the 1st configuration reload, ngx_regex_compile() will be called
> with the compile context borrowed from a previous cycle (if any), so
> any compile context changes won't be applied to the new configuration
> - on subsequent reloads, this is not an issue, because a previous
> cycle cleanup will free and reset ngx_regex_compile_context, such
> that a current cycle runs will ngx_regex_compile_context == NULL,
> and it will cause to allocate a new context in the next cycle.
Sure. Compile contexts are expected to be equivalent regardless
of when a particular context allocation happens, and the existing
code relies on this.
If we'll consider changing this, it would be required to somehow
provide access from ngx_regex_compile() to the relevant cycle
(that is, either the cycle being created during configuration
parsing, or ngx_cycle for runtime compilation). And with such
access, the ngx_regex_compile_context global variable and direct
allocation from heap won't be needed, as it would be easier to
simply allocate compile context from the cycle pool.
> Other than that, it looks good.
Thanks for looking, pushed to http://mdounin.ru/hg/nginx/
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list