diff --git a/src/ngx_http_lua_pipe.c b/src/ngx_http_lua_pipe.c index 8c0884bcc2..27e1c3b233 100644 --- a/src/ngx_http_lua_pipe.c +++ b/src/ngx_http_lua_pipe.c @@ -1200,6 +1200,40 @@ ngx_http_lua_ffi_pipe_proc_destroy(ngx_http_lua_ffi_pipe_proc_t *proc) } ngx_http_lua_pipe_proc_finalize(proc); + + /* + * pipe_proc_finalize -> ngx_http_lua_pipe_close_helper may leave pipe + * connections open with active timers/posted events when there are + * pending I/O operations (handler != dummy_handler). Close them now + * before destroying the pool. + * + * Without this, when pool cleanup LIFO ordering causes pipe_proc_destroy + * to run before request_cleanup_handler (e.g. QUIC connection close path: + * ngx_quic_close_streams -> ngx_http_free_request -> ngx_destroy_pool), + * request_cleanup sees proc->pipe == NULL and returns early, leaving the + * timer live. The timer then fires after the request pool is freed, + * accessing a dangling wait_co_ctx pointer -> SIGSEGV. + * + * ngx_close_connection handles everything: timers (ngx_del_timer), + * posted events (ngx_delete_posted_event), epoll removal, fd close, + * and connection recycling. + */ + + if (pipe->stdout_ctx && pipe->stdout_ctx->c) { + ngx_close_connection(pipe->stdout_ctx->c); + pipe->stdout_ctx->c = NULL; + } + + if (pipe->stderr_ctx && pipe->stderr_ctx->c) { + ngx_close_connection(pipe->stderr_ctx->c); + pipe->stderr_ctx->c = NULL; + } + + if (pipe->stdin_ctx && pipe->stdin_ctx->c) { + ngx_close_connection(pipe->stdin_ctx->c); + pipe->stdin_ctx->c = NULL; + } + ngx_destroy_pool(pipe->pool); proc->pipe = NULL; } @@ -2495,13 +2529,20 @@ ngx_http_lua_pipe_proc_read_stdout_cleanup(void *data) "lua pipe proc read stdout cleanup"); proc = wait_co_ctx->data; + + wait_co_ctx->cleanup = NULL; + + if (proc->pipe == NULL) { + /* pipe_proc_destroy already ran (LIFO pool cleanup) and cancelled + * timers/connections; nothing left to clean up here. */ + return; + } + c = proc->pipe->stdout_ctx->c; if (c) { rev = c->read; ngx_http_lua_pipe_clear_event(rev); } - - wait_co_ctx->cleanup = NULL; } @@ -2517,13 +2558,18 @@ ngx_http_lua_pipe_proc_read_stderr_cleanup(void *data) "lua pipe proc read stderr cleanup"); proc = wait_co_ctx->data; + + wait_co_ctx->cleanup = NULL; + + if (proc->pipe == NULL) { + return; + } + c = proc->pipe->stderr_ctx->c; if (c) { rev = c->read; ngx_http_lua_pipe_clear_event(rev); } - - wait_co_ctx->cleanup = NULL; } @@ -2539,13 +2585,18 @@ ngx_http_lua_pipe_proc_write_cleanup(void *data) "lua pipe proc write cleanup"); proc = wait_co_ctx->data; + + wait_co_ctx->cleanup = NULL; + + if (proc->pipe == NULL) { + return; + } + c = proc->pipe->stdin_ctx->c; if (c) { wev = c->write; ngx_http_lua_pipe_clear_event(wev); } - - wait_co_ctx->cleanup = NULL; } @@ -2561,13 +2612,18 @@ ngx_http_lua_pipe_proc_wait_cleanup(void *data) "lua pipe proc wait cleanup"); proc = wait_co_ctx->data; + + wait_co_ctx->cleanup = NULL; + + if (proc->pipe == NULL) { + return; + } + node = proc->pipe->node; pipe_node = (ngx_http_lua_pipe_node_t *) &node->color; pipe_node->wait_co_ctx = NULL; ngx_http_lua_pipe_clear_event(&wait_co_ctx->sleep); - - wait_co_ctx->cleanup = NULL; } diff --git a/t/191-pipe-proc-quic-close-crash.t b/t/191-pipe-proc-quic-close-crash.t new file mode 100644 index 0000000000..cb2f0da7f4 --- /dev/null +++ b/t/191-pipe-proc-quic-close-crash.t @@ -0,0 +1,101 @@ +# vim:set ft= ts=4 sw=4 et fdm=marker: +# +# Regression test for use-after-free crash in ngx_http_lua_pipe_resume_read_stdout_handler. +# +# Root cause: pool cleanup runs in LIFO order. pipe_proc_destroy (registered +# when the pipe is spawned, i.e. *later*) therefore runs *before* +# request_cleanup_handler (registered at Lua handler init time). +# pipe_proc_destroy calls pipe_proc_finalize → ngx_http_lua_pipe_close_helper, +# which, when a read is in progress, posts the event rather than calling +# ngx_close_connection, leaving the read-timeout timer live. It then sets +# proc->pipe = NULL. When request_cleanup_handler runs next it sees +# proc->pipe == NULL and returns early, so the timer is never cancelled. +# After ngx_destroy_pool(r->pool) frees wait_co_ctx, the timer fires and +# dereferences the dangling pointer → SIGSEGV. +# +# Trigger path (QUIC-specific): +# QUIC connection close +# → ngx_quic_close_streams → sc->read->handler (no-op without lua_check_client_abort) +# → ngx_http_free_request → ngx_destroy_pool(r->pool) +# → LIFO pool cleanups: pipe_proc_destroy first, request_cleanup_handler second +# → timer remains live; pool freed; timer fires → SIGSEGV +# +# Fix (ngx_http_lua_ffi_pipe_proc_destroy): after pipe_proc_finalize, call +# ngx_close_connection on any pipe ctx whose connection is still open. +# ngx_close_connection removes both the read-timeout timer (ngx_del_timer) +# and any already-posted event (ngx_delete_posted_event), preventing the UAF. +# +# Timing: +# pipe stdout read timeout : 1 s (timer armed 1 s after request lands) +# curl --max-time : 0.5 s (QUIC connection closed while timer live) +# --- wait : 2 s (covers remaining ~0.5 s + safety margin) + + +BEGIN { + if (!$ENV{TEST_NGINX_USE_HTTP3}) { + $SkipReason = "TEST_NGINX_USE_HTTP3 is not set"; + } else { + my $nginx = $ENV{TEST_NGINX_BINARY} || 'nginx'; + my $v = eval { `$nginx -V 2>&1` } // ''; + + if ($v !~ /--with-http_v3_module/) { + $SkipReason = "requires nginx built with --with-http_v3_module"; + } + } +} + +use Test::Nginx::Socket::Lua $SkipReason ? (skip_all => $SkipReason) : (); + +master_on(); # master process needed to detect/survive worker crash +workers(1); + +repeat_each(1); + +# 1 subtest per block: the --- no_error_log [alert] check. +# (--- ignore_response suppresses the default status-code subtest.) +plan tests => repeat_each() * blocks(); + +log_level('info'); +no_long_string(); + +add_block_preprocessor(sub { + my $block = shift; + + my $http_config = $block->http_config || ''; + $http_config .= <<'_EOC_'; + lua_package_path "../lua-resty-core/lib/?.lua;../lua-resty-lrucache/lib/?.lua;;"; + + init_by_lua_block { + require "resty.core" + } +_EOC_ + $block->set_value("http_config", $http_config); +}); + +run_tests(); + +__DATA__ + +=== TEST 1: pipe read timer must not fire after pool is freed on QUIC connection close +--- config + location = /t { + content_by_lua_block { + -- Spawn a long-lived child; stdout will never produce output. + -- set_timeouts(write_timeout, stdout_timeout, stderr_timeout, wait_timeout) + local proc = require("ngx.pipe").spawn({"sleep", "100"}) + proc:set_timeouts(nil, 1000) -- 1 s stdout read timeout + + -- This call yields and arms a 1 s timer. + -- The test client closes the QUIC connection before the timer fires, + -- triggering the LIFO pool-cleanup use-after-free (without the fix). + proc:stdout_read_line() + } + } +--- request +GET /t +--- timeout: 0.5 +--- wait: 2 +--- ignore_response +--- curl_error eval: qr/\(28\)/ +--- no_error_log +[alert]