Using stream wrappers breaks proc_open

[danog]

Description

The following code (from https://packagist.org/packages/dg/bypass-finals, which is where the issue is happening):

<?php

class Wrapper {
	/** @var resource|null  Stream context, which may be set by stream functions */
	public $context;

	/** @var resource|null  File handle, which may be set by stream functions */
	public $handle;


	public function dir_closedir(): void
	{
		closedir($this->handle);
	}


	public function dir_opendir(string $path, int $options): bool
	{
		$this->handle = $this->context
			? $this->native('opendir', $path, $this->context)
			: $this->native('opendir', $path);
		return (bool) $this->handle;
	}


	public function dir_readdir()
	{
		return readdir($this->handle);
	}


	public function dir_rewinddir(): bool
	{
		rewinddir($this->handle);
		return true;
	}


	public function mkdir(string $path, int $mode, int $options): bool
	{
		$recursive = (bool) ($options & STREAM_MKDIR_RECURSIVE);
		return $this->context
			? $this->native('mkdir', $path, $mode, $recursive, $this->context)
			: $this->native('mkdir', $path, $mode, $recursive);
	}


	public function rename(string $pathFrom, string $pathTo): bool
	{
		return $this->context
			? $this->native('rename', $pathFrom, $pathTo, $this->context)
			: $this->native('rename', $pathFrom, $pathTo);
	}


	public function rmdir(string $path, int $options): bool
	{
		return $this->context
			? $this->native('rmdir', $path, $this->context)
			: $this->native('rmdir', $path);
	}


	public function stream_cast(int $castAs)
	{
		return $this->handle;
	}


	public function stream_close(): void
	{
		fclose($this->handle);
	}


	public function stream_eof(): bool
	{
		return feof($this->handle);
	}


	public function stream_flush(): bool
	{
		return fflush($this->handle);
	}


	public function stream_lock(int $operation): bool
	{
		return $operation
			? flock($this->handle, $operation)
			: true;
	}


	public function stream_metadata(string $path, int $option, $value): bool
	{
		switch ($option) {
			case STREAM_META_TOUCH:
				return $this->native('touch', $path, $value[0] ?? time(), $value[1] ?? time());
			case STREAM_META_OWNER_NAME:
			case STREAM_META_OWNER:
				return $this->native('chown', $path, $value);
			case STREAM_META_GROUP_NAME:
			case STREAM_META_GROUP:
				return $this->native('chgrp', $path, $value);
			case STREAM_META_ACCESS:
				return $this->native('chmod', $path, $value);
			default:
				return false;
		}
	}


	public function stream_open(string $path, string $mode, int $options = 0, ?string &$openedPath = null): bool
	{
		$usePath = (bool) ($options & STREAM_USE_PATH);
		$this->handle = $this->context
			? $this->native('fopen', $path, $mode, $usePath, $this->context)
			: $this->native('fopen', $path, $mode, $usePath);
		return (bool) $this->handle;
	}


	public function stream_read(int $count)
	{
		return fread($this->handle, $count);
	}


	public function stream_seek(int $offset, int $whence = SEEK_SET): bool
	{
		if (stream_get_meta_data($this->handle)['seekable']) {
			return fseek($this->handle, $offset, $whence) === 0;
		}
		return false;
	}


	public function stream_set_option(int $option, int $arg1, ?int $arg2)
	{
		switch ($option) {
			case STREAM_OPTION_BLOCKING:
				return stream_set_blocking($this->handle, (bool) $arg1);
			case STREAM_OPTION_READ_BUFFER:
				return stream_set_read_buffer($this->handle, $arg2);
			case STREAM_OPTION_WRITE_BUFFER:
				return stream_set_write_buffer($this->handle, $arg2);
			case STREAM_OPTION_READ_TIMEOUT:
				return stream_set_timeout($this->handle, $arg1, $arg2);
			default:
				return false;
		}
	}


	public function stream_stat()
	{
		return fstat($this->handle);
	}


	public function stream_tell()
	{
		return ftell($this->handle);
	}


	public function stream_truncate(int $newSize): bool
	{
		return ftruncate($this->handle, $newSize);
	}


	public function stream_write(string $data)
	{
		return fwrite($this->handle, $data);
	}


	public function unlink(string $path): bool
	{
		return $this->native('unlink', $path);
	}


	public function url_stat(string $path, int $flags)
	{
		if ($flags & STREAM_URL_STAT_QUIET) {
			set_error_handler(function () {
				return true;
			});
		}
		try {
			$func = $flags & STREAM_URL_STAT_LINK ? 'lstat' : 'stat';
			return $this->native($func, $path);
		} catch (\RuntimeException $e) {
			// SplFileInfo::isFile throws exception
			return false;
		} finally {
			if ($flags & STREAM_URL_STAT_QUIET) {
				restore_error_handler();
			}
		}
	}


	/**
	 * Temporarily restores the native protocol handler to perform operations.
	 */
	private function native(string $func)
	{
		stream_wrapper_restore('file');
		try {
			return $func(...array_slice(func_get_args(), 1));
		} finally {
			stream_wrapper_unregister('file');
			stream_wrapper_register('file', self::class);
		}
	}
}

stream_wrapper_unregister('file');
stream_wrapper_register('file', Wrapper::class);

$proc = \proc_open(
	'/usr/bin/ls',
	[["file", "/dev/null", "r"], STDOUT, STDERR],
	$pipes,
);

Resulted in this output:

Warning: proc_open(): posix_spawn() failed: Bad file descriptor in /Users/daniil/repos/php-src/a.php on line 226

But I expected this output instead: No error.

Stepping through the code, it seems strange to me that this would fail, as after stream_cast returns the real underlying stream resource, it is then re-passed to _php_stream_cast, behaving just like as if no stream wrapper is in use.

Another issue is the fact that stream_cast is even part of the stream wrapper API in the first place, given that its only real function is to return the inner resource (regardless of the cast type); a get_inner_resource method would've been way less confusing...

PHP Version

PHP 8.4.4

Operating System

Ubuntu 24.04

[nielsdos]

Here's what happens:

1. set_proc_descriptor_to_file() is invoked:

   php-src/ext/standard/proc_open.c

   Lines 944 to 975 in da1e254

   	static zend_result set_proc_descriptor_to_file(descriptorspec_item *desc, zend_string *file_path,
   	zend_string *file_mode)
   	{
   	php_socket_t fd;
   	/* try a wrapper */
   	php_stream *stream = php_stream_open_wrapper(ZSTR_VAL(file_path), ZSTR_VAL(file_mode),
   	REPORT_ERRORS|STREAM_WILL_CAST, NULL);
   	if (stream == NULL) {
   	return FAILURE;
   	}
   	/* force into an fd */
   	if (php_stream_cast(stream, PHP_STREAM_CAST_RELEASE|PHP_STREAM_AS_FD, (void **)&fd,
   	REPORT_ERRORS) == FAILURE) {
   	return FAILURE;
   	}
   	#ifdef PHP_WIN32
   	desc->childend = dup_fd_as_handle((int)fd);
   	_close((int)fd);
   	/* Simulate the append mode by fseeking to the end of the file
   	* This introduces a potential race condition, but it is the best we can do */
   	if (strchr(ZSTR_VAL(file_mode), 'a')) {
   	SetFilePointer(desc->childend, 0, NULL, FILE_END);
   	}
   	#else
   	desc->childend = fd;
   	#endif
   	return SUCCESS;
   	}

   , which will call php_stream_cast() with PHP_STREAM_CAST_RELEASE set. PHP_STREAM_CAST_RELEASE is
2. php_stream_free is called after succesful casting:

   php-src/main/streams/cast.c

   Lines 340 to 342 in da1e254

   	if (flags & PHP_STREAM_CAST_RELEASE) {
   	php_stream_free(stream, PHP_STREAM_FREE_CLOSE_CASTED);
   	}

3. PHP_STREAM_FREE_CLOSE_CASTED is PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_PRESERVE_HANDLE, so the handle should be preserved
4. Indirect call to userspace op handler for close, with preserve_handle set to true, so 0 is passed:

   php-src/main/streams/streams.c

   Line 478 in da1e254

   ret = stream->ops->close(stream, preserve_handle ? 0 : 1);

5. php_userstreamop_close is invoked with close_handle set to false, but this argument is never used:

   php-src/main/streams/userspace.c

   Line 690 in da1e254

   static int php_userstreamop_close(php_stream *stream, int close_handle)

6. After your userspace handler calls fclose, it's no longer a valid file descriptor for the child process and posix_spawn therefore complains.

Fixing this would mean propagating close_handle to userspace probably, and handling it there properly.
Probably not a real BC break, but may be master branch material.

[danog]

Thanks, submitted a temporary userland fix in https://github.com/dg/bypass-finals/pull/60/files.

Apart from passing the close_handle flag, destruction of the object in

php-src/main/streams/userspace.c

Line 705 in da1e254

zval_ptr_dtor(&us->object);

should also be prevented, as it leads to garbage collection of the underlying handle, stored as a property (in fact, my userland fix leaks the resource on purpose to prevent this from happening when proc_open is detected).

A better native fix may be to early-return from php_userstreamop_close to completely prevent it from being invoked if close_handle is set to false...

[nielsdos]

Skipping object destruction makes sense.

A better native fix may be to early-return from php_userstreamop_close to completely prevent it from being invoked if close_handle is set to false...

I suppose it depends on what users expect and what users currently rely on. cfr Hyrum's law unfortunately

[nielsdos]

Argh, closed by accident, sorry. Reopening 🙄