Page MenuHomePhabricator

"proc_open()" may fail to return a resource on Windows if the command isn't executable
Closed, ResolvedPublic

Description

See PHI1685. See PHI1667. Two related cases likely arising out of bypass_shell changes connected to T13098:

  • Passthru commands, like commands from arc browse, may fail via ExecPassthru hitting proc_open().
  • Normal commands, most often file in mimetype detection, may fail via ExecFuture hitting proc_open().

On Mac/Linux, the behavior of proc_open() as we invoke it when a command isn't executable (usually, the binary does not exist) is to exit with an error code.

On Windows, the behavior of proc_open() as we invoke it (with bypass_shell) is for the proc_open() itself to fail somewhat opaquely:

CreateProcess failed, error code - 2

There doesn't appear to be any way to pull the error code out of proc_open() robustly or detect this particular failure, other than pattern-matching the error message. The actual implementation in proc_open.c discards the error code after formatting the string:

proc_open.c
	if (FALSE == newprocok) {
		DWORD dw = GetLastError();

		/* clean up all the descriptors */
		for (i = 0; i < ndesc; i++) {
			CloseHandle(descriptors[i].childend);
			if (descriptors[i].parentend) {
				CloseHandle(descriptors[i].parentend);
			}
		}
		php_error_docref(NULL, E_WARNING, "CreateProcess failed, error code - %u", dw);
		goto exit_fail;
	}

Rather than add more string matching where we don't strictly need it, I'm going to assume all proc_open() failure modes on Windows are hitting this, at least for now. The other failure modes of proc_open() largely seem unlikely, although there are a lot of them ("env conversion failed", "command conversion failed", etc).

Event Timeline

epriestley triaged this task as Normal priority.Apr 1 2020, 10:36 PM
epriestley created this task.

Another possibility is to do a Filesystem::binaryExists() functional test on the subprocess binary, assume the failure is "missing binary" if that test fails, and assume the failure is catastrophic otherwise.

A subproblem here is that start <url> fails on Windows with bypass_shell. The likely fix is to make cmd /c start, not start, the default "browser" on Windows.

This may partly have arisen from changes to Future behavior, not just bypass_shell changes. In particular, ExecFuture already had approximately the desired behavior, but was documented like this:

// ... Throw a "CommandException"
// here for consistency with the Linux behavior in this common failure
// case.

This didn't lead to consistency under exec_manual(), but would have been consistent under execx(). I'm not sure if the Futures changes broke consistency under exec_manual() -- it's possible, but I suspect this construction was just never actually consistent under general Future behavior.

PHI1667 has a couple of adjacent issues that I think aren't fully covered here.

Character encoding issues aren't stable yet (see also T13500) but I think everything else here is.