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.

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index 2f3ce936ea07993a0fb39c29974d4b63e7e92388..467a5eeaec9d99772b74135d23f3574870af97aa 100644 (file)
--- 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 c0ea90ee886f1b2cd3680b89cc1bd3916ecd164b..ff9e1ad13cfe519bd6da192fb904d737da591b11 100644 (file)
--- 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', <<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}`);
+    }
+
+    function bytes_sent(r) {
+        return r.variables.bytes_sent;
+    }
+
+    function foo_header(r) {
+        return Buffer.from(r.headersIn.foo).toString('hex');
+    }
 
-           r.return(200, `bs: \${r.variables.bytes_sent}`);
+    function url(r) {
+        return r.uri;
     }
 
-    export default { test };
+    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');
 
 ###############################################################################

diff --git a/nginx/t/js_shared_dict_exit.t b/nginx/t/js_shared_dict_exit.t
new file mode 100644 (file)
index 0000000..cf5c9ad
--- /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');
+
+###############################################################################