QuickJS: keep reused contexts in their origin queue

After an internal redirect, HTTP cleanup selected the destination
location configuration.  If the source and destination used different
module graphs, the source context was inserted into the destination
reuse queue.  A later request could then run a destination handler in
the source runtime, returning an incorrect response or a 500 error.

Record the originating location configuration in each cloned JavaScript
context and use it during teardown.  Source and destination reuse queues
now remain isolated even though internal redirects mutate the request's
current location.

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index 8fc3a154c045d5ac344e1631e5e6c5dea69d4571..03ef1cb92ba6942f73e48386f5c4f238df43759a 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -2183,10 +2183,8 @@ ngx_http_js_init_vm(ngx_http_request_t *r, njs_int_t proto_id)
 static void
 ngx_http_js_cleanup_ctx(void *data)
 {
-    ngx_http_request_t      *r;
-    ngx_http_js_loc_conf_t  *jlcf;
-
-    ngx_http_js_ctx_t        *ctx = data;
+    ngx_http_request_t  *r;
+    ngx_http_js_ctx_t   *ctx = data;
 
     if (ngx_js_ctx_pending(ctx)) {
         ngx_log_error(NGX_LOG_ERR, ctx->log, 0, "pending events");
@@ -2198,14 +2196,12 @@ ngx_http_js_cleanup_ctx(void *data)
     r = ngx_js_ctx_external(ctx);
 
     /*
-     * Restoring the original module context, because it can be reset
-     * by internalRedirect() method. Proper ctx is required for
-     * ngx_http_qjs_request_finalizer() to work correctly.
+     * Restoring the original module context, because it can be reset by
+     * internalRedirect().  Exit hooks and event destructors must resolve the
+     * context being destroyed.
      */
     ngx_http_set_ctx(r, ctx, ngx_http_js_module);
 
-    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', ...)
@@ -2213,7 +2209,7 @@ ngx_http_js_cleanup_ctx(void *data)
      */
     r->pool = ngx_create_pool(128, ctx->log);
 
-    ngx_js_ctx_destroy((ngx_js_ctx_t *) ctx, (ngx_js_loc_conf_t *) jlcf);
+    ngx_js_ctx_destroy((ngx_js_ctx_t *) ctx);
 
     ngx_destroy_pool(r->pool);
 }

diff --git a/nginx/ngx_js.c b/nginx/ngx_js.c
index c077f50368be32a84af1660f1eebcda08d057bc9..f18acb6ab6d1549cf272a1dab00c57f8ff167732 100644 (file)
--- a/nginx/ngx_js.c
+++ b/nginx/ngx_js.c
@@ -769,6 +769,7 @@ ngx_njs_clone(ngx_js_ctx_t *ctx, ngx_js_loc_conf_t *cf, void *external)
     memcpy(engine, cf->engine, sizeof(ngx_engine_t));
     engine->pool = njs_vm_memory_pool(vm);
     engine->u.njs.vm = vm;
+    ctx->conf = cf;
 
     if (njs_vm_start(vm, njs_value_arg(&retval)) == NJS_ERROR) {
         ngx_js_log_exception(vm, ctx->log, "exception");
@@ -1055,6 +1056,7 @@ ngx_qjs_clone(ngx_js_ctx_t *ctx, ngx_js_loc_conf_t *cf, void *external)
 
     memcpy(engine, cf->engine, sizeof(ngx_engine_t));
     engine->pool = mp;
+    ctx->conf = cf;
 
     if (cf->reuse_queue != NULL) {
         engine->u.qjs.ctx = ngx_js_queue_pop(cf->reuse_queue);
@@ -2606,9 +2608,9 @@ ngx_js_ctx_init(ngx_js_ctx_t *ctx, ngx_log_t *log)
 
 
 void
-ngx_js_ctx_destroy(ngx_js_ctx_t *ctx, ngx_js_loc_conf_t *conf)
+ngx_js_ctx_destroy(ngx_js_ctx_t *ctx)
 {
-    ctx->engine->destroy(ctx->engine, ctx, conf);
+    ctx->engine->destroy(ctx->engine, ctx, ctx->conf);
 }
 
 

diff --git a/nginx/ngx_js.h b/nginx/ngx_js.h
index 5eadfa3230fbd0d891fcc91d9d9ef7b708b8ee3b..8f9cb1ff270b3194ae4e4ea1b0162a6803bc8276 100644 (file)
--- a/nginx/ngx_js.h
+++ b/nginx/ngx_js.h
@@ -189,6 +189,7 @@ typedef struct {
 
 #define NGX_JS_COMMON_CTX                                                     \
     ngx_engine_t          *engine;                                            \
+    ngx_js_loc_conf_t     *conf;                                              \
     ngx_log_t             *log;                                               \
     njs_opaque_value_t     args[3];                                           \
     njs_opaque_value_t     retval;                                            \
@@ -357,7 +358,7 @@ void ngx_js_ctx_init(ngx_js_ctx_t *ctx, ngx_log_t *log);
     ((ctx)->engine->external(ctx->engine))
 
 
-void ngx_js_ctx_destroy(ngx_js_ctx_t *ctx, ngx_js_loc_conf_t *conf);
+void ngx_js_ctx_destroy(ngx_js_ctx_t *ctx);
 ngx_int_t ngx_js_call(njs_vm_t *vm, njs_function_t *func,
     njs_opaque_value_t *args, njs_uint_t nargs);
 ngx_int_t ngx_js_log_exception(njs_vm_t *vm, ngx_log_t *log, const char *txt);

diff --git a/nginx/ngx_stream_js_module.c b/nginx/ngx_stream_js_module.c
index 8bed4cab0001437e39f90a7a116fe9fbcd1348e8..fbf84c2e4fd4ce799f53c7ee12af442d63017b93 100644 (file)
--- a/nginx/ngx_stream_js_module.c
+++ b/nginx/ngx_stream_js_module.c
@@ -1275,8 +1275,7 @@ ngx_stream_js_drop_events(ngx_stream_js_ctx_t *ctx)
 static void
 ngx_stream_js_cleanup(void *data)
 {
-    ngx_stream_js_ctx_t       *ctx;
-    ngx_stream_js_srv_conf_t  *jscf;
+    ngx_stream_js_ctx_t  *ctx;
 
     ngx_stream_session_t *s = data;
 
@@ -1289,9 +1288,7 @@ ngx_stream_js_cleanup(void *data)
     ngx_log_debug1(NGX_LOG_DEBUG_STREAM, ctx->log, 0,
                    "stream js vm destroy: %p", ctx->engine);
 
-    jscf = ngx_stream_get_module_srv_conf(s, ngx_stream_js_module);
-
-    ngx_js_ctx_destroy((ngx_js_ctx_t *) ctx, (ngx_js_loc_conf_t *) jscf);
+    ngx_js_ctx_destroy((ngx_js_ctx_t *) ctx);
 }
 
 

diff --git a/nginx/t/js_context_reuse_redirect.t b/nginx/t/js_context_reuse_redirect.t
new file mode 100644 (file)
index 0000000..56f5e3c
--- /dev/null
+++ b/nginx/t/js_context_reuse_redirect.t
@@ -0,0 +1,103 @@
+#!/usr/bin/perl
+
+# (C) Dmitry Volyntsev
+# (C) F5, Inc.
+
+# Tests for QuickJS context reuse after an internal redirect.  A context must
+# be returned to the reuse queue of the location that created it.
+
+###############################################################################
+
+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;
+worker_processes 1;
+
+events {
+}
+
+http {
+    %%TEST_GLOBALS_HTTP%%
+
+    js_engine qjs;
+    js_context_reuse 4;
+    js_context_reuse_max_size 64m;
+
+    server {
+        listen       127.0.0.1:8080;
+        server_name  localhost;
+
+        location = /source {
+            js_import handler from source.js;
+            js_content handler.content;
+        }
+
+        location = /destination {
+            js_import handler from destination.js;
+            js_content handler.content;
+        }
+    }
+}
+
+EOF
+
+$t->write_file('source.js', <<'EOF');
+    let visits = 0;
+
+    function content(r) {
+        visits++;
+
+        if (r.uri == '/source') {
+            r.internalRedirect(`/destination?from=${visits}`);
+            return;
+        }
+
+        r.return(200, `source:${visits}`);
+    }
+
+    export default { content };
+
+EOF
+
+$t->write_file('destination.js', <<'EOF');
+    let visits = 0;
+
+    function content(r) {
+        visits++;
+        r.return(200, `destination:${visits}:from=${r.args.from || '-'}`);
+    }
+
+    export default { content };
+
+EOF
+
+$t->try_run('no njs available')->plan(4);
+
+###############################################################################
+
+like(http_get('/source'), qr/destination:1:from=1/, 'internal redirect');
+like(http_get('/destination'), qr/destination:2:from=-/, 'destination reuse');
+like(http_get('/destination'), qr/destination:3:from=-/,
+       'destination reuse queue isolation');
+like(http_get('/source'), qr/destination:4:from=2/,
+       'source reuse queue ownership');
+
+###############################################################################