From 6d17089b6cb10c6399c7790eda13eb1146665e89 Mon Sep 17 00:00:00 2001 From: Tony Abbott Date: Fri, 15 Apr 2016 16:45:41 +0200 Subject: [PATCH] GH #1222 Escape command line arguments passed to Process::launch() on Windows Conflicts: Foundation/testsuite/src/TestApp.cpp --- Foundation/src/Process_WIN32.cpp | 56 +++++++++++++++++++++- Foundation/src/Process_WIN32U.cpp | 54 ++++++++++++++++++++- Foundation/testsuite/src/ProcessTest.cpp | 61 ++++++++++++++++++++++++ Foundation/testsuite/src/ProcessTest.h | 1 + Foundation/testsuite/src/TestApp.cpp | 12 +++++ 5 files changed, 181 insertions(+), 3 deletions(-) diff --git a/Foundation/src/Process_WIN32.cpp b/Foundation/src/Process_WIN32.cpp index e1989c333..7a5ad0a1e 100644 --- a/Foundation/src/Process_WIN32.cpp +++ b/Foundation/src/Process_WIN32.cpp @@ -107,14 +107,66 @@ void ProcessImpl::timesImpl(long& userTime, long& kernelTime) } +static bool argNeedsEscaping(const std::string& arg) +{ + bool containsQuotableChar = arg.npos != arg.find_first_of(" \t\n\v\""); + // Assume args that start and end with quotes are already quoted and do not require further quoting. + // There is probably code out there written before launch() escaped the arguments that does its own + // escaping of arguments. This ensures we do not interfere with those arguments. + bool isAlreadyQuoted = arg.size() > 1 && '\"' == arg[0] && '\"' == arg[arg.size() - 1]; + return containsQuotableChar && !isAlreadyQuoted; +} + + +// Based on code from https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ +static std::string escapeArg(const std::string& arg) +{ + if (argNeedsEscaping(arg)) + { + std::string quotedArg("\""); + for (auto it = arg.begin(); ; ++it) + { + unsigned backslashCount = 0; + while (it != arg.end() && '\\' == *it) + { + ++it; + ++backslashCount; + } + + if (it == arg.end()) + { + quotedArg.append(2 * backslashCount, '\\'); + break; + } + else if ('"' == *it) + { + quotedArg.append(2 * backslashCount + 1, '\\'); + quotedArg.push_back('"'); + } + else + { + quotedArg.append(backslashCount, '\\'); + quotedArg.push_back(*it); + } + } + quotedArg.push_back('"'); + return quotedArg; + } + else + { + return arg; + } +} + + ProcessHandleImpl* ProcessImpl::launchImpl(const std::string& command, const ArgsImpl& args, const std::string& initialDirectory, Pipe* inPipe, Pipe* outPipe, Pipe* errPipe, const EnvImpl& env) { std::string commandLine = command; for (ArgsImpl::const_iterator it = args.begin(); it != args.end(); ++it) { commandLine.append(" "); - commandLine.append(*it); - } + commandLine.append(escapeArg(*it)); + } STARTUPINFOA startupInfo; GetStartupInfoA(&startupInfo); // take defaults from current process diff --git a/Foundation/src/Process_WIN32U.cpp b/Foundation/src/Process_WIN32U.cpp index 226ffabe2..de75c6783 100644 --- a/Foundation/src/Process_WIN32U.cpp +++ b/Foundation/src/Process_WIN32U.cpp @@ -108,13 +108,65 @@ void ProcessImpl::timesImpl(long& userTime, long& kernelTime) } +static bool argNeedsEscaping(const std::string& arg) +{ + bool containsQuotableChar = arg.npos != arg.find_first_of(" \t\n\v\""); + // Assume args that start and end with quotes are already quoted and do not require further quoting. + // There is probably code out there written before launch() escaped the arguments that does its own + // escaping of arguments. This ensures we do not interfere with those arguments. + bool isAlreadyQuoted = arg.size() > 1 && '\"' == arg[0] && '\"' == arg[arg.size() - 1]; + return containsQuotableChar && !isAlreadyQuoted; +} + + +// Based on code from https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ +static std::string escapeArg(const std::string& arg) +{ + if (argNeedsEscaping(arg)) + { + std::string quotedArg("\""); + for (auto it = arg.begin(); ; ++it) + { + unsigned backslashCount = 0; + while (it != arg.end() && '\\' == *it) + { + ++it; + ++backslashCount; + } + + if (it == arg.end()) + { + quotedArg.append(2 * backslashCount, '\\'); + break; + } + else if ('"' == *it) + { + quotedArg.append(2 * backslashCount + 1, '\\'); + quotedArg.push_back('"'); + } + else + { + quotedArg.append(backslashCount, '\\'); + quotedArg.push_back(*it); + } + } + quotedArg.push_back('"'); + return quotedArg; + } + else + { + return arg; + } +} + + ProcessHandleImpl* ProcessImpl::launchImpl(const std::string& command, const ArgsImpl& args, const std::string& initialDirectory, Pipe* inPipe, Pipe* outPipe, Pipe* errPipe, const EnvImpl& env) { std::string commandLine = command; for (ArgsImpl::const_iterator it = args.begin(); it != args.end(); ++it) { commandLine.append(" "); - commandLine.append(*it); + commandLine.append(escapeArg(*it)); } std::wstring ucommandLine; diff --git a/Foundation/testsuite/src/ProcessTest.cpp b/Foundation/testsuite/src/ProcessTest.cpp index 3ea533fb0..598377661 100644 --- a/Foundation/testsuite/src/ProcessTest.cpp +++ b/Foundation/testsuite/src/ProcessTest.cpp @@ -157,6 +157,66 @@ void ProcessTest::testLaunchEnv() } +void ProcessTest::testLaunchArgs() +{ +#if !defined(_WIN32_WCE) + std::string name("TestApp"); + std::string cmd; + +#if defined(POCO_OS_FAMILY_UNIX) + cmd = "./"; + cmd += name; +#else + cmd = name; +#endif + + std::vector args; + args.push_back("-echo-args"); + args.push_back("simple"); + args.push_back("with space"); + args.push_back("with\ttab"); + args.push_back("with\vverticaltab"); + // can't test newline here because TestApp -echo-args uses newline to separate the echoed args + //args.push_back("with\nnewline"); + args.push_back("with \" quotes"); + args.push_back("ends with \"quotes\""); + args.push_back("\"starts\" with quotes"); + args.push_back("\""); + args.push_back("\\"); + args.push_back("c:\\program files\\ends with backslash\\"); + args.push_back("\"already quoted \\\" \\\\\""); + Pipe outPipe; + ProcessHandle ph = Process::launch(cmd, args, 0, &outPipe, 0); + PipeInputStream istr(outPipe); + std::string receivedArg; + int c = istr.get(); + int argNumber = 1; + while (c != -1) + { + if ('\n' == c) + { + assert(argNumber < args.size()); + std::string expectedArg = args[argNumber]; + if (expectedArg.npos != expectedArg.find("already quoted")) { + expectedArg = "already quoted \" \\"; + } + assert(receivedArg == expectedArg); + ++argNumber; + receivedArg = ""; + } + else if ('\r' != c) + { + receivedArg += (char)c; + } + c = istr.get(); + } + assert(argNumber == args.size()); + int rc = ph.wait(); + assert(rc == args.size()); +#endif // !defined(_WIN32_WCE) +} + + void ProcessTest::testIsRunning() { #if !defined(_WIN32_WCE) @@ -208,6 +268,7 @@ CppUnit::Test* ProcessTest::suite() CppUnit_addTest(pSuite, ProcessTest, testLaunchRedirectIn); CppUnit_addTest(pSuite, ProcessTest, testLaunchRedirectOut); CppUnit_addTest(pSuite, ProcessTest, testLaunchEnv); + CppUnit_addTest(pSuite, ProcessTest, testLaunchArgs); CppUnit_addTest(pSuite, ProcessTest, testIsRunning); return pSuite; diff --git a/Foundation/testsuite/src/ProcessTest.h b/Foundation/testsuite/src/ProcessTest.h index a4fee2ea0..4a2218621 100644 --- a/Foundation/testsuite/src/ProcessTest.h +++ b/Foundation/testsuite/src/ProcessTest.h @@ -30,6 +30,7 @@ public: void testLaunchRedirectIn(); void testLaunchRedirectOut(); void testLaunchEnv(); + void testLaunchArgs(); void testIsRunning(); void setUp(); diff --git a/Foundation/testsuite/src/TestApp.cpp b/Foundation/testsuite/src/TestApp.cpp index c2a081ad3..5d63dd419 100644 --- a/Foundation/testsuite/src/TestApp.cpp +++ b/Foundation/testsuite/src/TestApp.cpp @@ -46,6 +46,18 @@ int main(int argc, char** argv) } else return 1; } + else if (arg == "-raise-int") + { + std::signal(SIGINT, SIG_DFL); + std::raise(SIGINT); + } + else if (arg == "-echo-args") + { + for (int i = 2; i < argc; ++i) + { + std::cout << argv[i] << std::endl; + } + } } return argc - 1; }