From 754771984a7f1d017f80e1c3a602a4c9dc64cc56 Mon Sep 17 00:00:00 2001 From: Dmitry Volyntsev Date: Thu, 26 Mar 2026 17:16:11 -0700 Subject: [PATCH] Modules: fixed double-free in shared dict update with eviction. Previously, when updating an existing key's string value in a shared dictionary with timeout and evict enabled, ngx_js_dict_alloc() could trigger ngx_js_dict_evict() if the zone was full. Since the node being updated was still in the expire tree, eviction could free it. The subsequent ngx_slab_free_locked() call in the update path then freed the already-freed string data, causing the "chunk is already free" alert followed by a segfault. The fix removes the node from the expire tree before allocating memory for the new value, preventing eviction from reaching it. On allocation failure the node is re-inserted with its original expiry time. --- nginx/ngx_js_shared_dict.c | 41 +++++++++- nginx/t/js_shared_dict.t | 46 ++--------- nginx/t/js_shared_dict_evict.t | 134 +++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 43 deletions(-) create mode 100644 nginx/t/js_shared_dict_evict.t diff --git a/nginx/ngx_js_shared_dict.c b/nginx/ngx_js_shared_dict.c index b15c6951..b4a09bb1 100644 --- a/nginx/ngx_js_shared_dict.c +++ b/nginx/ngx_js_shared_dict.c @@ -1539,11 +1539,26 @@ ngx_js_dict_update(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_js_dict_node_t *node, u_char *p; njs_str_t string; + if (dict->timeout) { + /* + * Remove the node from the expire tree before allocating + * memory for the new value. This serves two purposes: + * it prevents ngx_js_dict_evict() from freeing this node + * during allocation, and it allows the node to be reinserted + * with the updated expiry time below. + */ + ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire); + } + if (dict->type == NGX_JS_DICT_TYPE_STRING) { njs_value_string_get(vm, value, &string); p = ngx_js_dict_alloc(dict, string.length); if (p == NULL) { + if (dict->timeout) { + ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire); + } + return NGX_ERROR; } @@ -1558,7 +1573,6 @@ ngx_js_dict_update(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_js_dict_node_t *node, } if (dict->timeout) { - ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire); node->expire.key = now + timeout; ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire); } @@ -4174,15 +4188,35 @@ ngx_qjs_dict_update(JSContext *cx, ngx_js_dict_t *dict, u_char *p; ngx_str_t string; + if (dict->timeout) { + /* + * Remove the node from the expire tree before allocating + * memory for the new value. This serves two purposes: + * it prevents ngx_js_dict_evict() from freeing this node + * during allocation, and it allows the node to be reinserted + * with the updated expiry time below. + */ + ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire); + } + if (dict->type == NGX_JS_DICT_TYPE_STRING) { string.data = (u_char *) JS_ToCStringLen(cx, &string.len, value); if (string.data == NULL) { + if (dict->timeout) { + ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire); + } + return NGX_ERROR; } p = ngx_js_dict_alloc(dict, string.len); if (p == NULL) { JS_FreeCString(cx, (char *) string.data); + + if (dict->timeout) { + ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire); + } + return NGX_ERROR; } @@ -4196,12 +4230,15 @@ ngx_qjs_dict_update(JSContext *cx, ngx_js_dict_t *dict, } else { if (JS_ToFloat64(cx, &node->value.number, value) < 0) { + if (dict->timeout) { + ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire); + } + return NGX_ERROR; } } if (dict->timeout) { - ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire); node->expire.key = now + timeout; ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire); } diff --git a/nginx/t/js_shared_dict.t b/nginx/t/js_shared_dict.t index a28d09f8..b30e2865 100644 --- a/nginx/t/js_shared_dict.t +++ b/nginx/t/js_shared_dict.t @@ -100,10 +100,6 @@ http { js_content test.name; } - location /evict_stress { - js_content test.evict_stress; - } - location /overflow { js_content test.overflow; } @@ -269,31 +265,6 @@ $t->write_file('test.js', <<'EOF'); r.return(200, dict.replace(r.args.key, value)); } - function evict_stress(r) { - var dict = ngx.shared.foo; - dict.clear(); - - var v = 'x'.repeat(1024); - var n = 64; - var failed = 0; - - for (var i = 0; i < n; i++) { - try { - dict.set('stress_' + i, v); - } catch (e) { - failed++; - } - } - - var last = dict.get('stress_' + (n - 1)); - var first = dict.get('stress_0'); - - var last_s = last !== undefined ? 'exists' : 'missing'; - var first_s = first !== undefined ? 'exists' : 'missing'; - - r.return(200, `failed:${failed},last:${last_s},first:${first_s}`); - } - function overflow(r) { var dict = ngx.shared.overflow; @@ -377,15 +348,15 @@ $t->write_file('test.js', <<'EOF'); r.return(200, Object.keys(ngx.shared).sort()); } - export default { add, capacity, chain, clear, del, evict_stress, - free_space, get, has, incr, items, keys, name, - njs: test_njs, pop, replace, set, set_clear, size, - ttl, zones, overflow }; + export default { add, capacity, chain, clear, del, free_space, get, + has, incr, items, keys, name, njs: test_njs, pop, + replace, set, set_clear, size, ttl, zones, + overflow }; EOF $t->try_run('no js_shared_dict_zone'); -$t->plan(63); +$t->plan(59); ############################################################################### @@ -525,13 +496,6 @@ like(http_get('/overflow'), qr/SharedMemoryError/, 'overflow exception'); } -my $evict_resp = http_get('/evict_stress'); -like($evict_resp, qr/failed:0/, 'evict stress: all writes succeeded'); -like($evict_resp, qr/last:exists/, 'evict stress: last write exists'); -like($evict_resp, qr/first:missing/, 'evict stress: first write evicted'); -unlike($t->read_file('error.log'), qr/no memory in js shared zone "foo"/, - 'evict stress: no shared zone foo errors in error log'); - ############################################################################### sub get_ttl { diff --git a/nginx/t/js_shared_dict_evict.t b/nginx/t/js_shared_dict_evict.t new file mode 100644 index 00000000..3501c238 --- /dev/null +++ b/nginx/t/js_shared_dict_evict.t @@ -0,0 +1,134 @@ +#!/usr/bin/perl + +# (C) Dmitry Volyntsev +# (C) F5, Inc. + +# Tests for js_shared_dict_zone eviction. + +############################################################################### + +use warnings; +use strict; + +use Test::More; + +BEGIN { use FindBin; chdir($FindBin::Bin); } + +use lib 'lib'; +use Test::Nginx; + +############################################################################### + +select STDERR; $| = 1; +select STDOUT; $| = 1; + +my $t = Test::Nginx->new()->has(qw/http/) + ->write_file_expand('nginx.conf', <<'EOF'); + +%%TEST_GLOBALS%% + +daemon off; + +events { +} + +http { + %%TEST_GLOBALS_HTTP%% + + js_import test.js; + + js_shared_dict_zone zone=foo:32k timeout=2s evict; + js_shared_dict_zone zone=stress:32k timeout=1000s evict; + + server { + listen 127.0.0.1:8080; + server_name localhost; + + location /stress { + js_content test.stress; + } + + location /no_self_evict { + js_content test.no_self_evict; + } + } +} + +EOF + +$t->write_file('test.js', <<'EOF'); + function stress(r) { + var dict = ngx.shared.foo; + dict.clear(); + + var v = 'x'.repeat(1024); + var n = 64; + var failed = 0; + + for (var i = 0; i < n; i++) { + try { + dict.set('stress_' + i, v); + } catch (e) { + failed++; + } + } + + var last = dict.get('stress_' + (n - 1)); + var first = dict.get('stress_0'); + + var last_s = last !== undefined ? 'exists' : 'missing'; + var first_s = first !== undefined ? 'exists' : 'missing'; + + r.return(200, `failed:${failed},last:${last_s},first:${first_s}`); + } + + function no_self_evict(r) { + var dict = ngx.shared.stress; + var v = 'x'.repeat(128); + + dict.clear(); + dict.set('target', v); + + /* Count how many items can be added while 'target' is present. */ + + var elems = 0; + while (dict.has('target')) { + dict.set('fill_' + elems++, v); + } + + dict.clear(); + dict.set('target', v); + + for (var i = 0; i < elems - 1; i++) { + dict.set('fill_' + i, v); + } + + /* Check that 'target' is not evicted by the updates of itself. */ + + dict.set('target', 'y'.repeat(128)); + + r.return(200, 'FILLED:' + elems); + } + + export default { stress, no_self_evict }; +EOF + +$t->try_run('no js_shared_dict_zone'); + +$t->plan(6); + +############################################################################### + +my $evict_resp = http_get('/stress'); +like($evict_resp, qr/failed:0/, 'evict stress: all writes succeeded'); +like($evict_resp, qr/last:exists/, 'evict stress: last write exists'); +like($evict_resp, qr/first:missing/, 'evict stress: first write evicted'); +unlike($t->read_file('error.log'), qr/no memory in js shared zone "foo"/, + 'evict stress: no shared zone foo errors in error log'); + +my $update_resp = http_get('/no_self_evict'); +like($update_resp, qr/FILLED:/, 'evict update: zone filled'); +unlike($t->read_file('error.log'), qr/is already free/, + 'evict update: no double-free in error log'); + +############################################################################### -- 2.47.3