From 0f01ffec191f617915e36541e411316e17267434 Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Sat, 7 Nov 2026 22:40:55 +0100 Subject: [PATCH] Fix use-after-free when proc_open() pipe is closed from its user filter The process resource destructor closed every pipe stream with zend_list_close(), regardless of whether a user filter callback was running on it. Both proc_close() and releasing the last reference to the process from inside such a callback freed the pipe stream and the filter under the running callback. The destructor now only drops its reference to a pipe stream marked PHP_STREAM_FLAG_NO_FCLOSE, so the stream closes with its last reference instead. It also does not wait for the child in that case, as the child may be blocked on the kept open pipe, which would deadlock when the destructor runs nested in another pclose() or proc_close() that still has FG(pclose_wait) set. proc_close() additionally refuses to close the process while one of its pipes carries that flag, since waiting for the child with the pipe still open could deadlock. It warns and returns -1, its documented error value, like pclose() does for such a stream. --- ext/standard/proc_open.c | 30 ++++++++-- .../tests/filters/proc_close_in_filter.phpt | 35 ++++++++++++ .../tests/filters/proc_release_in_filter.phpt | 33 +++++++++++ .../proc_release_in_filter_nested_pclose.phpt | 57 +++++++++++++++++++ 4 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 ext/standard/tests/filters/proc_close_in_filter.phpt create mode 100644 ext/standard/tests/filters/proc_release_in_filter.phpt create mode 100644 ext/standard/tests/filters/proc_release_in_filter_nested_pclose.phpt diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index ed01405e840e..1a21f7b8a9f6 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -274,6 +274,12 @@ static pid_t waitpid_cached(php_process_handle *proc, int *wait_status, int opti } #endif +static bool proc_open_pipe_must_not_close(const zend_resource *pipe) +{ + const php_stream *stream = (const php_stream*)pipe->ptr; + return stream != NULL && (stream->flags & PHP_STREAM_FLAG_NO_FCLOSE) != 0; +} + /* {{{ proc_open_rsrc_dtor * Free `proc` resource, either because all references to it were dropped or because `pclose` or * `proc_close` were called */ @@ -287,12 +293,18 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) int waitpid_options = 0; pid_t wait_pid; #endif + bool pipe_kept_open = false; /* Close all handles to avoid a deadlock */ for (int i = 0; i < proc->npipes; i++) { if (proc->pipes[i] != NULL) { - GC_DELREF(proc->pipes[i]); - zend_list_close(proc->pipes[i]); + if (proc_open_pipe_must_not_close(proc->pipes[i])) { + zend_list_delete(proc->pipes[i]); + pipe_kept_open = true; + } else { + GC_DELREF(proc->pipes[i]); + zend_list_close(proc->pipes[i]); + } proc->pipes[i] = NULL; } } @@ -300,9 +312,10 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) /* `pclose_wait` tells us: Are we freeing this resource because `pclose` or `proc_close` were * called? If so, we need to wait until the child process exits, because its exit code is * needed as the return value of those functions. - * But if we're freeing the resource because of GC, don't wait. */ + * But if we're freeing the resource because of GC, don't wait. + * Also don't wait if a pipe had to be kept open, because the child might block on it. */ #ifdef PHP_WIN32 - if (FG(pclose_wait)) { + if (FG(pclose_wait) && !pipe_kept_open) { WaitForSingleObject(proc->childHandle, INFINITE); } GetExitCodeProcess(proc->childHandle, &wstatus); @@ -314,7 +327,7 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) CloseHandle(proc->childHandle); #elif HAVE_SYS_WAIT_H - if (!FG(pclose_wait)) { + if (!FG(pclose_wait) || pipe_kept_open) { waitpid_options = WNOHANG; } do { @@ -391,6 +404,13 @@ PHP_FUNCTION(proc_close) RETURN_THROWS(); } + for (int i = 0; i < proc->npipes; i++) { + if (proc->pipes[i] != NULL && proc_open_pipe_must_not_close(proc->pipes[i])) { + php_error_docref(NULL, E_WARNING, "Cannot close the process, as one of its pipes must not be manually closed"); + RETURN_LONG(-1); + } + } + FG(pclose_wait) = 1; /* See comment in `proc_open_rsrc_dtor` */ zend_list_close(Z_RES_P(zproc)); FG(pclose_wait) = 0; diff --git a/ext/standard/tests/filters/proc_close_in_filter.phpt b/ext/standard/tests/filters/proc_close_in_filter.phpt new file mode 100644 index 000000000000..b7888e6bfccb --- /dev/null +++ b/ext/standard/tests/filters/proc_close_in_filter.phpt @@ -0,0 +1,35 @@ +--TEST-- +proc_close() of the process from a user filter's callback on one of its pipes +--SKIPIF-- + +--FILE-- +datalen; + stream_bucket_append($out, $bucket); + } + return PSFS_PASS_ON; + } +} + +stream_filter_register('closer', 'Closer'); +$proc = proc_open([PHP_BINARY, '-n', '-r', 'echo "abc";'], [1 => ['pipe', 'w']], $pipes); +stream_filter_append($pipes[1], 'closer', STREAM_FILTER_READ); +var_dump(fread($pipes[1], 10)); +var_dump(proc_close($proc)); +?> +--EXPECTF-- +Warning: proc_close(): Cannot close the process, as one of its pipes must not be manually closed in %s on line %d +int(-1) +string(3) "abc" +int(0) diff --git a/ext/standard/tests/filters/proc_release_in_filter.phpt b/ext/standard/tests/filters/proc_release_in_filter.phpt new file mode 100644 index 000000000000..1aeca2ad6e2f --- /dev/null +++ b/ext/standard/tests/filters/proc_release_in_filter.phpt @@ -0,0 +1,33 @@ +--TEST-- +Releasing the process resource from a user filter's callback on one of its pipes +--SKIPIF-- + +--FILE-- +datalen; + stream_bucket_append($out, $bucket); + } + return PSFS_PASS_ON; + } +} + +stream_filter_register('closer', 'Closer'); +$proc = proc_open([PHP_BINARY, '-n', '-r', 'echo "abc";'], [1 => ['pipe', 'w']], $pipes); +stream_filter_append($pipes[1], 'closer', STREAM_FILTER_READ); +var_dump(fread($pipes[1], 10)); +var_dump(fclose($pipes[1])); +?> +--EXPECT-- +released +string(3) "abc" +bool(true) diff --git a/ext/standard/tests/filters/proc_release_in_filter_nested_pclose.phpt b/ext/standard/tests/filters/proc_release_in_filter_nested_pclose.phpt new file mode 100644 index 000000000000..744bb00d6909 --- /dev/null +++ b/ext/standard/tests/filters/proc_release_in_filter_nested_pclose.phpt @@ -0,0 +1,57 @@ +--TEST-- +Releasing the process from a filter flushed by pclose() while a filter on its pipe is running +--SKIPIF-- + +--FILE-- +datalen; + stream_bucket_append($out, $bucket); + } + return PSFS_PASS_ON; + } +} + +class ReleaseProc extends php_user_filter { + public function filter($in, $out, &$consumed, bool $closing): int { + if ($closing) { + $GLOBALS['proc'] = null; + echo "released\n"; + } + while ($bucket = stream_bucket_make_writeable($in)) { + $consumed += $bucket->datalen; + stream_bucket_append($out, $bucket); + } + return PSFS_PASS_ON; + } +} + +stream_filter_register('pclose_other', 'PcloseOther'); +stream_filter_register('release_proc', 'ReleaseProc'); + +// Child writes more than fits into the pipe buffer, so it blocks until the parent reads. +$proc = proc_open([PHP_BINARY, '-n', '-r', 'echo str_repeat("a", 1 << 20);'], [1 => ['pipe', 'w']], $pipes); +stream_filter_append($pipes[1], 'pclose_other', STREAM_FILTER_READ); + +$other = popen(escapeshellarg(PHP_BINARY) . ' -n -r ' . escapeshellarg('stream_get_contents(STDIN);'), 'w'); +stream_filter_append($other, 'release_proc', STREAM_FILTER_WRITE); + +var_dump(strlen(fread($pipes[1], 10))); +var_dump(strlen(stream_get_contents($pipes[1]))); +var_dump(fclose($pipes[1])); +?> +--EXPECT-- +released +int(0) +int(10) +int(1048566) +bool(true)