[Process] Windows: stronger escaping, disabled delayed expansion, dep…

…recated options
symfony · nicolas-grekas · Jan 12, 2017 · Jan 18, 2017 · 53e5652f9a89d1c8cb600260edc820d06584eabb · stof
commit 53e5652f9a89d1c8cb600260edc820d06584eabb
diff --git a/.travis.yml b/.travis.yml
@@ -96,7 +96,7 @@ script:
     - if [[ ! $deps && ! $PHP = hhvm* ]]; then echo "$COMPONENTS" | parallel --gnu '$PHPUNIT --exclude-group tty,benchmark,intl-data {}'"$REPORT"; fi
     - if [[ ! $deps && ! $PHP = hhvm* ]]; then echo -e "\\nRunning tests requiring tty"; $PHPUNIT --group tty; fi
     - if [[ ! $deps && $PHP = hhvm* ]]; then $PHPUNIT --exclude-group benchmark,intl-data; fi
-    - if [[ ! $deps && $PHP = ${MIN_PHP%.*} ]]; then echo -e "1\\n0" | xargs -I{} sh -c 'echo "\\nPHP --enable-sigchild enhanced={}" && ENHANCE_SIGCHLD={} php-$MIN_PHP/sapi/cli/php .phpunit/phpunit-4.8/phpunit --colors=always src/Symfony/Component/Process/'; fi
+    - if [[ ! $deps && $PHP = ${MIN_PHP%.*} ]]; then echo -e "1\\n0" | xargs -I{} sh -c 'echo "\\nPHP --enable-sigchild enhanced={}" && SYMFONY_DEPRECATIONS_HELPER=weak ENHANCE_SIGCHLD={} php-$MIN_PHP/sapi/cli/php .phpunit/phpunit-4.8/phpunit --colors=always src/Symfony/Component/Process/'; fi
     - if [[ $deps = high ]]; then echo "$COMPONENTS" | parallel --gnu -j10% 'cd {}; composer update --no-progress --ansi; $PHPUNIT --exclude-group tty,benchmark,intl-data'$LEGACY"$REPORT"; fi
     - if [[ $deps = low ]]; then echo "$COMPONENTS" | parallel --gnu -j10% 'cd {}; composer update --no-progress --ansi --prefer-lowest --prefer-stable; $PHPUNIT --exclude-group tty,benchmark,intl-data'"$REPORT"; fi
     # Test the PhpUnit bridge using the original phpunit script

diff --git a/UPGRADE-3.3.md b/UPGRADE-3.3.md
@@ -42,6 +42,17 @@ HttpKernel
  * The `Psr6CacheClearer::addPool()` method has been deprecated. Pass an array of pools indexed
    by name to the constructor instead.
 
+Process
+-------
+
+ * Delayed expansion on Windows has been disabled.
+
+ * `!VAR!` expansion allowed by ProcessUtils::escapeArgument() is deprecated.
+   You should resolve these variables before calling `ProcessUtils::escapeArgument()`.
+
+ * Configuring `proc_open()` options, Windows compatibility and SIGCHLD
+   compatibility is deprecated - they will be always enabled in 4.0.
+
 Security
 --------
 

diff --git a/UPGRADE-4.0.md b/UPGRADE-4.0.md
@@ -204,6 +204,15 @@ HttpKernel
  * The `Psr6CacheClearer::addPool()` method has been removed. Pass an array of pools indexed
    by name to the constructor instead.
 
+Process
+-------
+
+ * Delayed expansion on Windows has been disabled, you now must resolve
+   `!VAR!` variables before calling `ProcessUtils::escapeArgument()`.
+
+ * Configuring `proc_open()` options, Windows compatibility and SIGCHLD
+   compatibility is not possible anymore - they are always enabled.
+
 Security
 --------
 

diff --git a/src/Symfony/Component/Console/Tests/Helper/ProcessHelperTest.php b/src/Symfony/Component/Console/Tests/Helper/ProcessHelperTest.php
@@ -16,6 +16,7 @@
 use Symfony\Component\Console\Output\StreamOutput;
 use Symfony\Component\Console\Helper\ProcessHelper;
 use Symfony\Component\Process\Process;
