Memory Leak Issue in Nginx PCRE2
Sergey Kandaurov
pluknet at nginx.com
Mon Oct 16 14:19:48 UTC 2023
> 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).
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.
To illustrate:
compile[gen#0]: ngx_regex_compile_context 0x0
compile: ngx_regex_compile_context = 0x607000003580
init[gen#0]: ngx_regex_compile_context 0x607000003580
reload#1
compile[gen#1]: ngx_regex_compile_context 0x607000003580
init[gen#1]: ngx_regex_compile_context 0x607000003580
cleanup[gen#0]: ngx_regex_compile_context 0x607000003580
reload#2
compile[gen#2]: ngx_regex_compile_context 0x0
compile: ngx_regex_compile_context = 0x60700000a510
init[gen#2]: ngx_regex_compile_context 0x60700000a510
cleanup[gen#1]: ngx_regex_compile_context 0x60700000a510
Other than that, it looks good.
--
Sergey Kandaurov
More information about the nginx-devel
mailing list