Merge pull request #973 from sgraham/canonicalize-fix

Fix crash in attempting to canonicalize paths longer than _MAX_PATH

8 files changed, 149 insertions, 63 deletions

diff --git a/src/build.cc b/src/build.cc
index cdb8e0a..e4820d0 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -849,7 +849,10 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
 #ifdef _WIN32
   if (deps_type == "msvc") {
     CLParser parser;
-    result->output = parser.Parse(result->output, deps_prefix);
+    string output;
+    if (!parser.Parse(result->output, deps_prefix, &output, err))
+      return false;
+    result->output = output;
     for (set<string>::iterator i = parser.includes_.begin();
          i != parser.includes_.end(); ++i) {
       // ~0 is assuming that with MSVC-parsed headers, it's ok to always make
diff --git a/src/includes_normalize-win32.cc b/src/includes_normalize-win32.cc
index 1e88a0a..ca35012 100644
--- a/src/includes_normalize-win32.cc
+++ b/src/includes_normalize-win32.cc
@@ -94,15 +94,18 @@ string IncludesNormalize::Relativize(StringPiece path, const string& start) {
   return Join(rel_list, '/');
 }
 
-string IncludesNormalize::Normalize(const string& input,
-                                    const char* relative_to) {
-  char copy[_MAX_PATH];
+bool IncludesNormalize::Normalize(const string& input, const char* relative_to,
+                                  string* result, string* err) {
+  char copy[_MAX_PATH + 1];
   size_t len = input.size();
+  if (len > _MAX_PATH) {
+    *err = "path too long";
+    return false;
+  }
   strncpy(copy, input.c_str(), input.size() + 1);
-  string err;
   unsigned int slash_bits;
-  if (!CanonicalizePath(copy, &len, &slash_bits, &err))
-    Warning("couldn't canonicalize '%s': %s\n", input.c_str(), err.c_str());
+  if (!CanonicalizePath(copy, &len, &slash_bits, err))
+    return false;
   StringPiece partially_fixed(copy, len);
 
   string curdir;
@@ -110,7 +113,10 @@ string IncludesNormalize::Normalize(const string& input,
     curdir = AbsPath(".");
     relative_to = curdir.c_str();
   }
-  if (!SameDrive(partially_fixed, relative_to))
-    return partially_fixed.AsString();
-  return Relativize(partially_fixed, relative_to);
+  if (!SameDrive(partially_fixed, relative_to)) {
+    *result = partially_fixed.AsString();
+    return true;
+  }
+  *result = Relativize(partially_fixed, relative_to);
+  return true;
 }
diff --git a/src/includes_normalize.h b/src/includes_normalize.h
index 634fef3..98e912f 100644
--- a/src/includes_normalize.h
+++ b/src/includes_normalize.h
@@ -30,5 +30,6 @@ struct IncludesNormalize {
 
   /// Normalize by fixing slashes style, fixing redundant .. and . and makes the
   /// path relative to |relative_to|.
-  static string Normalize(const string& input, const char* relative_to);
+  static bool Normalize(const string& input, const char* relative_to,
+                        string* result, string* err);
 };
diff --git a/src/includes_normalize_test.cc b/src/includes_normalize_test.cc
index c4c2476..aba25d0 100644
--- a/src/includes_normalize_test.cc
+++ b/src/includes_normalize_test.cc
@@ -14,18 +14,13 @@
 
 #include "includes_normalize.h"
 
+#include <algorithm>
+
 #include <direct.h>
 
 #include "test.h"
 #include "util.h"
 
-TEST(IncludesNormalize, Simple) {
-  EXPECT_EQ("b", IncludesNormalize::Normalize("a\\..\\b", NULL));
-  EXPECT_EQ("b", IncludesNormalize::Normalize("a\\../b", NULL));
-  EXPECT_EQ("a/b", IncludesNormalize::Normalize("a\\.\\b", NULL));
-  EXPECT_EQ("a/b", IncludesNormalize::Normalize("a\\./b", NULL));
-}
-
 namespace {
 
 string GetCurDir() {
@@ -35,28 +30,50 @@ string GetCurDir() {
   return parts[parts.size() - 1];
 }
 
+string NormalizeAndCheckNoError(const std::string& input) {
+  string result, err;
+  EXPECT_TRUE(IncludesNormalize::Normalize(input.c_str(), NULL, &result, &err));
+  EXPECT_EQ("", err);
+  return result;
+}
+
+string NormalizeRelativeAndCheckNoError(const std::string& input,
+                                        const std::string& relative_to) {
+  string result, err;
+  EXPECT_TRUE(IncludesNormalize::Normalize(input.c_str(), relative_to.c_str(),
+                                           &result, &err));
+  EXPECT_EQ("", err);
+  return result;
+}
+
 }  // namespace
 
+TEST(IncludesNormalize, Simple) {
+  EXPECT_EQ("b", NormalizeAndCheckNoError("a\\..\\b"));
+  EXPECT_EQ("b", NormalizeAndCheckNoError("a\\../b"));
+  EXPECT_EQ("a/b", NormalizeAndCheckNoError("a\\.\\b"));
+  EXPECT_EQ("a/b", NormalizeAndCheckNoError("a\\./b"));
+}
+
 TEST(IncludesNormalize, WithRelative) {
   string currentdir = GetCurDir();
-  EXPECT_EQ("c", IncludesNormalize::Normalize("a/b/c", "a/b"));
-  EXPECT_EQ("a", IncludesNormalize::Normalize(IncludesNormalize::AbsPath("a"),
-                                              NULL));
+  EXPECT_EQ("c", NormalizeRelativeAndCheckNoError("a/b/c", "a/b"));
+  EXPECT_EQ("a", NormalizeAndCheckNoError(IncludesNormalize::AbsPath("a")));
   EXPECT_EQ(string("../") + currentdir + string("/a"),
-            IncludesNormalize::Normalize("a", "../b"));
+            NormalizeRelativeAndCheckNoError("a", "../b"));
   EXPECT_EQ(string("../") + currentdir + string("/a/b"),
-            IncludesNormalize::Normalize("a/b", "../c"));
-  EXPECT_EQ("../../a", IncludesNormalize::Normalize("a", "b/c"));
-  EXPECT_EQ(".", IncludesNormalize::Normalize("a", "a"));
+            NormalizeRelativeAndCheckNoError("a/b", "../c"));
+  EXPECT_EQ("../../a", NormalizeRelativeAndCheckNoError("a", "b/c"));
+  EXPECT_EQ(".", NormalizeRelativeAndCheckNoError("a", "a"));
 }
 
 TEST(IncludesNormalize, Case) {
-  EXPECT_EQ("b", IncludesNormalize::Normalize("Abc\\..\\b", NULL));
-  EXPECT_EQ("BdEf", IncludesNormalize::Normalize("Abc\\..\\BdEf", NULL));
-  EXPECT_EQ("A/b", IncludesNormalize::Normalize("A\\.\\b", NULL));
-  EXPECT_EQ("a/b", IncludesNormalize::Normalize("a\\./b", NULL));
-  EXPECT_EQ("A/B", IncludesNormalize::Normalize("A\\.\\B", NULL));
-  EXPECT_EQ("A/B", IncludesNormalize::Normalize("A\\./B", NULL));
+  EXPECT_EQ("b", NormalizeAndCheckNoError("Abc\\..\\b"));
+  EXPECT_EQ("BdEf", NormalizeAndCheckNoError("Abc\\..\\BdEf"));
+  EXPECT_EQ("A/b", NormalizeAndCheckNoError("A\\.\\b"));
+  EXPECT_EQ("a/b", NormalizeAndCheckNoError("a\\./b"));
+  EXPECT_EQ("A/B", NormalizeAndCheckNoError("A\\.\\B"));
+  EXPECT_EQ("A/B", NormalizeAndCheckNoError("A\\./B"));
 }
 
 TEST(IncludesNormalize, Join) {
@@ -89,16 +106,63 @@ TEST(IncludesNormalize, ToLower) {
 
 TEST(IncludesNormalize, DifferentDrive) {
   EXPECT_EQ("stuff.h",
-      IncludesNormalize::Normalize("p:\\vs08\\stuff.h", "p:\\vs08"));
+            NormalizeRelativeAndCheckNoError("p:\\vs08\\stuff.h", "p:\\vs08"));
   EXPECT_EQ("stuff.h",
-      IncludesNormalize::Normalize("P:\\Vs08\\stuff.h", "p:\\vs08"));
+            NormalizeRelativeAndCheckNoError("P:\\Vs08\\stuff.h", "p:\\vs08"));
   EXPECT_EQ("p:/vs08/stuff.h",
-      IncludesNormalize::Normalize("p:\\vs08\\stuff.h", "c:\\vs08"));
-  EXPECT_EQ("P:/vs08/stufF.h",
-      IncludesNormalize::Normalize("P:\\vs08\\stufF.h", "D:\\stuff/things"));
-  EXPECT_EQ("P:/vs08/stuff.h",
-      IncludesNormalize::Normalize("P:/vs08\\stuff.h", "D:\\stuff/things"));
+            NormalizeRelativeAndCheckNoError("p:\\vs08\\stuff.h", "c:\\vs08"));
+  EXPECT_EQ("P:/vs08/stufF.h", NormalizeRelativeAndCheckNoError(
+                                   "P:\\vs08\\stufF.h", "D:\\stuff/things"));
+  EXPECT_EQ("P:/vs08/stuff.h", NormalizeRelativeAndCheckNoError(
+                                   "P:/vs08\\stuff.h", "D:\\stuff/things"));
   EXPECT_EQ("P:/wee/stuff.h",
-            IncludesNormalize::Normalize("P:/vs08\\../wee\\stuff.h",
-                                         "D:\\stuff/things"));
+            NormalizeRelativeAndCheckNoError("P:/vs08\\../wee\\stuff.h",
+                                             "D:\\stuff/things"));
+}
+
+TEST(IncludesNormalize, LongInvalidPath) {
+  const char kLongInputString[] =
+      "C:\\Program Files (x86)\\Microsoft Visual Studio "
+      "12.0\\VC\\INCLUDEwarning #31001: The dll for reading and writing the "
+      "pdb (for example, mspdb110.dll) could not be found on your path. This "
+      "is usually a configuration error. Compilation will continue using /Z7 "
+      "instead of /Zi, but expect a similar error when you link your program.";
+  // Too long, won't be canonicalized. Ensure doesn't crash.
+  string result, err;
+  EXPECT_FALSE(
+      IncludesNormalize::Normalize(kLongInputString, NULL, &result, &err));
+  EXPECT_EQ("path too long", err);
+
+  const char kExactlyMaxPath[] =
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "012345678\\"
+      "0123456789";
+  std::string forward_slashes(kExactlyMaxPath);
+  std::replace(forward_slashes.begin(), forward_slashes.end(), '\\', '/');
+  // Make sure a path that's exactly _MAX_PATH long is canonicalized.
+  EXPECT_EQ(forward_slashes,
+            NormalizeAndCheckNoError(kExactlyMaxPath));
 }
