proc_open() fails for non-existent commands with error: posix_spawn() failed

[ausi]

Description

Since #7933 there is a change in how proc_open() handles non-existent commands.

The following code:

<?php
var_dump($p = proc_open(["invalid_command"], [], $pipes)); 
var_dump(proc_close($p));

Resulted in this output:

PHP Warning:  proc_open(): posix_spawn() failed: No such file or directory in Command line code on line 1
bool(false)
PHP Fatal error:  Uncaught TypeError: proc_close(): Argument #1 ($process) must be of type resource, false given in Command line code:1

But I expected this output instead (as it was before PHP 8.3):

resource(4) of type (process)
PHP Warning:  proc_open(): Exec failed: No such file or directory in Command line code on line 1
int(127)

The different behavior only occurs when passing the command as an array, passing it as a string still behaves as it always did:

<?php
var_dump($p = proc_open("invalid_command", [], $pipes)); 
var_dump(proc_close($p));

Results in this output for all PHP Versions:

resource(4) of type (process)
sh: invalid_command: command not found
int(127)

As calling non-existent commands is probably pretty common, I think the old behavior of returning exit code 127 should be restored for that case.

PHP Version

PHP 8.3.0RC5

Operating System

macOS 13.6 (22G120)

[crrodriguez]

One of the problems of the old way is the way errors are handled..sure.we could special-case ENOENT and emulate the other path.. I however remain unconviced this is a good idea. executable not existing is an error and the function must return false, not doing so was a bug that was fixed by this change.

When you execute it the other way, as a string it goes through the shell which returns an error because the -c command executable does not exist, ergo also works as expected.

[ausi]

executable not existing is an error and the function must return false, not doing so was a bug that was fixed by this change.

Seems reasonable ☺️
For my understanding: is the new behavior (returning false) the same for all platforms? Or only platforms with USE_POSIX_SPAWN enabled?

Am I correct, that the only way to check for that specific case in PHP is with a custom error handler that matches the warning string against No such file or directory?
Proper exceptions would be nice here, but I guess this is out of scope for the posix_spawn() change, right?

[crrodriguez]

adding exceptions is out the scope of the change.
returning info from the child is/was unreliable.. You will find comments in the source code like.. Let's follow it shall we ?

if (child == 0) {
		/* This is the child process */

		if (close_parentends_of_pipes(descriptors, ndesc) == FAILURE) {
			/* We are already in child process and can't do anything to make
			 * `proc_open` return an error in the parent
			 * All we can do is exit with a non-zero (error) exit code */
			_exit(127);
		}

So, this fails.. you will get nothing, other than a failure exit..

if (cwd) {
			php_ignore_value(chdir(cwd));
		}

So if $cwd is given, and the directory cannot be used.. error..will not be returned to you .. in the new USE_POSIX_SPAWN failure will be reported but later at the point posix_spawn is actually executed. at least on linux,..maybe as MacOS implements this stuff as a syscall it is able to bail out eariler if $cwd cannot be used.

		if (argv) {
			/* execvpe() is non-portable, use environ instead. */
			if (env.envarray) {
				environ = env.envarray;
			}
			execvp(ZSTR_VAL(command_str), argv);
		} else {
			if (env.envarray) {
				execle("/bin/sh", "sh", "-c", ZSTR_VAL(command_str), NULL, env.envarray);
			} else {
				execl("/bin/sh", "sh", "-c", ZSTR_VAL(command_str), NULL);
			}
		}

		/* If execvp/execle/execl are successful, we will never reach here
		 * Display error and exit with non-zero (error) status code */
		php_error_docref(NULL, E_WARNING, "Exec failed: %s", strerror(errno));
		_exit(127);

Now if executing the binary fails, it can only exit with code 127, not the code the command actually exited with if any and maybe give you an error if errno has not been clobbered..see where this is going.. you will not get reliable information.

In the old code there is no parent-child syncronization, the OS may run it in any order usually parent first ..in the posix_spawn case the child always runs first..which is what humans app writers expect.

[ausi]

@crrodriguez thank you for the detailed explanation!

I think I understand now. The new behavior is an improvement. I will try to work with it in the Symfony Process component (where this issue originated from) in symfony/symfony#52409

BTW, I noticed that the new implemenation seems to be twice as fast 🚀
Thank you for your work!

[crrodriguez]

@crrodriguez thank you for the detailed explanation!

BTW, I noticed that the new implemenation seems to be twice as fast 🚀 Thank you for your work!

Sounds about right.

[bukka]

I will re-open this as I need to think about this. In general it should behave the same so we might need to emulate even though the new behavior is better - read BC is more important. But haven't though about it much so might change my mind.

[crrodriguez]

I will re-open this as I need to think about this. In general it should behave the same so we might need to emulate even though the new behavior is better - read BC is more important. But haven't though about it much so might change my mind.

IMHO it will only make things already broken light up. If you dig up the PHP 's own test suite you will find code as the example that "command not existing does not result in valid resource handle"