More robust escaping of $in, $out paths

In summary: don’t escape if the path doesn’t contain problematic characters, otherwise:
- Shell: enclose string in single quotes, escape embedded single quotes with the magic quote-backslash-quote sequence
- Win32: Escape double quotes by doubling the number of consecutive backslashes that precede them (if any) and adding one more. Finally, double the number of trailing backslashes, and enclose the whole thing in double quotes.

6 files changed, 152 insertions, 12 deletions

diff --git a/src/graph.cc b/src/graph.cc
index 9801a7b..65f9244 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -250,16 +250,13 @@ string EdgeEnv::MakePathList(vector<Node*>::iterator begin,
                              char sep) {
   string result;
   for (vector<Node*>::iterator i = begin; i != end; ++i) {
-    if (!result.empty())
-      result.push_back(sep);
+    if (!result.empty()) result.push_back(sep);
     const string& path = (*i)->path();
-    if (path.find(" ") != string::npos) {
-      result.append("\"");
-      result.append(path);
-      result.append("\"");
-    } else {
-      result.append(path);
-    }
+#if _WIN32
+    GetWin32EscapedString(path, &result);
+#else
+    GetShellEscapedString(path, &result);
+#endif
   }
   return result;
 }
diff --git a/src/graph_test.cc b/src/graph_test.cc
index 8521216..14dc678 100644
--- a/src/graph_test.cc
+++ b/src/graph_test.cc
@@ -139,13 +139,18 @@ TEST_F(GraphTest, RootNodes) {
   }
 }
 
-TEST_F(GraphTest, VarInOutQuoteSpaces) {
+TEST_F(GraphTest, VarInOutPathEscaping) {
   ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
-"build a$ b: cat nospace with$ space nospace2\n"));
+"build a$ b: cat no'space with$ space$$ no\"space2\n"));
 
   Edge* edge = GetNode("a b")->in_edge();
-  EXPECT_EQ("cat nospace \"with space\" nospace2 > \"a b\"",
+#if _WIN32
+  EXPECT_EQ("cat no'space \"with space$\" \"no\\\"space2\" > \"a b\"",
       edge->EvaluateCommand());
+#else
+  EXPECT_EQ("cat 'no'\\''space' 'with space$' 'no\"space2' > 'a b'",
+      edge->EvaluateCommand());
+#endif
 }
 
 // Regression test for https://github.com/martine/ninja/issues/380
diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc
index 0b4c042..152b965 100644
--- a/src/manifest_parser_test.cc
+++ b/src/manifest_parser_test.cc
@@ -236,7 +236,11 @@ TEST_F(ParserTest, Dollars) {
 "build $x: foo y\n"
 ));
   EXPECT_EQ("$dollar", state.bindings_.LookupVariable("x"));
+#ifdef _WIN32
   EXPECT_EQ("$dollarbar$baz$blah", state.edges_[0]->EvaluateCommand());
