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).

Revisions and Commits

Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision

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.

epriestley added a revision: Restricted Differential Revision.Apr 1 2020, 11:06 PM
epriestley closed this task as Resolved by committing Restricted Diffusion Commit.Apr 1 2020, 11:11 PM
epriestley claimed this task.
epriestley added a commit: Restricted Diffusion Commit.

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

epriestley added a revision: Restricted Differential Revision.Apr 2 2020, 3:20 PM
epriestley added a commit: Restricted Diffusion Commit.Apr 2 2020, 3:24 PM
epriestley added a revision: Restricted Differential Revision.Apr 2 2020, 7:40 PM
epriestley added a revision: Restricted Differential Revision.Apr 2 2020, 8:43 PM
epriestley added a commit: Restricted Diffusion Commit.Apr 2 2020, 8:44 PM
epriestley added a commit: Restricted Diffusion Commit.Apr 2 2020, 8:51 PM

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