+use Symfony\Component\Process\ProcessUtils;
 
 class ProcessHelperTest extends \PHPUnit_Framework_TestCase
 {
@@ -84,7 +85,9 @@ public function provideCommandsAndOutput()
 
         $errorMessage = 'An error occurred';
         if ('\\' === DIRECTORY_SEPARATOR) {
-            $successOutputProcessDebug = str_replace("'", '"', $successOutputProcessDebug);
+            $args = array('php', '-r', 'echo 42;');
+            $args = array_map(array(ProcessUtils::class, 'escapeArgument'), $args);
+            $successOutputProcessDebug = str_replace("'php' '-r' 'echo 42;'", implode(' ', $args), $successOutputProcessDebug);
         }
 
         return array(

diff --git a/src/Symfony/Component/Process/CHANGELOG.md b/src/Symfony/Component/Process/CHANGELOG.md
@@ -1,6 +1,16 @@
 CHANGELOG
 =========
 
+3.3.0
+-----
+
+ * disabled delayed expansion on Windows
+ * added second argument to ProcessUtils::escapeArgument() for controlling the escaping context on Windows
+ * deprecated `!VAR!` expansion allowed by ProcessUtils::escapeArgument()
+ * deprecated configuring `proc_open()` options
+ * deprecated configuring enhanced Windows compatibility
+ * deprecated configuring enhanced SIGCHLD compatibility
+
 2.5.0
 -----
 

diff --git a/src/Symfony/Component/Process/PhpProcess.php b/src/Symfony/Component/Process/PhpProcess.php
@@ -33,7 +33,7 @@ class PhpProcess extends Process
      * @param int         $timeout The timeout in seconds
      * @param array       $options An array of options for proc_open
      */
-    public function __construct($script, $cwd = null, array $env = null, $timeout = 60, array $options = array())
+    public function __construct($script, $cwd = null, array $env = null, $timeout = 60, array $options = null)
     {
         $executableFinder = new PhpExecutableFinder();
         if (false === $php = $executableFinder->find()) {
@@ -52,6 +52,9 @@ public function __construct($script, $cwd = null, array $env = null, $timeout =
             // command with exec
             $php = 'exec '.$php;
         }
+        if (null !== $options) {
+            @trigger_error(sprintf('The $options parameter of the %s constructor is deprecated since version 3.3 and will be removed in 4.0.', __CLASS__), E_USER_DEPRECATED);
+        }
 
         parent::__construct($php, $cwd, $env, $script, $timeout, $options);
     }

diff --git a/src/Symfony/Component/Process/Process.php b/src/Symfony/Component/Process/Process.php
@@ -58,7 +58,7 @@ class Process implements \IteratorAggregate
     private $lastOutputTime;
     private $timeout;
     private $idleTimeout;
-    private $options;
+    private $options = array('suppress_errors' => true);
     private $exitcode;
     private $fallbackStatus = array();
     private $processInformation;
@@ -145,7 +145,7 @@ class Process implements \IteratorAggregate
      *
      * @throws RuntimeException When proc_open is not installed
      */
-    public function __construct($commandline, $cwd = null, array $env = null, $input = null, $timeout = 60, array $options = array())
+    public function __construct($commandline, $cwd = null, array $env = null, $input = null, $timeout = 60, array $options = null)
     {
         if (!function_exists('proc_open')) {
             throw new RuntimeException('The Process class relies on proc_open, which is not available on your PHP installation.');
@@ -171,7 +171,10 @@ public function __construct($commandline, $cwd = null, array $env = null, $input
         $this->pty = false;
         $this->enhanceWindowsCompatibility = true;
         $this->enhanceSigchildCompatibility = '\\' !== DIRECTORY_SEPARATOR && $this->isSigchildEnabled();
-        $this->options = array_replace(array('suppress_errors' => true, 'binary_pipes' => true), $options);
+        if (null !== $options) {
+            @trigger_error(sprintf('The $options parameter of the %s constructor is deprecated since version 3.3 and will be removed in 4.0.', __CLASS__), E_USER_DEPRECATED);
+            $this->options = $options + $this->options;
+        }
     }
 
     public function __destruct()
@@ -274,24 +277,23 @@ public function start(callable $callback = null)
             if ('\\' === DIRECTORY_SEPARATOR && !empty($this->options['bypass_shell']) && !$this->enhanceWindowsCompatibility) {
                 throw new LogicException('The "bypass_shell" option must be false to inherit environment variables while enhanced Windows compatibility is off');
             }
+            $escape = '\\' === DIRECTORY_SEPARATOR ? function ($v) { return ProcessUtils::escapeArgument($v, ProcessUtils::ESC_WINDOWS_CMD); } : 'escapeshellarg';
             $env = '\\' === DIRECTORY_SEPARATOR ? '(SET %s)&&' : 'export %s;';
             foreach ($this->env as $k => $v) {
-                $envline .= sprintf($env, ProcessUtils::escapeArgument("$k=$v"));
+                $envline .= sprintf($env, $escape("$k=$v"));
             }
             $env = null;
         } else {
             $env = $this->env;
         }
         if ('\\' === DIRECTORY_SEPARATOR && $this->enhanceWindowsCompatibility) {
-            $commandline = 'cmd /V:ON /E:ON /D /C "('.$envline.$commandline.')';
+            $commandline = 'cmd /V:OFF /E:ON /D /C "('.$envline.$commandline.')';
             foreach ($this->processPipes->getFiles() as $offset => $filename) {
-                $commandline .= ' '.$offset.'>'.ProcessUtils::escapeArgument($filename);
+                $commandline .= ' '.$offset.'>'.ProcessUtils::escapeArgument($filename, ProcessUtils::ESC_WINDOWS_CMD);
             }
             $commandline .= '"';
 
-            if (!isset($this->options['bypass_shell'])) {
-                $this->options['bypass_shell'] = true;
-            }
+            $this->options['bypass_shell'] = true;
         } elseif (!$this->useFileHandles && $this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
             // last exit code is output on the fourth pipe and caught to work around --enable-sigchild
             $descriptors[3] = array('pipe', 'w');
@@ -1148,9 +1150,13 @@ public function setInput($input)
      * Gets the options for proc_open.
      *
      * @return array The current options
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function getOptions()
     {
+        @trigger_error(sprintf('The %s method is deprecated since version 3.3 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->options;
     }
 
@@ -1160,9 +1166,13 @@ public function getOptions()
      * @param array $options The new options
      *
      * @return self The current Process instance
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function setOptions(array $options)
     {
+        @trigger_error(sprintf('The %s method is deprecated since version 3.3 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
+
         $this->options = $options;
 
         return $this;
@@ -1174,9 +1184,13 @@ public function setOptions(array $options)
      * This is true by default.
      *
      * @return bool
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function getEnhanceWindowsCompatibility()
     {
+        @trigger_error(sprintf('The %s method is deprecated since version 3.3 and will be removed in 4.0. Enhanced Windows compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->enhanceWindowsCompatibility;
     }
 
@@ -1186,9 +1200,13 @@ public function getEnhanceWindowsCompatibility()
      * @param bool $enhance
      *
      * @return self The current Process instance
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function setEnhanceWindowsCompatibility($enhance)
     {
+        @trigger_error(sprintf('The %s method is deprecated since version 3.3 and will be removed in 4.0. Enhanced Windows compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         $this->enhanceWindowsCompatibility = (bool) $enhance;
 
         return $this;
@@ -1198,9 +1216,13 @@ public function setEnhanceWindowsCompatibility($enhance)
      * Returns whether sigchild compatibility mode is activated or not.
      *
      * @return bool
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
     */
     public function getEnhanceSigchildCompatibility()
     {
+        @trigger_error(sprintf('The %s method is deprecated since version 3.3 and will be removed in 4.0. SIGCHLD compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->enhanceSigchildCompatibility;
     }
 
@@ -1214,9 +1236,13 @@ public function getEnhanceSigchildCompatibility()
      * @param bool $enhance
      *
      * @return self The current Process instance
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function setEnhanceSigchildCompatibility($enhance)
     {
+        @trigger_error(sprintf('The %s method is deprecated since version 3.3 and will be removed in 4.0. SIGCHLD compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         $this->enhanceSigchildCompatibility = (bool) $enhance;
 
         return $this;

diff --git a/src/Symfony/Component/Process/ProcessBuilder.php b/src/Symfony/Component/Process/ProcessBuilder.php
@@ -26,7 +26,7 @@ class ProcessBuilder
     private $env = array();
     private $input;
     private $timeout = 60;
-    private $options = array();
+    private $options;
     private $inheritEnv = true;
     private $prefix = array();
     private $outputDisabled = false;
@@ -217,9 +217,13 @@ public function setTimeout($timeout)
      * @param string $value The option value
      *
      * @return $this
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function setOption($name, $value)
     {
+        @trigger_error(sprintf('The %s method is deprecated since version 3.3 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
+
         $this->options[$name] = $value;
 
         return $this;
@@ -262,12 +266,10 @@ public function getProcess()
             throw new LogicException('You must add() command arguments before calling getProcess().');
         }
 
-        $options = $this->options;
-
         $arguments = array_merge($this->prefix, $this->arguments);
         $script = implode(
2A39
' ', array_map(array(__NAMESPACE__.'\\ProcessUtils', 'escapeArgument'), $arguments));
 
-        $process = new Process($script, $this->cwd, $this->env, $this->input, $this->timeout, $options);
+        $process = new Process($script, $this->cwd, $this->env, $this->input, $this->timeout, $this->options);
 
         if ($this->inheritEnv) {
             $process->inheritEnvironmentVariables();

diff --git a/src/Symfony/Component/Process/ProcessUtils.php b/src/Symfony/Component/Process/ProcessUtils.php
@@ -17,11 +17,13 @@
  * ProcessUtils is a bunch of utility methods.
  *
  * This class contains static methods only and is not meant to be instantiated.
- *
- * @author Martin Hasoň <martin.hason@gmail.com>
  */
 class ProcessUtils
 {
+    const ESC_WINDOWS_ARGV = 1;
+    const ESC_WINDOWS_CMD = 2;
+    const ESC_WINDOWS_ARGV_CMD = 3;
+
     /**
      * This class should not be instantiated.
      */
@@ -32,46 +34,42 @@ private function __construct()
     /**
      * Escapes a string to be used as a shell argument.
      *
+     * Provides a more robust method on Windows than escapeshellarg.
+     * See https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
+     *
      * @param string $argument The argument that will be escaped
+     * @param int    $mode     A bitfield of self::ESC_WINDOWS_* constants to configure escaping context on Windows
      *
      * @return string The escaped argument
      */
-    public static function escapeArgument($argument)
+    public static function escapeArgument($argument, $mode = self::ESC_WINDOWS_ARGV_CMD)
     {
-        //Fix for PHP bug #43784 escapeshellarg removes % from given string
-        //Fix for PHP bug #49446 escapeshellarg doesn't work on Windows
-        //@see https://bugs.php.net/bug.php?id=43784
-        //@see https://bugs.php.net/bug.php?id=49446
-        if ('\\' === DIRECTORY_SEPARATOR) {
-            if ('' === $argument) {
-                return escapeshellarg($argument);
-            }
+        if ('\\' !== DIRECTORY_SEPARATOR) {
+            return escapeshellarg($argument);
+        }
+        if (!(self::ESC_WINDOWS_ARGV_CMD & $mode)) {
+            throw new \InvalidArgumentException(sprintf('The $mode argument of %s must be a non-zero bitfield of self::ESC_WINDOWS_* constants.', __METHOD__));
+        }
 
-            $escapedArgument = '';
-            $quote = false;
-            foreach (preg_split('/(")/', $argument, -1, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE) as $part) {
-                if ('"' === $part) {
-                    $escapedArgument .= '\\"';
-                } elseif (self::isSurroundedBy($part, '%')) {
-                    // Avoid environment variable expansion
-                    $escapedArgument .= '^%"'.substr($part, 1, -1).'"^%';
-                } else {
-                    // escape trailing backslash
-                    if ('\\' === substr($part, -1)) {
-                        $part .= '\\';
-                    }
-                    $quote = true;
-                    $escapedArgument .= $part;
-                }
-            }
-            if ($quote) {
-                $escapedArgument = '"'.$escapedArgument.'"';
-            }
+        if (1 === func_num_args()) {
+            $argument = preg_replace_callback("/!([^!=\n]++)!/", function ($m) {
+                @trigger_error(sprintf('Delayed variables are deprecated since Symfony 3.3 and will be left unresolved in 4.0. Resolve the %s variable before calling ProcessUtil::escapeArgument().', $m[0]), E_USER_DEPRECATED);
 
-            return $escapedArgument;
+                return getenv($m[1]);
+            }, $argument);
         }
 
-        return escapeshellarg($argument);
+        if ((self::ESC_WINDOWS_ARGV & $mode) && (false !== strpbrk($argument, " \t\n\v\"") || !isset($argument[0]))) {
+            $argument = preg_replace('/(\\\\*+)"/', '$1$1\\"', $argument);
+            $argument = preg_replace('/(\\\\++)$/', '$1$1', $argument);
+            $argument = '"'.$argument.'"';
+        }
+
+        if (self::ESC_WINDOWS_CMD & $mode) {
+            $argument = preg_replace('/[()%!^"<>&|]/', '^$0', $argument);
+        }
+
+        return $argument;
     }
 
     /**
@@ -111,9 +109,4 @@ public static function validateInput($caller, $input)
 
         return $input;
     }
-
-    private static function isSurroundedBy($arg, $char)
-    {
-        return 2 < strlen($arg) && $char === $arg[0] && $char === $arg[strlen($arg) - 1];
-    }
 }