+#else
+  EXPECT_EQ("'$dollar'bar$baz$blah", state.edges_[0]->EvaluateCommand());
+#endif
 }
 
 TEST_F(ParserTest, EscapeSpaces) {
diff --git a/src/util.cc b/src/util.cc
index 6ba3c6c..0e4dc59 100644
--- a/src/util.cc
+++ b/src/util.cc
@@ -20,6 +20,7 @@
 #include <share.h>
 #endif
 
+#include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <stdarg.h>
@@ -175,6 +176,109 @@ bool CanonicalizePath(char* path, size_t* len, string* err) {
   return true;
 }
 
+static inline bool IsKnownShellSafeCharacter(char ch) {
+  if ('A' <= ch && ch <= 'Z') return true;
+  if ('a' <= ch && ch <= 'z') return true;
+  if ('0' <= ch && ch <= '9') return true;
+
+  switch (ch) {
+    case '_':
+    case '-':
+    case '.':
+    case '/':
+      return true;
+    default:
+      return false;
+  }
+}
+
+static inline bool IsKnownWin32SafeCharacter(char ch) {
+  switch (ch) {
+    case '\\':
+    case ' ':
+    case '"':
+      return false;
+    default:
+      return true;
+  }
+}
+
+static inline bool StringNeedsShellEscaping(const string& input) {
+  for (size_t i = 0; i < input.size(); ++i) {
+    if (!IsKnownShellSafeCharacter(input[i])) return true;
+  }
+  return false;
+}
+
+static inline bool StringNeedsWin32Escaping(const string& input) {
+  for (size_t i = 0; i < input.size(); ++i) {
+    if (!IsKnownWin32SafeCharacter(input[i])) return true;
+  }
+  return false;
+}
+
+void GetShellEscapedString(const string& input, string* result) {
+  assert(result);
+
+  if (!StringNeedsShellEscaping(input)) {
+    result->append(input);
+    return;
+  }
+
+  const char kQuote = '\'';
+  const char kEscapeSequence[] = "'\\'";
+
+  result->push_back(kQuote);
+
+  string::const_iterator span_begin = input.begin();
+  for (string::const_iterator it = input.begin(), end = input.end(); it != end;
+       ++it) {
+    if (*it == kQuote) {
+      result->append(span_begin, it);
+      result->append(kEscapeSequence);
+      span_begin = it;
+    }
+  }
+  result->append(span_begin, input.end());
+  result->push_back(kQuote);
+}
+
+
+void GetWin32EscapedString(const string& input, string* result) {
+  assert(result);
+  if (!StringNeedsWin32Escaping(input)) {
+    result->append(input);
+    return;
+  }
+
+  const char kQuote = '"';
+  const char kBackslash = '\\';
+
+  result->push_back(kQuote);
+  size_t consecutive_backslash_count = 0;
+  string::const_iterator span_begin = input.begin();
+  for (string::const_iterator it = input.begin(), end = input.end(); it != end;
+       ++it) {
+    switch (*it) {
+      case kBackslash:
+        ++consecutive_backslash_count;
+        break;
+      case kQuote:
+        result->append(span_begin, it);
+        result->append(consecutive_backslash_count + 1, kBackslash);
+        span_begin = it;
+        consecutive_backslash_count = 0;
+        break;
+      default:
+        consecutive_backslash_count = 0;
+        break;
+    }
+  }
+  result->append(span_begin, input.end());
+  result->append(consecutive_backslash_count, kBackslash);
+  result->push_back(kQuote);
+}
+
 int ReadFile(const string& path, string* contents, string* err) {
   FILE* f = fopen(path.c_str(), "r");
   if (!f) {
diff --git a/src/util.h b/src/util.h
index 6788410..7101770 100644
--- a/src/util.h
+++ b/src/util.h
@@ -45,6 +45,13 @@ bool CanonicalizePath(string* path, string* err);
 
 bool CanonicalizePath(char* path, size_t* len, string* err);
 
+/// Appends |input| to |*result|, escaping according to the whims of either
+/// Bash, or Win32's CommandLineToArgvW().
+/// Appends the string directly to |result| without modification if we can
+/// determine that it contains no problematic characters.
+void GetShellEscapedString(const string& input, string* result);
+void GetWin32EscapedString(const string& input, string* result);
+
 /// Read a file to a string (in text mode: with CRLF conversion
 /// on Windows).
 /// Returns -errno and fills in \a err on error.
diff --git a/src/util_test.cc b/src/util_test.cc
index 1e29053..f6728fb 100644
--- a/src/util_test.cc
+++ b/src/util_test.cc
@@ -136,6 +136,29 @@ TEST(CanonicalizePath, NotNullTerminated) {
   EXPECT_EQ("file ./file bar/.", string(path));
 }
 
+TEST(PathEscaping, TortureTest) {
+  string result;
+  
+  GetWin32EscapedString("foo bar\\\"'$@d!st!c'\\path'\\", &result);
+  EXPECT_EQ("\"foo bar\\\\\\\"'$@d!st!c'\\path'\\\\\"", result);
+  result.clear();  
+
+  GetShellEscapedString("foo bar\"/'$@d!st!c'/path'", &result);
+  EXPECT_EQ("'foo bar\"/'\\''$@d!st!c'\\''/path'\\'''", result);
+}
+
+TEST(PathEscaping, SensiblePathsAreNotNeedlesslyEscaped) {
+  const char* path = "some/sensible/path/without/crazy/characters.cc";
+  string result;
+
+  GetWin32EscapedString(path, &result);
+  EXPECT_EQ(path, result);
+  result.clear();
+
+  GetShellEscapedString(path, &result);
+  EXPECT_EQ(path, result);
+}
+
 TEST(StripAnsiEscapeCodes, EscapeAtEnd) {
   string stripped = StripAnsiEscapeCodes("foo\33");
   EXPECT_EQ("foo", stripped);