From 4936209ba7819135e683e90f364b55cdfee23704 Mon Sep 17 00:00:00 2001 From: Dmitry Volyntsev Date: Mon, 29 Sep 2025 18:30:15 -0700 Subject: [PATCH] Fixed heap-use-after-free in js_set handler. The issue was introduced in commit 04f6dfb (0.9.2) by moving VM destruction from the pool cleanup handler to the http cleanup handler. Moving VM destruction to the http cleanup handler broke js_set variable usage during the log phase, because these variables are called after the VM has been destroyed. The fix is to move VM destruction back to the pool cleanup handler, but use a temporary pool while njs.on('exit', ...) is executing. This fixes #969 and #971 issues on Github. --- nginx/ngx_http_js_module.c | 13 +++++- nginx/t/js_exit.t | 41 +++++++++++++---- nginx/t/js_shared_dict_exit.t | 84 +++++++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 10 deletions(-) create mode 100644 nginx/t/js_shared_dict_exit.t diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c index 2f3ce936e..467a5eeae 100644 --- a/nginx/ngx_http_js_module.c +++ b/nginx/ngx_http_js_module.c @@ -1628,7 +1628,7 @@ static ngx_int_t ngx_http_js_init_vm(ngx_http_request_t *r, njs_int_t proto_id) { ngx_http_js_ctx_t *ctx; - ngx_http_cleanup_t *cln; + ngx_pool_cleanup_t *cln; ngx_http_js_loc_conf_t *jlcf; jlcf = ngx_http_get_module_loc_conf(r, ngx_http_js_module); @@ -1663,7 +1663,7 @@ ngx_http_js_init_vm(ngx_http_request_t *r, njs_int_t proto_id) "http js vm clone %s: %p from: %p", jlcf->engine->name, ctx->engine, jlcf->engine); - cln = ngx_http_cleanup_add(r, 0); + cln = ngx_pool_cleanup_add(r->pool, 0); if (cln == NULL) { return NGX_ERROR; } @@ -1701,7 +1701,16 @@ ngx_http_js_cleanup_ctx(void *data) jlcf = ngx_http_get_module_loc_conf(r, ngx_http_js_module); + /* + * r->pool set to NULL by ngx_http_free_request(). + * Creating a temporary pool for potential use in njs.on('exit', ...) + * handler. + */ + r->pool = ngx_create_pool(128, ctx->log); + ngx_js_ctx_destroy((ngx_js_ctx_t *) ctx, (ngx_js_loc_conf_t *) jlcf); + + ngx_destroy_pool(r->pool); } diff --git a/nginx/t/js_exit.t b/nginx/t/js_exit.t index c0ea90ee8..ff9e1ad13 100644 --- a/nginx/t/js_exit.t +++ b/nginx/t/js_exit.t @@ -36,7 +36,13 @@ events { http { %%TEST_GLOBALS_HTTP%% + log_format test '[var:$bs header:$foo url:$url]'; + access_log %%TESTDIR%%/access.log test; + js_import test.js; + js_set $foo test.foo_header; + js_set $url test.url; + js_set $bs test.bytes_sent; server { listen 127.0.0.1:8080; @@ -52,25 +58,44 @@ EOF $t->write_file('test.js', <try_run('no njs')->plan(2); +$t->try_run('no njs')->plan(3); ############################################################################### -like(http_get('/test'), qr/bs: 0/, 'response'); +like(http( + 'GET /test HTTP/1.0' . CRLF + . 'Foo: bar' . CRLF + . 'Host: localhost' . CRLF . CRLF +), qr/bs: 0/, 'response'); $t->stop(); -like($t->read_file('error.log'), qr/\[warn\].*exit hook: bs: \d+/, 'exit hook logged'); +like($t->read_file('error.log'), qr/\[warn\].*exit hook: bs: \d+/, + 'exit hook logged'); +like($t->read_file('access.log'), qr/\[var:\d+ header:626172 url:\/test\]/, + 'access log has bytes_sent'); ############################################################################### diff --git a/nginx/t/js_shared_dict_exit.t b/nginx/t/js_shared_dict_exit.t new file mode 100644 index 000000000..cf5c9ad94 --- /dev/null +++ b/nginx/t/js_shared_dict_exit.t @@ -0,0 +1,84 @@ +#!/usr/bin/perl + +# (C) Dmitry Volyntsev +# (C) F5, Inc. + +# Tests for js_shared_dict_zone directive, njs.on('exit', ...). + +############################################################################### + +use warnings; +use strict; + +use Test::More; +use Socket qw/ CRLF /; + +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=bar:64k type=string; + + server { + listen 127.0.0.1:8080; + server_name localhost; + + location /exit { + js_content test.exit; + } + } +} + +EOF + +$t->write_file('test.js', <<'EOF'); + function exit(r) { + var dict = ngx.shared.bar; + var key = r.args.key; + + if (!dict.add(key, 'value')) { + r.return(200, `Key ${key} exists`); + return; + } + + njs.on('exit', function() { + dict.delete(key); + }); + + r.return(200, 'SUCCESS'); + } + + export default { exit }; +EOF + +$t->try_run('no js_shared_dict_zone'); + +$t->plan(2); + +############################################################################### + +like(http_get('/exit?key=foo'), qr/SUCCESS/, 'first'); +like(http_get('/exit?key=foo'), qr/SUCCESS/, 'second'); + +###############################################################################