Skip to content

Commit

Permalink
Update threshold for long path shortening to be MAX_PATH - 4
Browse files Browse the repository at this point in the history
This PR applies the suggested fix for bazelbuild#12310. Although I could not reproduce it.

Fixes: bazelbuild#12310

Closes bazelbuild#12941.

PiperOrigin-RevId: 362327025
  • Loading branch information
mai93 authored and katre committed Jul 13, 2021
1 parent d740655 commit a5807e6
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 27 deletions.
21 changes: 13 additions & 8 deletions src/main/native/windows/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"";
Expand All @@ -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");
}
Expand All @@ -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);
Expand Down
40 changes: 21 additions & 19 deletions src/test/native/windows/util_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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;
}

Expand Down Expand Up @@ -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";
Expand All @@ -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) {
Expand Down Expand Up @@ -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");
Expand Down

0 comments on commit a5807e6

Please sign in to comment.