Bug 1601905 - Add quotes for a path including whitespaces in ShellExecuteWithIFile. r=aklotz

A fix for Bug 1588975 replaced the call to `LaunchWithIProcess` in
`nsMIMEInfoWin::LaunchWithFile` with the call to `ShellExecuteWithIFile` to use
`mozilla::ShellExecuteByExplorer`.  This caused a regression that if a filepath
contains whitespaces, we pass it to a target application without quoting it
while the old codepath `ShellExecuteWithIFile` quoted a path accordingly in
`assembleCmdLine`.

A proposed fix is to quote a path in the same way as `assembleCmdLine` does.
This patch also moves `assembleCmdLine` to an exported header so that we can add
a unittest to cover both of `assembleCmdLine` and a new function
`assembleSingleArgument.  It would be better to refactor these functions in the
future because many lines are duplicated.

Differential Revision: https://phabricator.services.mozilla.com/D56646

--HG--
extra : moz-landing-system : lando

toolkit/xre/AssembleCmdLine.h

@@ -0,0 +1,228 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#ifndef mozilla_AssembleCmdLine_h
+#define mozilla_AssembleCmdLine_h
+
+#if defined(XP_WIN)
+
+#  include "mozilla/UniquePtr.h"
+
+#  include <stdlib.h>
+#  include <windows.h>
+
+#  ifdef MOZILLA_INTERNAL_API
+#    include "nsString.h"
+#  endif  // MOZILLA_INTERNAL_API
+
+namespace mozilla {
+
+// Out param `aWideCmdLine` must be free()d by the caller.
+inline int assembleCmdLine(const char* const* aArgv, wchar_t** aWideCmdLine,
+                           UINT aCodePage) {
+  const char* const* arg;
+  char* p;
+  const char* q;
+  char* cmdLine;
+  int cmdLineSize;
+  int numBackslashes;
+  int i;
+  int argNeedQuotes;
+
+  /*
+   * Find out how large the command line buffer should be.
+   */
+  cmdLineSize = 0;
+  for (arg = aArgv; *arg; ++arg) {
+    /*
+     * \ and " need to be escaped by a \.  In the worst case,
+     * every character is a \ or ", so the string of length
+     * may double.  If we quote an argument, that needs two ".
+     * Finally, we need a space between arguments, and
+     * a null byte at the end of command line.
+     */
+    cmdLineSize += 2 * strlen(*arg) /* \ and " need to be escaped */
+                   + 2              /* we quote every argument */
+                   + 1;             /* space in between, or final null */
+  }
+  p = cmdLine = (char*)malloc(cmdLineSize * sizeof(char));
+  if (!p) {
+    return -1;
+  }
+
+  for (arg = aArgv; *arg; ++arg) {
+    /* Add a space to separates the arguments */
+    if (arg != aArgv) {
+      *p++ = ' ';
+    }
+    q = *arg;
+    numBackslashes = 0;
+    argNeedQuotes = 0;
+
+    /* If the argument contains white space, it needs to be quoted. */
+    if (strpbrk(*arg, " \f\n\r\t\v")) {
+      argNeedQuotes = 1;
+    }
+
+    if (argNeedQuotes) {
+      *p++ = '"';
+    }
+    while (*q) {
+      if (*q == '\\') {
+        numBackslashes++;
+        q++;
+      } else if (*q == '"') {
+        if (numBackslashes) {
+          /*
+           * Double the backslashes since they are followed
+           * by a quote
+           */
+          for (i = 0; i < 2 * numBackslashes; i++) {
+            *p++ = '\\';
+          }
+          numBackslashes = 0;
+        }
+        /* To escape the quote */
+        *p++ = '\\';
+        *p++ = *q++;
+      } else {
+        if (numBackslashes) {
+          /*
+           * Backslashes are not followed by a quote, so
+           * don't need to double the backslashes.
+           */
+          for (i = 0; i < numBackslashes; i++) {
+            *p++ = '\\';
+          }
+          numBackslashes = 0;
+        }
+        *p++ = *q++;
+      }
+    }
+
+    /* Now we are at the end of this argument */
+    if (numBackslashes) {
+      /*
+       * Double the backslashes if we have a quote string
+       * delimiter at the end.
+       */
+      if (argNeedQuotes) {
+        numBackslashes *= 2;
+      }
+      for (i = 0; i < numBackslashes; i++) {
+        *p++ = '\\';
+      }
+    }
+    if (argNeedQuotes) {
+      *p++ = '"';
+    }
+  }
+
+  *p = '\0';
+  int numChars = MultiByteToWideChar(aCodePage, 0, cmdLine, -1, nullptr, 0);
+  *aWideCmdLine = (wchar_t*)malloc(numChars * sizeof(wchar_t));
+  MultiByteToWideChar(aCodePage, 0, cmdLine, -1, *aWideCmdLine, numChars);
+  free(cmdLine);
+  return 0;
+}
+
+#  ifdef MOZILLA_INTERNAL_API
+inline UniquePtr<wchar_t[]> assembleSingleArgument(const nsString& aArg) {
+  static_assert(sizeof(char16_t) == sizeof(wchar_t),
+                "char16_t and wchar_t sizes differ");
+
+  /*
+   * \ and " need to be escaped by a \.  In the worst case,
+   * every character is a \ or ", so the string of length
+   * may double.  If we quote an argument, that needs two ".
+   * Finally, we need a space between arguments, and
+   * a null byte at the end of command line.
+   */
+  int cmdLineSize = 2 * aArg.Length() /* \ and " need to be escaped */
+                    + 2               /* we quote every argument */
+                    + 1;              /* space in between, or final null */
+
+  auto assembledArg = MakeUnique<wchar_t[]>(cmdLineSize);
+  if (!assembledArg) {
+    return nullptr;
+  }
+
+  /* If the argument contains white space, it needs to be quoted. */
+  int argNeedQuotes = 0;
+  if (aArg.FindCharInSet(u" \f\n\r\t\v") != kNotFound) {
+    argNeedQuotes = 1;
+  }
+
+  wchar_t* p = assembledArg.get();
+
+  if (argNeedQuotes) {
+    *p++ = '"';
+  }
+
+  const char16_t* q = aArg.get();
+
+  int numBackslashes = 0;
+  while (*q) {
+    if (*q == '\\') {
+      numBackslashes++;
+      q++;
+    } else if (*q == '"') {
+      if (numBackslashes) {
+        /*
+         * Double the backslashes since they are followed
+         * by a quote
+         */
+        for (int i = 0; i < 2 * numBackslashes; i++) {
+          *p++ = '\\';
+        }
+        numBackslashes = 0;
+      }
+      /* To escape the quote */
+      *p++ = '\\';
+      *p++ = *q++;
+    } else {
+      if (numBackslashes) {
+        /*
+         * Backslashes are not followed by a quote, so
+         * don't need to double the backslashes.
+         */
+        for (int i = 0; i < numBackslashes; i++) {
+          *p++ = '\\';
+        }
+        numBackslashes = 0;
+      }
+      *p++ = *q++;
+    }
+  }
+
+  /* Now we are at the end of this argument */
+  if (numBackslashes) {
+    /*
+     * Double the backslashes if we have a quote string
+     * delimiter at the end.
+     */
+    if (argNeedQuotes) {
+      numBackslashes *= 2;
+    }
+    for (int i = 0; i < numBackslashes; i++) {
+      *p++ = '\\';
+    }
+  }
+  if (argNeedQuotes) {
+    *p++ = '"';
+  }
+
+  *p = '\0';
+
+  return assembledArg;
+}
+#  endif  // MOZILLA_INTERNAL_API
+
+}  // namespace mozilla
+
+#endif  // defined(XP_WIN)
+
+#endif  // mozilla_AssembleCmdLine_h

toolkit/xre/moz.build

@@ -46,6 +46,7 @@ if CONFIG['MOZ_INSTRUMENT_EVENT_LOOP']:
 
 if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows':
     EXPORTS.mozilla += [
+        'AssembleCmdLine.h',
         'ModuleVersionInfo.h',
         'PolicyChecks.h',
         'UntrustedModulesProcessor.h',

toolkit/xre/test/gtest/TestAssembleCommandLineWin.cpp

@@ -0,0 +1,96 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "gtest/gtest.h"
+
+#include "mozilla/AssembleCmdLine.h"
+#include "mozilla/UniquePtrExtensions.h"
+
+using namespace mozilla;
+
+template <typename T>
+struct TestCase {
+  const T* mArgs[4];
+  const wchar_t* mExpected;
+};
+
+#define ALPHA_IN_UTF8 "\xe3\x82\xa2\xe3\x83\xab\xe3\x83\x95\xe3\x82\xa1"
+#define OMEGA_IN_UTF8 "\xe3\x82\xaa\xe3\x83\xa1\xe3\x82\xac"
+#define ALPHA_IN_UTF16 L"\u30A2\u30EB\u30D5\u30A1"
+#define OMEGA_IN_UTF16 L"\u30AA\u30E1\u30AC"
+
+TestCase<char> testCases[] = {
+    // Copied from TestXREMakeCommandLineWin.ini
+    {{"a:\\", nullptr}, L"a:\\"},
+    {{"a:\"", nullptr}, L"a:\\\""},
+    {{"a:\\b c", nullptr}, L"\"a:\\b c\""},
+    {{"a:\\b c\"", nullptr}, L"\"a:\\b c\\\"\""},
+    {{"a:\\b c\\d e", nullptr}, L"\"a:\\b c\\d e\""},
+    {{"a:\\b c\\d e\"", nullptr}, L"\"a:\\b c\\d e\\\"\""},
+    {{"a:\\", nullptr}, L"a:\\"},
+    {{"a:\"", "b:\\c d", nullptr}, L"a:\\\" \"b:\\c d\""},
+    {{"a", "b:\" c:\\d", "e", nullptr}, L"a \"b:\\\" c:\\d\" e"},
+    {{"abc", "d", "e", nullptr}, L"abc d e"},
+    {{"a b c", "d", "e", nullptr}, L"\"a b c\" d e"},
+    {{"a\\\\\\b", "de fg", "h", nullptr}, L"a\\\\\\b \"de fg\" h"},
+    {{"a", "b", nullptr}, L"a b"},
+    {{"a\tb", nullptr}, L"\"a\tb\""},
+    {{"a\\\"b", "c", "d", nullptr}, L"a\\\\\\\"b c d"},
+    {{"a\\\"b", "c", nullptr}, L"a\\\\\\\"b c"},
+    {{"a\\\\\\b c", nullptr}, L"\"a\\\\\\b c\""},
+    {{"\"a", nullptr}, L"\\\"a"},
+    {{"\\a", nullptr}, L"\\a"},
+    {{"\\\\\\a", nullptr}, L"\\\\\\a"},
+    {{"\\\\\\\"a", nullptr}, L"\\\\\\\\\\\\\\\"a"},
+    {{"a\\\"b c\" d e", nullptr}, L"\"a\\\\\\\"b c\\\" d e\""},
+    {{"a\\\\\"b", "c d e", nullptr}, L"a\\\\\\\\\\\"b \"c d e\""},
+    {{"a:\\b", "c\\" ALPHA_IN_UTF8, OMEGA_IN_UTF8 "\\d", nullptr},
+     L"a:\\b c\\" ALPHA_IN_UTF16 L" " OMEGA_IN_UTF16 L"\\d"},
+    {{"a:\\b", "c\\" ALPHA_IN_UTF8 " " OMEGA_IN_UTF8 "\\d", nullptr},
+     L"a:\\b \"c\\" ALPHA_IN_UTF16 L" " OMEGA_IN_UTF16 L"\\d\""},
+    {{ALPHA_IN_UTF8, OMEGA_IN_UTF8, nullptr},
+     ALPHA_IN_UTF16 L" " OMEGA_IN_UTF16},
+
+    // More single-argument cases
+    {{"", nullptr}, L""},
+    {{"a\fb", nullptr}, L"\"a\fb\""},
+    {{"a\nb", nullptr}, L"\"a\nb\""},
+    {{"a\rb", nullptr}, L"\"a\rb\""},
+    {{"a\vb", nullptr}, L"\"a\vb\""},
+    {{"\"a\" \"b\"", nullptr}, L"\"\\\"a\\\" \\\"b\\\"\""},
+    {{"\"a\\b\" \"c\\d\"", nullptr}, L"\"\\\"a\\b\\\" \\\"c\\d\\\"\""},
+    {{"\\\\ \\\\", nullptr}, L"\"\\\\ \\\\\\\\\""},
+    {{"\"\" \"\"", nullptr}, L"\"\\\"\\\" \\\"\\\"\""},
+    {{ALPHA_IN_UTF8 "\\" OMEGA_IN_UTF8, nullptr},
+     ALPHA_IN_UTF16 L"\\" OMEGA_IN_UTF16},
+    {{ALPHA_IN_UTF8 " " OMEGA_IN_UTF8, nullptr},
+     L"\"" ALPHA_IN_UTF16 L" " OMEGA_IN_UTF16 L"\""},
+};
+
+TEST(AssembleCommandLineWin, assembleCmdLine)
+{
+  for (const auto& testCase : testCases) {
+    UniqueFreePtr<wchar_t> assembled;
+    wchar_t* assembledRaw = nullptr;
+    EXPECT_EQ(assembleCmdLine(testCase.mArgs, &assembledRaw, CP_UTF8), 0);
+    assembled.reset(assembledRaw);
+
+    EXPECT_STREQ(assembled.get(), testCase.mExpected);
+  }
+}
+
+TEST(AssembleCommandLineWin, assembleSingleArgument)
+{
+  for (const auto& testCase : testCases) {
+    if (testCase.mArgs[1]) {
+      // Skip a case having more than one argument.
+      continue;
+    }
+
+    auto assembled =
+        assembleSingleArgument(NS_ConvertUTF8toUTF16(testCase.mArgs[0]));
+    EXPECT_STREQ(assembled.get(), testCase.mExpected);
+  }
+}

toolkit/xre/test/gtest/moz.build

@@ -10,4 +10,9 @@ UNIFIED_SOURCES = [
     'TestCompatVersionCompare.cpp',
 ]
 
+if CONFIG['OS_TARGET'] == 'WINNT':
+    UNIFIED_SOURCES += [
+        'TestAssembleCommandLineWin.cpp',
+    ]
+
 FINAL_LIBRARY = 'xul-gtest'

uriloader/exthandler/win/nsMIMEInfoWin.cpp

@@ -20,6 +20,7 @@
 #include "nsUnicharUtils.h"
 #include "nsITextToSubURI.h"
 #include "nsVariant.h"
+#include "mozilla/AssembleCmdLine.h"
 #include "mozilla/ShellHeaderOnlyUtils.h"
 #include "mozilla/UrlmonHeaderOnlyUtils.h"
 #include "mozilla/UniquePtrExtensions.h"
@@ -41,7 +42,7 @@ nsresult nsMIMEInfoWin::LaunchDefaultWithFile(nsIFile* aFile) {
 
 // Helper routine to call mozilla::ShellExecuteByExplorer
 static nsresult ShellExecuteWithIFile(const nsCOMPtr<nsIFile>& aExecutable,
-                                      const _variant_t& aArgs) {
+                                      const nsString& aArgs) {
   nsresult rv;
 
   nsAutoString execPath;
@@ -53,6 +54,11 @@ static nsresult ShellExecuteWithIFile(const nsCOMPtr<nsIFile>& aExecutable,
     return rv;
   }
 
+  auto assembledArgs = mozilla::assembleSingleArgument(aArgs);
+  if (!assembledArgs) {
+    return NS_ERROR_FILE_EXECUTION_FAILED;
+  }
+
   _bstr_t execPathBStr(execPath.get());
   // Pass VT_ERROR/DISP_E_PARAMNOTFOUND to omit an optional RPC parameter
   // to execute a file with the default verb.
@@ -64,7 +70,7 @@ static nsresult ShellExecuteWithIFile(const nsCOMPtr<nsIFile>& aExecutable,
   // Skype for Business do not start correctly when inheriting our process's
   // migitation policies.
   mozilla::LauncherVoidResult shellExecuteOk = mozilla::ShellExecuteByExplorer(
-      execPathBStr, aArgs, verbDefault, workingDir, showCmd);
+      execPathBStr, assembledArgs.get(), verbDefault, workingDir, showCmd);
   if (shellExecuteOk.isErr()) {
     return NS_ERROR_FILE_EXECUTION_FAILED;
   }
@@ -96,9 +102,6 @@ nsMIMEInfoWin::LaunchWithFile(nsIFile* aFile) {
     rv = localHandler->GetExecutable(getter_AddRefs(executable));
     NS_ENSURE_SUCCESS(rv, rv);
 
-    nsAutoString path;
-    aFile->GetPath(path);
-
     // Deal with local dll based handlers
     nsCString filename;
     executable->GetNativeLeafName(filename);
@@ -162,7 +165,9 @@ nsMIMEInfoWin::LaunchWithFile(nsIFile* aFile) {
         return NS_ERROR_FILE_EXECUTION_FAILED;
       }
     }
-    return ShellExecuteWithIFile(executable, _variant_t(path.get()));
+    nsAutoString path;
+    aFile->GetPath(path);
+    return ShellExecuteWithIFile(executable, path);
   }
 
   return NS_ERROR_INVALID_ARG;

xpcom/threads/nsProcessCommon.cpp

@@ -33,6 +33,7 @@
 #  include "nsString.h"
 #  include "nsLiteralString.h"
 #  include "nsReadableUtils.h"
+#  include "mozilla/AssembleCmdLine.h"
 #  include "mozilla/UniquePtrExtensions.h"
 #else
 #  ifdef XP_MACOSX
@@ -109,117 +110,6 @@ nsProcess::Init(nsIFile* aExecutable) {
   return rv;
 }
 
-#if defined(XP_WIN)
-// Out param `aWideCmdLine` must be free()d by the caller.
-static int assembleCmdLine(char* const* aArgv, wchar_t** aWideCmdLine,
-                           UINT aCodePage) {
-  char* const* arg;
-  char* p;
-  char* q;
-  char* cmdLine;
-  int cmdLineSize;
-  int numBackslashes;
-  int i;
-  int argNeedQuotes;
-
-  /*
-   * Find out how large the command line buffer should be.
-   */
-  cmdLineSize = 0;
-  for (arg = aArgv; *arg; ++arg) {
-    /*
-     * \ and " need to be escaped by a \.  In the worst case,
-     * every character is a \ or ", so the string of length
-     * may double.  If we quote an argument, that needs two ".
-     * Finally, we need a space between arguments, and
-     * a null byte at the end of command line.
-     */
-    cmdLineSize += 2 * strlen(*arg) /* \ and " need to be escaped */
-                   + 2              /* we quote every argument */
-                   + 1;             /* space in between, or final null */
-  }
-  p = cmdLine = (char*)malloc(cmdLineSize * sizeof(char));
-  if (!p) {
-    return -1;
-  }
-
-  for (arg = aArgv; *arg; ++arg) {
-    /* Add a space to separates the arguments */
-    if (arg != aArgv) {
-      *p++ = ' ';
-    }
-    q = *arg;
-    numBackslashes = 0;
-    argNeedQuotes = 0;
-
-    /* If the argument contains white space, it needs to be quoted. */
-    if (strpbrk(*arg, " \f\n\r\t\v")) {
-      argNeedQuotes = 1;
-    }
-
-    if (argNeedQuotes) {
-      *p++ = '"';
-    }
-    while (*q) {
-      if (*q == '\\') {
-        numBackslashes++;
-        q++;
-      } else if (*q == '"') {
-        if (numBackslashes) {
-          /*
-           * Double the backslashes since they are followed
-           * by a quote
-           */
-          for (i = 0; i < 2 * numBackslashes; i++) {
-            *p++ = '\\';
-          }
-          numBackslashes = 0;
-        }
-        /* To escape the quote */
-        *p++ = '\\';
-        *p++ = *q++;
-      } else {
-        if (numBackslashes) {
-          /*
-           * Backslashes are not followed by a quote, so
-           * don't need to double the backslashes.
-           */
-          for (i = 0; i < numBackslashes; i++) {
-            *p++ = '\\';
-          }
-          numBackslashes = 0;
-        }
-        *p++ = *q++;
-      }
-    }
-
-    /* Now we are at the end of this argument */
-    if (numBackslashes) {
-      /*
-       * Double the backslashes if we have a quote string
-       * delimiter at the end.
-       */
-      if (argNeedQuotes) {
-        numBackslashes *= 2;
-      }
-      for (i = 0; i < numBackslashes; i++) {
-        *p++ = '\\';
-      }
-    }
-    if (argNeedQuotes) {
-      *p++ = '"';
-    }
-  }
-
-  *p = '\0';
-  int32_t numChars = MultiByteToWideChar(aCodePage, 0, cmdLine, -1, nullptr, 0);
-  *aWideCmdLine = (wchar_t*)malloc(numChars * sizeof(wchar_t));
-  MultiByteToWideChar(aCodePage, 0, cmdLine, -1, *aWideCmdLine, numChars);
-  free(cmdLine);
-  return 0;
-}
-#endif
-
 void nsProcess::Monitor(void* aArg) {
   RefPtr<nsProcess> process = dont_AddRef(static_cast<nsProcess*>(aArg));