Update threshold for long path shortening to be MAX_PATH - 4

mai93 · katre · commit 451b296c3ace · 2021-07-13T10:40:35.000-04:00
This PR applies the suggested fix for bazelbuild#12310. Although I could not reproduce it. Fixes: bazelbuild#12310 Closes bazelbuild#12941. PiperOrigin-RevId: 362327025
diff --git a/src/main/native/windows/util.cc b/src/main/native/windows/util.cc
@@ -195,6 +195,10 @@ static bool Contains(const wstring& s, const WCHAR* substr) {
 }
 
 wstring AsShortPath(wstring path, wstring* result) {
+  // Using `MAX_PATH` - 4 (256) instead of `MAX_PATH` to fix
+  // https://github.com/bazelbuild/bazel/issues/12310
+  static const size_t kMaxPath = MAX_PATH - 4;
+
   if (path.empty()) {
     result->clear();
     return L"";
@@ -212,7 +216,7 @@ wstring AsShortPath(wstring path, wstring* result) {
     return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"AsShortPath", path,
                             L"path is not normalized");
   }
-  if (path.size() >= MAX_PATH && !HasSeparator(path)) {
+  if (path.size() >= kMaxPath && !HasSeparator(path)) {
     return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"AsShortPath", path,
                             L"path is just a file name but too long");
   }
@@ -221,28 +225,29 @@ wstring AsShortPath(wstring path, wstring* result) {
                             L"path is not absolute");
   }
   // At this point we know the path is either just a file name (shorter than
-  // MAX_PATH), or an absolute, normalized, Windows-style path (of any length).
+  // `kMaxPath`), or an absolute, normalized, Windows-style path (of any
+  // length).
 
   std::replace(path.begin(), path.end(), '/', '\\');
   // Fast-track: the path is already short.
