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

[Aaron Ballman]

LGTM!

~Aaron

On Mon, Apr 22, 2013 at 2:36 PM, Reid Kleckner <rnk at google.com> wrote:
> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>