HTTP: fix use-after-free in r.subrequest() on premature client close

Previously, since njs subrequests are background subrequests that nginx
does not wake on completion, the handlers ngx_http_js_subrequest_done()
and ngx_http_qjs_subrequest_done() woke the parent request themselves,
synchronously, via ngx_http_run_posted_requests(), from within
ngx_http_finalize_request() of the subrequest.  When the downstream client
had already aborted, that re-entered request processing and freed the
request pool shared by all subrequests before finalization unwound; the
freed subrequest was then dereferenced in ngx_http_post_action()
(heap-use-after-free).

This handler is unlike the other event completions.  Timers and
ngx.fetch() run their callbacks from their own event handlers, where
draining posted requests is safe; the subrequest completion handler runs
while the subrequest is still being finalized, where it is not.  The wake
helper is shared, but this caller must be treated differently.

Before 0.8.1 the parent was woken by posting the request only; 0ee01840
(0.8.1) added a synchronous ngx_http_run_posted_requests(), which let an
inner run free the request while an outer one was still unwinding.
d34fcb0 (0.8.5) dropped the synchronous run and posted the connection
write event instead, fixing a nested-run use-after-free when a subrequest
callback threw; but that lost the wake when the handler runs as a
subrequest of another module (lua), whose connection write handler is not
ours.  75d6b61 (0.9.5) reverted to posting the request plus the
synchronous run to fix the lost wake, reintroducing the use-after-free,
now seen on a premature client close.

The fix is to post the parent request without running posted requests in
place, as it was done before 0.8.1.

This closes #1077 issue on Github.

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index bba6a44ca79202238eced906730577b222de3f35..efb0220f67fe623dfd11e788ce434efb8321ab4a 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -5044,7 +5044,16 @@ ngx_http_js_subrequest_done(ngx_http_request_t *r, void *data, ngx_int_t rc)
 
     ngx_js_del_event(ctx, event);
 
-    ngx_http_js_event_finalize(r->parent, NGX_OK);
+    /*
+     * Using direct ngx_http_post_request() and not ngx_http_js_event_finalize()
+     * as the latter calls ngx_http_run_posted_requests() which is not safe to
+     * call from subrequest done handler due to re-entrancy issue when
+     * subrequest done handler is called from ngx_http_finalize_request().
+     */
+
+    if (ngx_http_post_request(r->parent, NULL) != NGX_OK) {
+        return NGX_ERROR;
+    }
 
     return NGX_OK;
 }
@@ -7604,7 +7613,16 @@ ngx_http_qjs_subrequest_done(ngx_http_request_t *r, void *data, ngx_int_t rc)
     JS_FreeValue(cx, reply);
     ngx_js_del_event(ctx, event);
 
-    ngx_http_js_event_finalize(r->parent, NGX_OK);
+    /*
+     * Using direct ngx_http_post_request() and not ngx_http_js_event_finalize()
+     * as the latter calls ngx_http_run_posted_requests() which is not safe to
+     * call from subrequest done handler due to re-entrancy issue when
+     * subrequest done handler is called from ngx_http_finalize_request().
+     */
+
+    if (ngx_http_post_request(r->parent, NULL) != NGX_OK) {
+        return NGX_ERROR;
+    }
 
     return NGX_OK;
 }

diff --git a/nginx/t/js_subrequest_premature_client_close.t b/nginx/t/js_subrequest_premature_client_close.t
new file mode 100644 (file)
index 0000000..537b84f
--- /dev/null
+++ b/nginx/t/js_subrequest_premature_client_close.t
@@ -0,0 +1,138 @@
+#!/usr/bin/perl
+
+# (C) Dmitry Volyntsev
+# (C) F5, Inc.
+
+# Tests for http njs module, r.subrequest() finalization safety.
+
+###############################################################################
+
+use warnings;
+use strict;
+
+use Test::More;
+
+use Socket qw/ SOL_SOCKET SO_LINGER CRLF /;
+use IO::Socket::INET;
+
+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 proxy/)
+       ->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+    %%TEST_GLOBALS_HTTP%%
+
+    js_import test.js;
+
+    server {
+        listen       127.0.0.1:8080;
+        server_name  localhost;
+
+        location /sub {
+            js_content test.sub;
+        }
+
+        location = /backend {
+            internal;
+            proxy_pass http://127.0.0.1:8081;
+        }
+
+        location /alive {
+            return 200 alive;
+        }
+    }
+}
+
+EOF
+
+$t->write_file('test.js', <<'EOF');
+    function sub(r) {
+        r.subrequest('/backend', { method: 'GET' })
+        .then(reply => r.return(reply.status))
+        .catch(e => r.return(502));
+    }
+
+    export default { sub };
+EOF
+
+$t->run_daemon(\&http_daemon, port(8081));
+$t->try_run('no njs available')->plan(3);
+$t->waitforsocket('127.0.0.1:' . port(8081));
+
+###############################################################################
+
+like(http_get('/sub'), qr/502/, 'subrequest upstream premature close');
+
+like($t->read_file('error.log'), qr/upstream prematurely closed connection/,
+       'upstream premature close logged');
+
+reset_requests('/sub');
+
+like(http_get('/alive'), qr/200 OK.*alive/s, 'worker alive');
+
+###############################################################################
+
+sub reset_requests {
+       my ($uri) = @_;
+
+       for (1 .. 50) {
+               my $s = IO::Socket::INET->new(
+                       Proto => 'tcp',
+                       PeerAddr => '127.0.0.1:' . port(8080)
+               )
+                       or next;
+
+               $s->autoflush(1);
+               $s->print('GET ' . $uri . ' HTTP/1.1' . CRLF
+                       . 'Host: localhost' . CRLF
+                       . 'Connection: close' . CRLF . CRLF);
+
+               select undef, undef, undef, 0.002;
+               setsockopt($s, SOL_SOCKET, SO_LINGER, pack('ii', 1, 0));
+               close $s;
+       }
+}
+
+sub http_daemon {
+       my $port = shift;
+
+       my $server = IO::Socket::INET->new(
+               Proto => 'tcp',
+               LocalAddr => '127.0.0.1:' . $port,
+               Listen => 50,
+               Reuse => 1
+       )
+               or die "Can't create listening socket: $!\n";
+
+       local $SIG{PIPE} = 'IGNORE';
+
+       while (my $client = $server->accept()) {
+               $client->autoflush(1);
+
+               while (<$client>) {
+                       last if (/^\x0d?\x0a?$/);
+               }
+
+               select undef, undef, undef, 0.05;
+
+               close $client;
+       }
+}
+
+###############################################################################