Mercurial > njs
changeset 2625:463485fdafa1
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.
| author | Dmitry Volyntsev <xeioex@nginx.com> |
|---|---|
| date | Mon, 29 Sep 2025 18:30:15 -0700 |
| parents | e08b3ffa9536 |
| children | c2300eb88dcf |
| files | nginx/ngx_http_js_module.c nginx/t/js_exit.t nginx/t/js_shared_dict_exit.t |
| diffstat | 3 files changed, 128 insertions(+), 10 deletions(-) [+] |
line wrap: on
line diff
--- a/nginx/ngx_http_js_module.c Wed Oct 01 20:07:14 2025 -0700 +++ b/nginx/ngx_http_js_module.c Mon Sep 29 18:30:15 2025 -0700 @@ -1628,7 +1628,7 @@ 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 @@ "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 @@ 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); }
--- a/nginx/t/js_exit.t Wed Oct 01 20:07:14 2025 -0700 +++ b/nginx/t/js_exit.t Mon Sep 29 18:30:15 2025 -0700 @@ -36,7 +36,13 @@ 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 @@ $t->write_file('test.js', <<EOF); function test(r) { - njs.on('exit', function() { - ngx.log(ngx.WARN, `exit hook: bs: \${r.variables.bytes_sent}`); - }); + njs.on('exit', function() { + ngx.log(ngx.WARN, `exit hook: bs: \${r.variables.bytes_sent}`); + }); - r.return(200, `bs: \${r.variables.bytes_sent}`); + r.return(200, `bs: \${r.variables.bytes_sent}`); } - export default { test }; + function bytes_sent(r) { + return r.variables.bytes_sent; + } + + function foo_header(r) { + return Buffer.from(r.headersIn.foo).toString('hex'); + } + + function url(r) { + return r.uri; + } + + export default { test, bytes_sent, foo_header, url }; EOF -$t->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'); ###############################################################################
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/nginx/t/js_shared_dict_exit.t Mon Sep 29 18:30:15 2025 -0700 @@ -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'); + +###############################################################################
