From 451b296c3aceb127ebb4a313b6e9608854fa68fa Mon Sep 17 00:00:00 2001 From: mai93 Date: Thu, 11 Mar 2021 10:42:51 -0800 Subject: [PATCH] Update threshold for long path shortening to be MAX_PATH - 4 This PR applies the suggested fix for #12310. Although I could not reproduce it. Fixes: #12310 Closes #12941. PiperOrigin-RevId: 362327025 --- src/main/native/windows/util.cc | 21 +++++++++------ src/test/native/windows/util_test.cc | 40 +++++++++++++++------------- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/main/native/windows/util.cc b/src/main/native/windows/util.cc index 0e46deef29880d..08d36caa9723cd 100644 --- 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 index 857518c3aa1776..3cb5e3ccba8320 100644 --- 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,9 +283,9 @@ 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; @@ -291,14 +293,14 @@ TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessConversions) { 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");