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).

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index fd3c1742c6c818011049759db3d1085fb60845fa..5c8c0480af655a8072d7d9a3a04b12977a296c53 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -276,8 +276,8 @@ static void ngx_http_js_event_finalize(ngx_http_request_t *r, ngx_int_t rc);
 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 *ev)
     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 *ev)
     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 *ev)
 
 
 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_connection_t    *c;
+    ngx_http_js_ctx_t   *ctx;
+    ngx_http_request_t  *r;
 
-    ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
-                   "http js periodic write event handler");
+    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_request_t *r, ngx_js_periodic_t *periodic)
     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_request_t *r, ngx_int_t rc)
     }
 
     if (rc == NGX_OK) {
-        ngx_http_post_request(r, NULL);
+        ngx_post_event(r->connection->write, &ngx_posted_events);
     }
-
-    ngx_http_run_posted_requests(r->connection);
 }
 
 

diff --git a/nginx/t/js_subrequests.t b/nginx/t/js_subrequests.t
index 4e81f4618267fd771c531e7b1327ff1395b7348b..f1937c9cbee5f1d87043fd9d9956fd22c0b97326 100644 (file)
--- a/nginx/t/js_subrequests.t
+++ b/nginx/t/js_subrequests.t
@@ -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 @@ $t->write_file('test.js', <<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 @@ $t->write_file('test.js', <<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, 'unsafe subrequest uri');
 
 }
 
+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,