Factor out command-line quoting code on Windows.

Our command line code is not very good with respect to getting
quoting right.  Because of this, callers will sometimes themselves
attempt to quote arguments, though some of them get it wrong(!) by
just surrounding with quotes.

The first step is to add a quoting function that we think is correct
and update the unit test to test it.

(Note that most of this is Windows-specific, because on POSIX
we can pass arguments to commands as a vector -- trying to do
quoting on POSIX is usually wrong.)

Review URL: http://codereview.chromium.org/3007038

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@55591 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 80 insertions, 16 deletions

diff --git a/base/command_line.cc b/base/command_line.cc
index 835ad9d..906c8f4 100644
--- a/base/command_line.cc
+++ b/base/command_line.cc
@@ -114,6 +114,7 @@ CommandLine CommandLine::FromString(const std::wstring& command_line) {
 CommandLine::CommandLine(const FilePath& program) {
   if (!program.empty()) {
     program_ = program.value();
+    // TODO(evanm): proper quoting here.
     command_line_string_ = L'"' + program.value() + L'"';
   }
 }
@@ -391,24 +392,55 @@ void CommandLine::AppendSwitchASCII(const std::string& switch_string,
   AppendSwitchNative(switch_string, ASCIIToWide(value_string));
 }
 
-void CommandLine::AppendSwitchNative(const std::string& switch_string,
-                                     const std::wstring& value_string) {
-  std::wstring value_string_edit;
-
-  // NOTE(jhughes): If the value contains a quotation mark at one
-  //                end but not both, you may get unusable output.
-  if (!value_string.empty() &&
-      (value_string.find(L" ") != std::wstring::npos) &&
-      (value_string[0] != L'"') &&
-      (value_string[value_string.length() - 1] != L'"')) {
-    // need to provide quotes
-    value_string_edit = StringPrintf(L"\"%ls\"", value_string.c_str());
-  } else {
-    value_string_edit = value_string;
+// Quote a string if necessary, such that CommandLineToArgvW() will
+// always process it as a single argument.
+static std::wstring WindowsStyleQuote(const std::wstring& arg) {
+  // We follow the quoting rules of CommandLineToArgvW.
+  // http://msdn.microsoft.com/en-us/library/17w5ykft.aspx
+  if (arg.find_first_of(L" \\\"") == std::wstring::npos) {
+    // No quoting necessary.
+    return arg;
   }
 
+  std::wstring out;
+  out.push_back(L'"');
+  for (size_t i = 0; i < arg.size(); ++i) {
+    if (arg[i] == '\\') {
+      // Find the extent of this run of backslashes.
+      size_t start = i, end = start + 1;
+      for (; end < arg.size() && arg[end] == '\\'; ++end)
+        /* empty */;
+      size_t backslash_count = end - start;
+
+      // Backslashes are escapes only if the run is followed by a double quote.
+      // Since we also will end the string with a double quote, we escape for
+      // either a double quote or the end of the string.
+      if (end == arg.size() || arg[end] == '"') {
+        // To quote, we need to output 2x as many backslashes.
+        backslash_count *= 2;
+      }
+      for (size_t j = 0; j < backslash_count; ++j)
+        out.push_back('\\');
+
+      // Advance i to one before the end to balance i++ in loop.
+      i = end - 1;
+    } else if (arg[i] == '"') {
+      out.push_back('\\');
+      out.push_back('"');
+    } else {
+      out.push_back(arg[i]);
+    }
+  }
+  out.push_back('"');
+
+  return out;
+}
+
+void CommandLine::AppendSwitchNative(const std::string& switch_string,
+                                     const std::wstring& value_string) {
+  std::wstring quoted_value_string = WindowsStyleQuote(value_string);
   std::wstring combined_switch_string =
-      PrefixedSwitchStringWithValue(switch_string, value_string_edit);
+      PrefixedSwitchStringWithValue(switch_string, quoted_value_string);
 
   command_line_string_.append(L" ");
   command_line_string_.append(combined_switch_string);
@@ -417,7 +449,8 @@ void CommandLine::AppendSwitchNative(const std::string& switch_string,
 }
 
 void CommandLine::AppendLooseValue(const std::wstring& value) {
-  // TODO(evan): quoting?
+  // TODO(evan): the quoting here is wrong, but current callers rely on it
+  // being wrong.  I have another branch which fixes all the callers.
   command_line_string_.append(L" ");
   command_line_string_.append(value);
   args_.push_back(value);
diff --git a/base/command_line_unittest.cc b/base/command_line_unittest.cc
index e44f240..8c8803c 100644
--- a/base/command_line_unittest.cc
+++ b/base/command_line_unittest.cc
@@ -11,6 +11,15 @@
 #include "base/string_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+// To test Windows quoting behavior, we use a string that has some backslashes
+// and quotes.
+// Consider the command-line argument: q\"bs1\bs2\\bs3q\\\"
+// Here it is with C-style escapes.
+#define TRICKY_QUOTED L"q\\\"bs1\\bs2\\\\bs3q\\\\\\\""
+// It should be parsed by Windows as: q"bs1\bs2\\bs3q\"
+// Here that is with C-style escapes.
+#define TRICKY L"q\"bs1\\bs2\\\\bs3q\\\""
+
 TEST(CommandLineTest, CommandLineConstructor) {
 #if defined(OS_WIN)
   CommandLine cl = CommandLine::FromString(
@@ -18,6 +27,7 @@ TEST(CommandLineTest, CommandLineConstructor) {
                      L"--other-switches=\"--dog=canine --cat=feline\" "
                      L"-spaetzle=Crepe   -=loosevalue  flan "
                      L"--input-translation=\"45\"--output-rotation "
+                     L"--quotes=" TRICKY_QUOTED L" "
                      L"-- -- --not-a-switch "
                      L"\"in the time of submarines...\"");
   EXPECT_FALSE(cl.command_line_string().empty());
@@ -51,6 +61,9 @@ TEST(CommandLineTest, CommandLineConstructor) {
 #endif
   EXPECT_TRUE(cl.HasSwitch("other-switches"));
   EXPECT_TRUE(cl.HasSwitch("input-translation"));
+#if defined(OS_WIN)
+  EXPECT_TRUE(cl.HasSwitch("quotes"));
+#endif
 
   EXPECT_EQ("Crepe", cl.GetSwitchValueASCII("spaetzle"));
   EXPECT_EQ("", cl.GetSwitchValueASCII("Foo"));
@@ -59,6 +72,9 @@ TEST(CommandLineTest, CommandLineConstructor) {
   EXPECT_EQ("--dog=canine --cat=feline", cl.GetSwitchValueASCII(
       "other-switches"));
   EXPECT_EQ("45--output-rotation", cl.GetSwitchValueASCII("input-translation"));
+#if defined(OS_WIN)
+  EXPECT_EQ(TRICKY, cl.GetSwitchValueNative("quotes"));
+#endif
 
   const std::vector<CommandLine::StringType>& args = cl.args();
   ASSERT_EQ(5U, args.size());
@@ -106,6 +122,8 @@ TEST(CommandLineTest, AppendSwitches) {
   std::string value3 = "a value with spaces";
   std::string switch4 = "switch4";
   std::string value4 = "\"a value with quotes\"";
+  std::string switch5 = "quotes";
+  std::string value5 = WideToASCII(TRICKY);
 
   CommandLine cl(FilePath(FILE_PATH_LITERAL("Program")));
 
@@ -113,6 +131,7 @@ TEST(CommandLineTest, AppendSwitches) {
   cl.AppendSwitchASCII(switch2, value);
   cl.AppendSwitchASCII(switch3, value3);
   cl.AppendSwitchASCII(switch4, value4);
+  cl.AppendSwitchASCII(switch5, value5);
 
   EXPECT_TRUE(cl.HasSwitch(switch1));
   EXPECT_TRUE(cl.HasSwitch(switch2));
@@ -121,4 +140,16 @@ TEST(CommandLineTest, AppendSwitches) {
   EXPECT_EQ(value3, cl.GetSwitchValueASCII(switch3));
   EXPECT_TRUE(cl.HasSwitch(switch4));
   EXPECT_EQ(value4, cl.GetSwitchValueASCII(switch4));
+  EXPECT_TRUE(cl.HasSwitch(switch5));
+  EXPECT_EQ(value5, cl.GetSwitchValueASCII(switch5));
+
+#if defined(OS_WIN)
+  EXPECT_EQ(L"\"Program\" "
+            L"--switch1 "
+            L"--switch2=value "
+            L"--switch3=\"a value with spaces\" "
+            L"--switch4=\"\\\"a value with quotes\\\"\" "
+            L"--quotes=\"" TRICKY_QUOTED L"\"",
+            cl.command_line_string());
+#endif
 }