-  if (path.size() < MAX_PATH) {
+  if (path.size() < kMaxPath) {
     *result = path;
     return L"";
   }
-  // At this point we know that the path is at least MAX_PATH long and that it's
-  // absolute, normalized, and Windows-style.
+  // At this point we know that the path is at least `kMaxPath` long and that
+  // it's absolute, normalized, and Windows-style.
 
   wstring wlong = wstring(L"\\\\?\\") + path;
 
   // Experience shows that:
   // - GetShortPathNameW's result has a "\\?\" prefix if and only if the input
   //   did too (though this behavior is not documented on MSDN)
-  // - CreateProcess{A,W} only accept an executable of MAX_PATH - 1 length
+  // - CreateProcess{A,W} only accept an executable of `MAX_PATH` - 1 length
   // Therefore for our purposes the acceptable shortened length is
-  // MAX_PATH + 4 (null-terminated). That is, MAX_PATH - 1 for the shortened
+  // `kMaxPath` + 4 (null-terminated). That is, `kMaxPath` - 1 for the shortened
   // path, plus a potential "\\?\" prefix that's only there if `wlong` also had
   // it and which we'll omit from `result`, plus a null terminator.
-  static const size_t kMaxShortPath = MAX_PATH + 4;
+  static const size_t kMaxShortPath = kMaxPath + 4;
 
   WCHAR wshort[kMaxShortPath];
   DWORD wshort_size = ::GetShortPathNameW(wlong.c_str(), NULL, 0);
diff --git a/src/test/native/windows/util_test.cc b/src/test/native/windows/util_test.cc
@@ -34,11 +34,13 @@
 namespace bazel {
 namespace windows {
 
-using std::wstring;
 using std::unique_ptr;
 using std::wstring;
 
 static const wstring kUncPrefix = wstring(L"\\\\?\\");
+// Using `MAX_PATH` - 4 instead of `MAX_PATH` to fix
+// https://github.com/bazelbuild/bazel/issues/12310
+constexpr size_t kMaxPath = MAX_PATH - 4;
 
 // Retrieves TEST_TMPDIR as a shortened path. Result won't have a "\\?\" prefix.
 static void GetShortTempDir(wstring* result) {
@@ -66,11 +68,11 @@ static void GetShortTempDir(wstring* result) {
   ::GetShortPathNameW(tmpdir.c_str(), buf.get(), size);
 
   // Set the result, omit the "\\?\" prefix.
-  // Ensure that the result is shorter than MAX_PATH and also has room for a
+  // Ensure that the result is shorter than `kMaxPath` and also has room for a
   // backslash (1 wchar) and a single-letter executable name with .bat
   // extension (5 wchars).
   *result = wstring(buf.get() + 4);
-  ASSERT_LT(result->size(), MAX_PATH - 6);
+  ASSERT_LT(result->size(), kMaxPath - 6);
 }
 
 // If `success` is true, returns an empty string, otherwise an error message.
@@ -165,14 +167,14 @@ static wstring DeleteDir(wstring path) {
 // `result_path` will be also a short path under `basedir`.
 //
 // Every directory in `result_path` will be created. The entire length of this
-// path will be exactly MAX_PATH - 7 (not including null-terminator).
+// path will be exactly `kMaxPath` - 7 (not including null-terminator).
 // Just by appending a file name segment between 6 and 8 characters long (i.e.
 // "\a.bat", "\ab.bat", or "\abc.bat") the caller can obtain a path that is
-// MAX_PATH - 1 long, or MAX_PATH long, or MAX_PATH + 1 long, respectively,
-// and cannot be shortened further.
+// `kMaxPath` - 1 long, or `kMaxPath` long, or `kMaxPath` + 1 long,
+// respectively, and cannot be shortened further.
 static void CreateShortDirsUnder(wstring basedir, wstring* result_path) {
-  ASSERT_LT(basedir.size(), MAX_PATH);
-  size_t remaining_len = MAX_PATH - 1 - basedir.size();
+  ASSERT_LT(basedir.size(), kMaxPath);
+  size_t remaining_len = kMaxPath - 1 - basedir.size();
   ASSERT_GE(remaining_len, 6);  // has room for suffix "\a.bat"
 
   // If `remaining_len` is odd, make it even.
@@ -188,7 +190,7 @@ static void CreateShortDirsUnder(wstring basedir, wstring* result_path) {
     basedir += wstring(L"\\a");
     CREATE_DIR(basedir);
   }
-  ASSERT_EQ(basedir.size(), MAX_PATH - 1 - 6);
+  ASSERT_EQ(basedir.size(), kMaxPath - 1 - 6);
   *result_path = basedir;
 }
 
@@ -260,7 +262,7 @@ TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessBadInputs) {
   ASSERT_SHORTENING_FAILS(L"\\bar.exe", L"path is absolute");
 
   wstring dummy = L"hello";
-  while (dummy.size() < MAX_PATH) {
+  while (dummy.size() < kMaxPath) {
     dummy += dummy;
   }
   dummy += L".exe";
@@ -281,24 +283,24 @@ TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessConversions) {
   CreateShortDirsUnder(tmpdir, &short_root);
 
   // Assert that we have enough room to append a file name that is just short
-  // enough to fit into MAX_PATH - 1, or one that's just long enough to make
-  // the whole path MAX_PATH long or longer.
-  ASSERT_EQ(short_root.size(), MAX_PATH - 1 - 6);
+  // enough to fit into `kMaxPath` - 1, or one that's just long enough to make
+  // the whole path `kMaxPath` long or longer.
+  ASSERT_EQ(short_root.size(), kMaxPath - 1 - 6);
 
   wstring actual;
   wstring error;
   for (size_t i = 0; i < 3; ++i) {
     wstring wfilename = short_root;
 
     APPEND_FILE_SEGMENT(6 + i, &wfilename);
-    ASSERT_EQ(wfilename.size(), MAX_PATH - 1 + i);
+    ASSERT_EQ(wfilename.size(), kMaxPath - 1 + i);
 
-    // When i=0 then `wfilename` is MAX_PATH - 1 long, so
+    // When i=0 then `wfilename` is `kMaxPath` - 1 long, so
     // `AsExecutablePathForCreateProcess` will not attempt to shorten it, and
     // so it also won't notice that the file doesn't exist. If however we pass
     // a non-existent path to CreateProcessA, then it'll fail, so we'll find out
     // about this error in production code.
-    // When i>0 then `wfilename` is at least MAX_PATH long, so
+    // When i>0 then `wfilename` is at least `kMaxPath` long, so
     // `AsExecutablePathForCreateProcess` will attempt to shorten it, but
     // because the file doesn't yet exist, the shortening attempt will fail.
     if (i > 0) {
@@ -326,14 +328,14 @@ TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessConversions) {
   // Finally construct a path that can and will be shortened. Just walk up a few
   // levels in `short_root` and create a long file name that can be shortened.
   wstring wshortenable_root = short_root;
-  while (wshortenable_root.size() > MAX_PATH - 1 - 13) {
+  while (wshortenable_root.size() > kMaxPath - 1 - 13) {
     wshortenable_root =
         wshortenable_root.substr(0, wshortenable_root.find_last_of(L'\\'));
   }
   wstring wshortenable = wshortenable_root + wstring(L"\\") +
-                         wstring(MAX_PATH - wshortenable_root.size(), L'a') +
+                         wstring(kMaxPath - wshortenable_root.size(), L'a') +
                          wstring(L".bat");
-  ASSERT_GT(wshortenable.size(), MAX_PATH);
+  ASSERT_GT(wshortenable.size(), kMaxPath);
 
   // Attempt to shorten. It will fail because the file doesn't exist yet.
   ASSERT_SHORTENING_FAILS(wshortenable, L"GetShortPathNameW");
