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');
+
+###############################################################################