Handle closed data pipe

Use error handler to throw ProcessException instead of checking error after calling proc_open().
amphp · Jan 15, 2023 · a65d3bc · a65d3bc
1 parent 9c9247f
commit a65d3bc
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 22 deletions.
diff --git a/src/Internal/Posix/PosixRunner.php b/src/Internal/Posix/PosixRunner.php
@@ -52,8 +52,9 @@ public function start(
  $command
  );
 
- // Errors checked below, so suppressing all errors during call to proc_open() and $this->generateFds().
- \set_error_handler(static fn () => true);
+ \set_error_handler(static function (int $code, string $message): never {
+ throw new ProcessException("Process could not be started: Errno: {$code}; {$message}");
+ });
 
  try {
  $proc = \proc_open(
@@ -69,14 +70,16 @@ public function start(
  }
 
  if (!\is_resource($proc)) {
- $message = "Could not start process";
- if ($error = \error_get_last()) {
- $message .= \sprintf(" Errno: %d; %s", $error["type"], $error["message"]);
- }
- throw new ProcessException($message);
+ throw new ProcessException("Process could not be started: unknown error");
  }
 
  $extraDataPipe = $pipes[3];
+
+ /** @psalm-suppress TypeDoesNotContainType */
+ if (!\is_resource($extraDataPipe)) {
+ throw new ProcessException("Process could not be started: the data pipe closed unexpectedly");
+ }
+
  \stream_set_blocking($extraDataPipe, false);
 
  $suspension = EventLoop::getSuspension();
@@ -106,9 +109,11 @@ static function (CancelledException $e) use ($suspension, $callbackId): void {
  $cancellation->unsubscribe($cancellationId);
  }
 
- $pid = \rtrim(\fgets($extraDataPipe));
+ $pid = \rtrim(\fgets($extraDataPipe) ?: '');
  if (!$pid || !\is_numeric($pid)) {
- throw new ProcessException("Could not determine PID");
+ \proc_terminate($proc);
+ \proc_close($proc);
+ throw new ProcessException("Process could not be started: could not determine PID");
  }
 
  $stdin = new WritableResourceStream($pipes[0]);

diff --git a/src/Internal/Windows/WindowsRunner.php b/src/Internal/Windows/WindowsRunner.php
@@ -56,21 +56,25 @@ public function start(
 
  $options['bypass_shell'] = true;
 
- $proc = @\proc_open(
- $this->makeCommand($workingDirectory ?? ''),
- self::FD_SPEC,
- $pipes,
- $workingDirectory ?: null,
- $environment ?: null,
- $options
- );
+ \set_error_handler(static function (int $code, string $message): never {
+ throw new ProcessException("Process could not be started: Errno: {$code}; {$message}");
+ });
+
+ try {
+ $proc = \proc_open(
+ $this->makeCommand($workingDirectory ?? ''),
+ self::FD_SPEC,
+ $pipes,
+ $workingDirectory ?: null,
+ $environment ?: null,
+ $options
+ );
+ } finally {
+ \restore_error_handler();
+ }
 
  if (!\is_resource($proc)) {
- $message = "Could not start process";
- if ($error = \error_get_last()) {
- $message .= \sprintf(" Errno: %d; %s", $error["type"], $error["message"]);
- }
- throw new ProcessException($message);
+ throw new ProcessException("Process could not be started: unknown error");
  }
 
  $status = \proc_get_status($proc);
@@ -84,6 +88,7 @@ public function start(
 
  if ($written !== SocketConnector::SECURITY_TOKEN_SIZE * 6 + \strlen($command) + 2) {
  \fclose($pipes[2]);
+ \proc_terminate($proc);
  \proc_close($proc);
 
  throw new ProcessException("Could not send security tokens / command to process wrapper");