Fix use-after-free when proc_open() pipe is closed from its user filter

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,22 +293,29 @@ 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;
 		}
 	}
 
 	/* `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;

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--
+<?php
+if (!function_exists('proc_open')) die('skip proc_open() not available');
+?>
+--FILE--
+<?php
+class Closer extends php_user_filter {
+    public function filter($in, $out, &$consumed, bool $closing): int {
+        global $proc;
+        static $tried = false;
+        if (!$tried) {
+            $tried = true;
+            var_dump(proc_close($proc));
+        }
+        while ($bucket = stream_bucket_make_writeable($in)) {
+            $consumed += $bucket->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)

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--
+<?php
+if (!function_exists('proc_open')) die('skip proc_open() not available');
+?>
+--FILE--
+<?php
+class Closer extends php_user_filter {
+    public function filter($in, $out, &$consumed, bool $closing): int {
+        global $proc;
+        if ($proc !== null) {
+            $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('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)

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--
+<?php
+if (!function_exists('proc_open')) die('skip proc_open() not available');
+?>
+--FILE--
+<?php
+class PcloseOther extends php_user_filter {
+    public function filter($in, $out, &$consumed, bool $closing): int {
+        static $done = false;
+        if (!$done) {
+            $done = true;
+            var_dump(pclose($GLOBALS['other']));
+        }
+        while ($bucket = stream_bucket_make_writeable($in)) {
+            $consumed += $bucket->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)