Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 64 additions & 8 deletions src/ngx_http_lua_pipe.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}


Expand All @@ -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;
}


Expand All @@ -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;
}


Expand All @@ -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;
}


Expand Down
101 changes: 101 additions & 0 deletions t/191-pipe-proc-quic-close-crash.t
Original file line number Diff line number Diff line change
@@ -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]
Loading