From 63276697eb8c15e2c21a7a281b1d99a936ed65db Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 1 Apr 2020 15:59:20 -0700
Subject: [PATCH] Fix three Windows subprocess issues

Summary:
Fixes T13504. This fixes three issues:

  # In ExecFuture, "proc_open()" on an invalid binary could fail with an unconditional exception.
  # In ExecPassthru, "proc_open()" on an invalid binary could fail with an unconditional exception.
  # In "arc browse", "start <url>" does not work when the shell is bypassed.

In (1) and (2), the desired behavior is to fail with an exit code which is sometimes upgraded to an exception depending on calling convention.

Issue (1) most commonly manifested as "find" failing when run via "cmd.exe".

Issue (2) most commonly manifested as "arc browse" failing.

Issue (3) was entangled with issue (2).

In cases (1) and (2), assume "proc_open()" failures under Windows are because of bad binaries and treat them like bogus commands on Linux/Mac.

In case (3), use "cmd /c start" instead of "start" as a default browser on Windows.

Test Plan:
  - On Windows, did mime type detection in cmd.exe. Before patch: proc_open() exception in "find". After patch: clean (albeit not terribly useful) detection.
  - On Windows, did "arc browse ...". Before patch: proc_open() exception in "start". After patch: clean browser execution.

Maniphest Tasks: T13504

Differential Revision: https://secure.phabricator.com/D21047
---
 src/future/exec/ExecFuture.php         | 22 ++++++++++++-------
 src/future/exec/PhutilExecPassthru.php | 21 ++++++++++++------
 src/workflow/ArcanistWorkflow.php      | 30 ++++++++++++++++++++------
 3 files changed, 51 insertions(+), 22 deletions(-)

diff --git a/src/future/exec/ExecFuture.php b/src/future/exec/ExecFuture.php
index afb0fd14..a49d5062 100644
--- a/src/future/exec/ExecFuture.php
+++ b/src/future/exec/ExecFuture.php
@@ -605,6 +605,9 @@ final class ExecFuture extends PhutilExecutableFuture {
         $trap->destroy();
       } else {
         $err = error_get_last();
+        if ($err) {
+          $err = $err['message'];
+        }
       }
 
       if ($is_windows) {
@@ -618,18 +621,21 @@ final class ExecFuture extends PhutilExecutableFuture {
         // it fails to resolve the command.
 
         // When you run an invalid command on a Windows system, we bypass the
-        // shell and the "proc_open()" itself fails. Throw a "CommandException"
-        // here for consistency with the Linux behavior in this common failure
-        // case.
+        // shell and the "proc_open()" itself fails. See also T13504. Fail the
+        // future immediately, acting as though it exited with an error code
+        // for consistency with Linux.
 
-        throw new CommandException(
+        $result = array(
+          1,
+          '',
           pht(
             'Call to "proc_open()" to open a subprocess failed: %s',
             $err),
-          $this->getCommand(),
-          1,
-          '',
-          '');
+        );
+
+        $this->setResult($result);
+
+        return true;
       }
 
       if ($is_windows) {
diff --git a/src/future/exec/PhutilExecPassthru.php b/src/future/exec/PhutilExecPassthru.php
index 44eef97e..6dec0f2a 100644
--- a/src/future/exec/PhutilExecPassthru.php
+++ b/src/future/exec/PhutilExecPassthru.php
@@ -68,15 +68,22 @@ final class PhutilExecPassthru extends PhutilExecutableFuture {
     $trap->destroy();
 
     if (!is_resource($proc)) {
-      throw new Exception(
-        pht(
-          'Failed to passthru %s: %s',
-          'proc_open()',
-          $errors));
+      // See T13504. When "proc_open()" is given a command with a binary
+      // that isn't available on Windows with "bypass_shell", the function
+      // fails entirely.
+      if (phutil_is_windows()) {
+        $err = 1;
+      } else {
+        throw new Exception(
+          pht(
+            'Failed to passthru %s: %s',
+            'proc_open()',
+            $errors));
+      }
+    } else {
+      $err = proc_close($proc);
     }
 
-    $err = proc_close($proc);
-
     return $err;
   }
 
diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php
index 7fc3db1a..94f5b66d 100644
--- a/src/workflow/ArcanistWorkflow.php
+++ b/src/workflow/ArcanistWorkflow.php
@@ -2069,13 +2069,19 @@ abstract class ArcanistWorkflow extends Phobject {
 
   protected function openURIsInBrowser(array $uris) {
     $browser = $this->getBrowserCommand();
+
+    // The "browser" may actually be a list of arguments.
+    if (!is_array($browser)) {
+      $browser = array($browser);
+    }
+
     foreach ($uris as $uri) {
-      $err = phutil_passthru('%s %s', $browser, $uri);
+      $err = phutil_passthru('%LR %R', $browser, $uri);
       if ($err) {
         throw new ArcanistUsageException(
           pht(
-            "Failed to open '%s' in browser ('%s'). ".
-            "Check your 'browser' config option.",
+            'Failed to open URI "%s" in browser ("%s"). '.
+            'Check your "browser" config option.',
             $uri,
             $browser));
       }
@@ -2089,19 +2095,29 @@ abstract class ArcanistWorkflow extends Phobject {
     }
 
     if (phutil_is_windows()) {
-      return 'start';
+      // See T13504. We now use "bypass_shell", so "start" alone is no longer
+      // a valid binary to invoke directly.
+      return array(
+        'cmd',
+        '/c',
+        'start',
+      );
     }
 
-    $candidates = array('sensible-browser', 'xdg-open', 'open');
+    $candidates = array(
+      'sensible-browser' => array('sensible-browser'),
+      'xdg-open' => array('xdg-open'),
+      'open' => array('open', '--'),
+    );
 
     // NOTE: The "open" command works well on OS X, but on many Linuxes "open"
     // exists and is not a browser. For now, we're just looking for other
     // commands first, but we might want to be smarter about selecting "open"
     // only on OS X.
 
-    foreach ($candidates as $cmd) {
+    foreach ($candidates as $cmd => $argv) {
       if (Filesystem::binaryExists($cmd)) {
-        return $cmd;
+        return $argv;
       }
     }