[njs] HTTP: fixed r.subrequest() error handling.

[noreply at nginx.com]

details:   https://hg.nginx.org/njs/rev/3ba1433803ee
branches:  
changeset: 2358:3ba1433803ee
user:      Dmitry Volyntsev <xeioex at nginx.com>
date:      Thu May 30 22:22:48 2024 -0700
description:
HTTP: fixed r.subrequest() error handling.

Previously, when at least 2 subrequests were scheduled they both
succeed, but the callback for the second threw an exception
heap-use-after-free happened: a nested chain of
ngx_http_run_posted_requests() calls and terminating request in the
inner call left outer calls with already freed request pointer.

The issue was introduced in 0.8.1 (4cb8e873e8c6).

diffstat:

 nginx/ngx_http_js_module.c |  31 ++++++++++++++++++++-----------
 nginx/t/js_subrequests.t   |  19 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 12 deletions(-)

diffs (129 lines):

diff -r 95bb22b30a0d -r 3ba1433803ee nginx/ngx_http_js_module.c
--- a/nginx/ngx_http_js_module.c	Mon Jun 10 23:06:26 2024 -0700
+++ b/nginx/ngx_http_js_module.c	Thu May 30 22:22:48 2024 -0700
@@ -276,8 +276,8 @@ static void ngx_http_js_event_finalize(n
 static ngx_js_ctx_t *ngx_http_js_ctx(njs_vm_t *vm, ngx_http_request_t *r);
 
 static void ngx_http_js_periodic_handler(ngx_event_t *ev);
-static void ngx_http_js_periodic_write_event_handler(ngx_http_request_t *r);
 static void ngx_http_js_periodic_shutdown_handler(ngx_event_t *ev);
+static void ngx_http_js_periodic_write_handler(ngx_event_t *ev);
 static void ngx_http_js_periodic_finalize(ngx_http_request_t *r, ngx_int_t rc);
 static void ngx_http_js_periodic_destroy(ngx_http_request_t *r,
     ngx_js_periodic_t *periodic);
@@ -4220,7 +4220,10 @@ ngx_http_js_periodic_handler(ngx_event_t
     c->data = r;
     c->destroyed = 0;
     c->pool = r->pool;
+    c->read->log = &periodic->log;
     c->read->handler = ngx_http_js_periodic_shutdown_handler;
+    c->write->log = &periodic->log;
+    c->write->handler = ngx_http_js_periodic_write_handler;
 
     periodic->connection = c;
     periodic->log_ctx.request = r;
@@ -4234,7 +4237,6 @@ ngx_http_js_periodic_handler(ngx_event_t
     r->valid_unparsed_uri = 1;
 
     r->health_check = 1;
-    r->write_event_handler = ngx_http_js_periodic_write_event_handler;
 
     rc = ngx_http_js_init_vm(r, ngx_http_js_periodic_session_proto_id);
 
@@ -4263,12 +4265,17 @@ ngx_http_js_periodic_handler(ngx_event_t
 
 
 static void
-ngx_http_js_periodic_write_event_handler(ngx_http_request_t *r)
+ngx_http_js_periodic_write_handler(ngx_event_t *ev)
 {
-    ngx_http_js_ctx_t  *ctx;
-
-    ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
-                   "http js periodic write event handler");
+    ngx_connection_t    *c;
+    ngx_http_js_ctx_t   *ctx;
+    ngx_http_request_t  *r;
+
+    c = ev->data;
+    r = c->data;
+
+    ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0,
+                   "http js periodic write handler");
 
     ctx = ngx_http_get_module_ctx(r, ngx_http_js_module);
 
@@ -4340,6 +4347,10 @@ ngx_http_js_periodic_destroy(ngx_http_re
     c->fd = (ngx_socket_t) -1;
     c->pool = NULL;
     c->destroyed = 1;
+
+    if (c->write->posted) {
+        ngx_delete_posted_event(c->write);
+    }
 }
 
 
@@ -4451,10 +4462,8 @@ ngx_http_js_event_finalize(ngx_http_requ
     }
 
     if (rc == NGX_OK) {
-        ngx_http_post_request(r, NULL);
-    }
-
-    ngx_http_run_posted_requests(r->connection);
+        ngx_post_event(r->connection->write, &ngx_posted_events);
+    }
 }
 
 
diff -r 95bb22b30a0d -r 3ba1433803ee nginx/t/js_subrequests.t
--- a/nginx/t/js_subrequests.t	Mon Jun 10 23:06:26 2024 -0700
+++ b/nginx/t/js_subrequests.t	Thu May 30 22:22:48 2024 -0700
@@ -180,6 +180,10 @@ http {
             js_content test.sr_in_sr_callback;
         }
 
+        location /sr_error_in_callback {
+            js_content test.sr_error_in_callback;
+        }
+
         location /sr_uri_except {
             js_content test.sr_uri_except;
         }
@@ -417,6 +421,12 @@ EOF
         .then(body_fwd_cb);
     }
 
+    function sr_error_in_callback(r) {
+        r.subrequest("/sub1", () => {});
+        r.subrequest("/sub1", () => { throw "Oops!"; });
+        r.return(200);
+    }
+
     function sr_in_sr_callback(r) {
         r.subrequest('/return', function (reply) {
                 try {
@@ -508,7 +518,7 @@ EOF
                     sr_js_in_subrequest, sr_js_in_subrequest_pr, js_sub,
                     sr_in_sr_callback, sr_out_of_order, sr_except_not_a_func,
                     sr_uri_except, sr_except_failed_to_convert_options_arg,
-                    sr_unsafe};
+                    sr_unsafe, sr_error_in_callback};
 
 EOF
 
@@ -575,6 +585,13 @@ like(http_get('/sr_unsafe'), qr/500/s, '
 
 }
 
+TODO: {
+local $TODO = 'not yet' unless has_version('0.8.5');
+
+http_get('/sr_error_in_callback');
+
+}
+
 $t->stop();
 
 ok(index($t->read_file('error.log'), 'callback is not a function') > 0,