openresty / lua-nginx-module

Embed the Power of Lua into NGINX HTTP servers
https://openresty.org/
11.34k stars 2.03k forks source link

bugfix: fix memory corruption in consecutive regex calls. #2290

Closed zhongweiy closed 8 months ago

zhongweiy commented 8 months ago

This memory corruption causes nginx crash and can reproduce under "--with-no-pool-patch" build by running t/048-match-dfa.t TEST 9. The error only happens when there is consecutive regex calls and first call is traditional mode with captures and second call is DFA mode. Check the details in the test.

test log:

ok 1 - t/048-match-dfa.t TEST 9: multiple match calls with captures and DFA. - status code ok 
ok 2 - t/048-match-dfa.t TEST 9: multiple match calls with captures and DFA. - response_body - response is expected (repeated req 0, req 0)
 t/048-match-dfa.t TEST 9: multiple match calls with captures and DFA. - Can't connect to 127.0.0.1:1984: Connection refused
    Retry connecting after 0.675 sec
t/048-match-dfa.t TEST 9: multiple match calls with captures and DFA. - Can't connect to 127.0.0.1:1984: Connection refused
    Retry connecting after 0.825 sec

error.log

[notice] 1683757#0: using the "epoll" event method [notice] 1683757#0: openresty/1.25.3.1 (no pool)
[notice] 1683757#0: built by gcc 13.2.1 20231011 (Red Hat 13.2.1-4) (GCC) [notice] 1683757#0: OS: Linux 6.7.4-100.fc38.x86_64 
[notice] 1683757#0: getrlimit(RLIMIT_NOFILE): 1024:524288 
free(): invalid next size (fast)

I hereby granted the copyright of the changes in this pull request to the authors of this lua-nginx-module project.

zhongweiy commented 8 months ago

I checked the failure cases in CI, these failure seems not caused by my change. Are these test flaky?

  1. 043-shdict TEST 34, does not contain any regex.
  2. 023-write TEST 8, does not contain any regex.
  3. 024-access TEST 8, does not contain any regex.
zhuizhuhaomeng commented 8 months ago
diff --git a/src/ngx_http_lua_regex.c b/src/ngx_http_lua_regex.c
index 1b52fa23..0d975f98 100644
--- a/src/ngx_http_lua_regex.c
+++ b/src/ngx_http_lua_regex.c
@@ -688,11 +688,11 @@ ngx_http_lua_ffi_exec_regex(ngx_http_lua_regex_t *re, int flags,
     ngx_pool_t  *old_pool;

     if (flags & NGX_LUA_RE_MODE_DFA) {
-        ovecsize = 2;
+        ovecsize = 1;
         re->ncaptures = 0;

     } else {
-        ovecsize = (re->ncaptures + 1) * 3;
+        ovecsize = re->ncaptures + 1;
     }

     old_pool = ngx_http_lua_pcre_malloc_init(NULL);
@@ -710,7 +710,7 @@ ngx_http_lua_ffi_exec_regex(ngx_http_lua_regex_t *re, int flags,
         }

         ngx_regex_match_data_size = ovecsize;
-        ngx_regex_match_data = pcre2_match_data_create(ovecsize / 3, NULL);
+        ngx_regex_match_data = pcre2_match_data_create(ovecsize, NULL);

         if (ngx_regex_match_data == NULL) {
             rc = PCRE2_ERROR_NOMEMORY;
@@ -750,14 +750,15 @@ ngx_http_lua_ffi_exec_regex(ngx_http_lua_regex_t *re, int flags,
     n = pcre2_get_ovector_count(ngx_regex_match_data);
     ov = pcre2_get_ovector_pointer(ngx_regex_match_data);

 #if (NGX_DEBUG)
     ngx_log_debug5(NGX_LOG_DEBUG_HTTP, ngx_cycle->log, 0,
                    "pcre2_match: flags 0x%05Xd, options 0x%08Xd, rc %d, "
                    "n %ui, ovecsize %ui", flags, exec_opts, rc, n, ovecsize);
 #endif

-    if (!(flags & NGX_LUA_RE_MODE_DFA) && n > ovecsize / 3) {
-        n = ovecsize / 3;
+    if (n > ovecsize) {
+        n = ovecsize;
     }

     for (i = 0; i < n; i++) {
bungle commented 8 months ago

Do we need same on https://github.com/openresty/stream-lua-nginx-module?

zhuizhuhaomeng commented 8 months ago

Do we need same on https://github.com/openresty/stream-lua-nginx-module?

I have fixed the issue in the stream-lua-nginx-module

zhuizhuhaomeng commented 8 months ago

@zhongweiy Cannot wait for your reply. I have made a small modification and pushed into the master branch.

mergify[bot] commented 8 months ago

This pull request is now in conflict :(

zhongweiy commented 8 months ago

@zhuizhuhaomeng thanks for your review!

Since we are updating the ovecsize in ngx_http_lua_ffi_exec_regex(), I think we should update the counterpart in ngx_http_lua_ffi_compile_regex() as well, i.e.:

     ngx_http_lua_regex_jit_compile(re, flags, pool, lmcf, &re_comp);

     if (flags & NGX_LUA_RE_MODE_DFA) {
-        ovecsize = 2;
+        ovecsize = 1;
         re_comp.captures = 0;

     } else {
-        ovecsize = (re_comp.captures + 1) * 3;
+        ovecsize = re_comp.captures + 1;
     }

     dd("allocating cap with size: %d", (int) ovecsize);

-    cap = ngx_palloc(pool, ovecsize * sizeof(int));
+    cap = ngx_palloc(pool, ovecsize * sizeof(int) * 2);
     if (cap == NULL) {
         msg = "no memory";
         goto error;

And the none NGX_PCRE2 ngx_http_lua_ffi_exec_regex() is also probably needed updated.

zhuizhuhaomeng commented 8 months ago

For the PCRE2, (re_comp.captures + 1) * 3 would waste a little memory. If you would like to change ngx_http_lua_ffi_compile_regex, PR is welcome.

zhongweiy commented 8 months ago

Follow PRs are created, please help take a look: