[llvm] r180035 - [Support] Fix argv string escape bug on Windows
Dimitry Andric
dimitry at andric.com
Sat Apr 27 14:59:29 PDT 2013
Hi,
The ProgramTest.cpp test always fails on FreeBSD (and any other BSD), since the part:
Path my_exe = Path::GetMainExecutable("SupportTests", &ProgramTestStringArg1);
always results in 'my_exe' being empty there. This is because the executable is not run from the PATH, so GetMainExecutable() cannot find it. Other OSes have different ways to get at the running executable name, so there are no failures there.
As discussed a bit on IRC, there are several ways of improving this. The most reasonable way would be to add something to llvm::cl::ParseCommandLineOptions() to make it save the original argv[0] (or the whole argv array) somewhere, and make it accessible via some sort of interface. Currently it does store ProgramName in a static variable in lib/Support/CommandLine.cpp, but there is no way to get at it...
Another option would be to pass the actual program name as a macro during compilation, since the build system will know in advance exactly where the SupportTests executable is located. This may be complicated with automake or CMake, though; I am not sure.
Finally, as a minor detail, the ProgramTest.cpp file has its executable bit set. This was probably inherited from a Windows file share?
-Dimitry
On Apr 22, 2013, at 21:03, Reid Kleckner <reid at kleckner.net> wrote:
> Author: rnk
> Date: Mon Apr 22 14:03:55 2013
> New Revision: 180035
>
> URL: http://llvm.org/viewvc/llvm-project?rev=180035&view=rev
> Log:
> [Support] Fix argv string escape bug on Windows
>
> Summary:
> 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.
>
> Reviewers: Bigcheese
>
> CC: llvm-commits
>
> Differential Revision: http://llvm-reviews.chandlerc.com/D705
>
> Added:
> llvm/trunk/unittests/Support/ProgramTest.cpp (with props)
> Modified:
> llvm/trunk/lib/Support/Windows/Program.inc
> llvm/trunk/unittests/Support/CMakeLists.txt
>
> Modified: llvm/trunk/lib/Support/Windows/Program.inc
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Program.inc?rev=180035&r1=180034&r2=180035&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Support/Windows/Program.inc (original)
> +++ llvm/trunk/lib/Support/Windows/Program.inc Mon Apr 22 14:03:55 2013
> @@ -126,15 +126,46 @@ static bool ArgNeedsQuotes(const char *S
> 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 @@ Program::Execute(const Path& path,
>
> 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++ = ' ';
> }
>
>
> Modified: llvm/trunk/unittests/Support/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CMakeLists.txt?rev=180035&r1=180034&r2=180035&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/Support/CMakeLists.txt (original)
> +++ llvm/trunk/unittests/Support/CMakeLists.txt Mon Apr 22 14:03:55 2013
> @@ -23,6 +23,7 @@ add_llvm_unittest(SupportTests
> MemoryTest.cpp
> Path.cpp
> ProcessTest.cpp
> + ProgramTest.cpp
> RegexTest.cpp
> SwapByteOrderTest.cpp
> TimeValue.cpp
>
> Added: llvm/trunk/unittests/Support/ProgramTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ProgramTest.cpp?rev=180035&view=auto
> ==============================================================================
> --- llvm/trunk/unittests/Support/ProgramTest.cpp (added)
> +++ llvm/trunk/unittests/Support/ProgramTest.cpp Mon Apr 22 14:03:55 2013
> @@ -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
>
> Propchange: llvm/trunk/unittests/Support/ProgramTest.cpp
> ------------------------------------------------------------------------------
> svn:executable = *
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130427/057c3663/attachment.html>
More information about the llvm-commits
mailing list