diff --git a/src/msvc_helper-win32.cc b/src/msvc_helper-win32.cc
index e465279..d516240 100644
--- a/src/msvc_helper-win32.cc
+++ b/src/msvc_helper-win32.cc
@@ -15,6 +15,7 @@
 #include "msvc_helper.h"
 
 #include <algorithm>
+#include <assert.h>
 #include <stdio.h>
 #include <string.h>
 #include <windows.h>
@@ -82,10 +83,10 @@ bool CLParser::FilterInputFilename(string line) {
       EndsWith(line, ".cpp");
 }
 
-string CLParser::Parse(const string& output, const string& deps_prefix) {
-  string filtered_output;
-
+bool CLParser::Parse(const string& output, const string& deps_prefix,
+                     string* filtered_output, string* err) {
   // Loop over all lines in the output to process them.
+  assert(&output != filtered_output);
   size_t start = 0;
   while (start < output.size()) {
     size_t end = output.find_first_of("\r\n", start);
@@ -95,16 +96,18 @@ string CLParser::Parse(const string& output, const string& deps_prefix) {
 
     string include = FilterShowIncludes(line, deps_prefix);
     if (!include.empty()) {
-      include = IncludesNormalize::Normalize(include, NULL);
-      if (!IsSystemInclude(include))
-        includes_.insert(include);
+      string normalized;
+      if (!IncludesNormalize::Normalize(include, NULL, &normalized, err))
+        return false;
+      if (!IsSystemInclude(normalized))
+        includes_.insert(normalized);
     } else if (FilterInputFilename(line)) {
       // Drop it.
       // TODO: if we support compiling multiple output files in a single
       // cl.exe invocation, we should stash the filename.
     } else {
-      filtered_output.append(line);
-      filtered_output.append("\n");
+      filtered_output->append(line);
+      filtered_output->append("\n");
     }
 
     if (end < output.size() && output[end] == '\r')
@@ -114,7 +117,7 @@ string CLParser::Parse(const string& output, const string& deps_prefix) {
     start = end;
   }
 
-  return filtered_output;
+  return true;
 }
 
 int CLWrapper::Run(const string& command, string* output) {
diff --git a/src/msvc_helper.h b/src/msvc_helper.h
index 5d7dcb0..30f87f3 100644
--- a/src/msvc_helper.h
+++ b/src/msvc_helper.h
@@ -40,9 +40,11 @@ struct CLParser {
   /// Exposed for testing.
   static bool FilterInputFilename(string line);
 
-  /// Parse the full output of cl, returning the output (if any) that
-  /// should printed.
-  string Parse(const string& output, const string& deps_prefix);
+  /// Parse the full output of cl, filling filtered_output with the text that
+  /// should be printed (if any). Returns true on success, or false with err
+  /// filled. output must not be the same object as filtered_object.
+  bool Parse(const string& output, const string& deps_prefix,
+             string* filtered_output, string* err);
 
   set<string> includes_;
 };
diff --git a/src/msvc_helper_main-win32.cc b/src/msvc_helper_main-win32.cc
index 58bc797..680aaad 100644
--- a/src/msvc_helper_main-win32.cc
+++ b/src/msvc_helper_main-win32.cc
@@ -127,7 +127,9 @@ int MSVCHelperMain(int argc, char** argv) {
 
   if (output_filename) {
     CLParser parser;
-    output = parser.Parse(output, deps_prefix);
+    string err;
+    if (!parser.Parse(output, deps_prefix, &output, &err))
+      Fatal("%s\n", err.c_str());
     WriteDepFileOrDie(output_filename, parser);
   }
 
diff --git a/src/msvc_helper_test.cc b/src/msvc_helper_test.cc
index 29db650..49d27c1 100644
--- a/src/msvc_helper_test.cc
+++ b/src/msvc_helper_test.cc
@@ -46,11 +46,12 @@ TEST(CLParserTest, FilterInputFilename) {
 
 TEST(CLParserTest, ParseSimple) {
   CLParser parser;
-  string output = parser.Parse(
+  string output, err;
+  ASSERT_TRUE(parser.Parse(
       "foo\r\n"
       "Note: inc file prefix:  foo.h\r\n"
       "bar\r\n",
-      "Note: inc file prefix:");
+      "Note: inc file prefix:", &output, &err));
 
   ASSERT_EQ("foo\nbar\n", output);
   ASSERT_EQ(1u, parser.includes_.size());
@@ -59,20 +60,22 @@ TEST(CLParserTest, ParseSimple) {
 
 TEST(CLParserTest, ParseFilenameFilter) {
   CLParser parser;
-  string output = parser.Parse(
+  string output, err;
+  ASSERT_TRUE(parser.Parse(
       "foo.cc\r\n"
       "cl: warning\r\n",
-      "");
+      "", &output, &err));
   ASSERT_EQ("cl: warning\n", output);
 }
 
 TEST(CLParserTest, ParseSystemInclude) {
   CLParser parser;
-  string output = parser.Parse(
+  string output, err;
+  ASSERT_TRUE(parser.Parse(
       "Note: including file: c:\\Program Files\\foo.h\r\n"
       "Note: including file: d:\\Microsoft Visual Studio\\bar.h\r\n"
       "Note: including file: path.h\r\n",
-      "");
+      "", &output, &err));
   // We should have dropped the first two includes because they look like
   // system headers.
   ASSERT_EQ("", output);
@@ -82,11 +85,12 @@ TEST(CLParserTest, ParseSystemInclude) {
 
 TEST(CLParserTest, DuplicatedHeader) {
   CLParser parser;
-  string output = parser.Parse(
+  string output, err;
+  ASSERT_TRUE(parser.Parse(
       "Note: including file: foo.h\r\n"
       "Note: including file: bar.h\r\n"
       "Note: including file: foo.h\r\n",
-      "");
+      "", &output, &err));
   // We should have dropped one copy of foo.h.
   ASSERT_EQ("", output);
   ASSERT_EQ(2u, parser.includes_.size());
@@ -94,11 +98,12 @@ TEST(CLParserTest, DuplicatedHeader) {
 
 TEST(CLParserTest, DuplicatedHeaderPathConverted) {
   CLParser parser;
-  string output = parser.Parse(
+  string output, err;
+  ASSERT_TRUE(parser.Parse(
       "Note: including file: sub/foo.h\r\n"
       "Note: including file: bar.h\r\n"
       "Note: including file: sub\\foo.h\r\n",
-      "");
+      "", &output, &err));
   // We should have dropped one copy of foo.h.
   ASSERT_EQ("", output);
   ASSERT_EQ(2u, parser.includes_.size());