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)