[PATCH] [Support] Fix argv string escape bug on Windows

[Reid Kleckner]

Hi Bigcheese,

This is http://llvm.org/PR15802.  Backslashes preceding double quotes in
arguments must be escaped.  The interesting bit is that all other
backslashes should *not* be escaped, because the un-escaping logic is
only triggered by the presence of a double quote character.

http://llvm-reviews.chandlerc.com/D705

Files:
  lib/Support/Windows/Program.inc
  unittests/Support/CMakeLists.txt
  unittests/Support/ProgramTest.cpp

Index: lib/Support/Windows/Program.inc
===================================================================
--- lib/Support/Windows/Program.inc
+++ lib/Support/Windows/Program.inc
@@ -126,15 +126,46 @@
   return Str[0] == '\0' || strpbrk(Str, "\t \"&\'()*<>\\`^|") != 0;
 }
 
+/// CountPrecedingBackslashes - Returns the number of backslashes preceding Cur
+/// in the C string Start.
+static unsigned int CountPrecedingBackslashes(const char *Start,
+                                              const char *Cur) {
+  unsigned int Count = 0;
+  --Cur;
+  while (Cur >= Start && *Cur == '\\') {
+    ++Count;
+    --Cur;
+  }
+  return Count;
+}
+
+/// EscapePrecedingEscapes - Append a backslash to Dst for every backslash
+/// preceding Cur in the Start string.  Assumes Dst has enough space.
+static char *EscapePrecedingEscapes(char *Dst, const char *Start,
+                                    const char *Cur) {
+  unsigned PrecedingEscapes = CountPrecedingBackslashes(Start, Cur);
+  while (PrecedingEscapes > 0) {
+    *Dst++ = '\\';
+    --PrecedingEscapes;
+  }
+  return Dst;
+}
 
 /// ArgLenWithQuotes - Check whether argument needs to be quoted when calling
 /// CreateProcess and returns length of quoted arg with escaped quotes
 static unsigned int ArgLenWithQuotes(const char *Str) {
+  const char *Start = Str;
   unsigned int len = ArgNeedsQuotes(Str) ? 2 : 0;
 
   while (*Str != '\0') {
-    if (*Str == '\"')
-      ++len;
+    if (*Str == '\"') {
+      // We need to add a backslash, but ensure that it isn't escaped.
+      unsigned PrecedingEscapes = CountPrecedingBackslashes(Start, Str);
+      len += PrecedingEscapes + 1;
+    }
+    // Note that we *don't* need to escape runs of backslashes that don't
+    // precede a double quote!  See MSDN:
+    // http://msdn.microsoft.com/en-us/library/17w5ykft%28v=vs.85%29.aspx
 
     ++len;
     ++Str;
@@ -180,20 +211,27 @@
 
   for (unsigned i = 0; args[i]; i++) {
     const char *arg = args[i];
+    const char *start = arg;
 
     bool needsQuoting = ArgNeedsQuotes(arg);
     if (needsQuoting)
       *p++ = '"';
 
     while (*arg != '\0') {
-      if (*arg == '\"')
+      if (*arg == '\"') {
+        // Escape all preceding escapes (if any), and then escape the quote.
+        p = EscapePrecedingEscapes(p, start, arg);
         *p++ = '\\';
+      }
 
       *p++ = *arg++;
     }
 
-    if (needsQuoting)
+    if (needsQuoting) {
+      // Make sure our quote doesn't get escaped by a trailing backslash.
+      p = EscapePrecedingEscapes(p, start, arg);
       *p++ = '"';
+    }
     *p++ = ' ';
   }
 
Index: unittests/Support/CMakeLists.txt
===================================================================
--- unittests/Support/CMakeLists.txt
+++ unittests/Support/CMakeLists.txt
@@ -23,6 +23,7 @@
   MemoryTest.cpp
   Path.cpp
   ProcessTest.cpp
+  ProgramTest.cpp
   RegexTest.cpp
   SwapByteOrderTest.cpp
   TimeValue.cpp
Index: unittests/Support/ProgramTest.cpp
===================================================================
--- /dev/null
+++ unittests/Support/ProgramTest.cpp
@@ -0,0 +1,63 @@
+//===- unittest/Support/ProgramTest.cpp -----------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Program.h"
+#include "gtest/gtest.h"
+
+#include <stdlib.h>
+
+namespace {
+
+using namespace llvm;
+using namespace sys;
+
+static cl::opt<std::string>
+ProgramTestStringArg1("program-test-string-arg1");
+static cl::opt<std::string>
+ProgramTestStringArg2("program-test-string-arg2");
+
+TEST(ProgramTest, CreateProcessTrailingSlash) {
+  if (getenv("LLVM_PROGRAM_TEST_CHILD")) {
+    if (ProgramTestStringArg1 == "has\\\\ trailing\\" &&
+        ProgramTestStringArg2 == "has\\\\ trailing\\") {
+      exit(0);  // Success!  The arguments were passed and parsed.
+    }
+    exit(1);
+  }
+
+  // FIXME: Hardcoding argv0 here since I don't know a good cross-platform way
+  // to get it.  Maybe ParseCommandLineOptions() should save it?
+  Path my_exe = Path::GetMainExecutable("SupportTests", &ProgramTestStringArg1);
+  const char *argv[] = {
+    my_exe.c_str(),
+    "--gtest_filter=ProgramTest.CreateProcessTrailingSlashChild",
+    "-program-test-string-arg1", "has\\\\ trailing\\",
+    "-program-test-string-arg2", "has\\\\ trailing\\",
+    0
+  };
+  const char *envp[] = { "LLVM_PROGRAM_TEST_CHILD=1", 0 };
+  std::string error;
+  bool ExecutionFailed;
+  // Redirect stdout and stdin to NUL, but let stderr through.
+#ifdef LLVM_ON_WIN32
+  Path nul("NUL");
+#else
+  Path nul("/dev/null");
+#endif
+  const Path *redirects[] = { &nul, &nul, 0 };
+  int rc = Program::ExecuteAndWait(my_exe, argv, envp, redirects,
+                                   /*secondsToWait=*/10, /*memoryLimit=*/0,
+                                   &error, &ExecutionFailed);
+  EXPECT_FALSE(ExecutionFailed) << error;
+  EXPECT_EQ(0, rc);
+}
+
+} // end anonymous namespace
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D705.1.patch
Type: text/x-patch
Size: 5211 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130422/5ee23348/attachment.bin>