diff --git a/src/main/native/windows/process.cc b/src/main/native/windows/process.cc index 4646a657ce98e5..c8f0a698bb02ef 100644 --- a/src/main/native/windows/process.cc +++ b/src/main/native/windows/process.cc @@ -77,7 +77,9 @@ bool WaitableProcess::Create(const std::wstring& argv0, } std::wstring argv0short; - error_msg = AsExecutablePathForCreateProcess(argv0, &argv0short); + std::wstring extended_path; + error_msg = + AsExecutablePathForCreateProcess(argv0, &argv0short, &extended_path); if (!error_msg.empty()) { *error = MakeErrorMessage(WSTR(__FILE__), __LINE__, L"WaitableProcess::Create", argv0, error_msg); @@ -159,7 +161,8 @@ bool WaitableProcess::Create(const std::wstring& argv0, STARTUPINFOEXW info; attr_list->InitStartupInfoExW(&info); if (!CreateProcessW( - /* lpApplicationName */ nullptr, + /* lpApplicationName */ extended_path.empty() ? nullptr + : extended_path.c_str(), /* lpCommandLine */ mutable_commandline.get(), /* lpProcessAttributes */ nullptr, /* lpThreadAttributes */ nullptr, diff --git a/src/main/native/windows/util.cc b/src/main/native/windows/util.cc index 615d09951c6993..872fb10e0e0acf 100644 --- a/src/main/native/windows/util.cc +++ b/src/main/native/windows/util.cc @@ -194,6 +194,15 @@ static bool Contains(const wstring& s, const WCHAR* substr) { return s.find(substr) != wstring::npos; } +static bool IsBatchFile(const wstring& path) { + if (path.size() < 4) { + return false; + } + const wchar_t* ext = path.c_str() + (path.size() - 4); + return CompareStringOrdinal(ext, 4, L".bat", 4, TRUE) == CSTR_EQUAL || + CompareStringOrdinal(ext, 4, L".cmd", 4, TRUE) == CSTR_EQUAL; +} + wstring AsShortPath(wstring path, wstring* result) { // Using `MAX_PATH` - 4 (256) instead of `MAX_PATH` to fix // https://github.com/bazelbuild/bazel/issues/12310 @@ -267,7 +276,8 @@ wstring AsShortPath(wstring path, wstring* result) { return L""; } -wstring AsExecutablePathForCreateProcess(wstring path, wstring* result) { +wstring AsExecutablePathForCreateProcess(wstring path, wstring* quoted_path, + wstring* extended_path) { if (path.empty()) { return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"AsExecutablePathForCreateProcess", path, @@ -287,16 +297,34 @@ wstring AsExecutablePathForCreateProcess(wstring path, wstring* result) { } path = cwd + L"\\" + path; } - wstring error = AsShortPath(path, result); - if (!error.empty()) { - return MakeErrorMessage(WSTR(__FILE__), __LINE__, - L"AsExecutablePathForCreateProcess", path, error); + wstring error = AsShortPath(path, quoted_path); + if (error.empty()) { + // Quote the path in case it's something like "c:\foo\app name.exe". + // Do this unconditionally, there's no harm in quoting. Quotes are not + // allowed inside paths so we don't need to escape quotes. + QuotePath(*quoted_path, quoted_path); + extended_path->clear(); + return L""; } - // Quote the path in case it's something like "c:\foo\app name.exe". - // Do this unconditionally, there's no harm in quoting. Quotes are not - // allowed inside paths so we don't need to escape quotes. - QuotePath(*result, result); - return L""; + // Shortening might fail (https://github.com/bazelbuild/bazel/issues/19710), + // typically when 8.3 aliases are disabled on the volume, which is common in + // containers (https://github.com/microsoft/Windows-Containers/issues/507). + // As a fallback, set `quoted_path` as above and `extended_path` to the + // extended-length form of `path`, suitable for CreateProcessW's + // lpApplicationName: it is not subject to MAX_PATH, and providing it lifts + // that limit from the executable part of CreateProcessW's lpCommandLine too. + // This works only for a plain executable with an absolute, normalized path. + if (!IsBatchFile(path)) { + wstring native_path = path; + std::replace(native_path.begin(), native_path.end(), L'/', L'\\'); + if (IsAbsoluteNormalizedWindowsPath(native_path)) { + QuotePath(native_path, quoted_path); + *extended_path = wstring(L"\\\\?\\") + native_path; + return L""; + } + } + return MakeErrorMessage(WSTR(__FILE__), __LINE__, + L"AsExecutablePathForCreateProcess", path, error); } } // namespace windows diff --git a/src/main/native/windows/util.h b/src/main/native/windows/util.h index a98e7f8efb9043..1a107fd5ecbeba 100644 --- a/src/main/native/windows/util.h +++ b/src/main/native/windows/util.h @@ -136,12 +136,15 @@ wstring GetLastErrorString(DWORD error_code); // Same as `AsExecutablePathForCreateProcess` except it won't quote the result. wstring AsShortPath(wstring path, wstring* result); -// Computes a path suitable as the executable part in CreateProcessA's cmdline. +// Computes a path suitable as the executable part in CreateProcessW's +// lpCommandLine, plus an extended-length form suitable for CreateProcessW's +// lpApplicationName when applicable. // -// The null-terminated executable path for CreateProcessA has to fit into -// MAX_PATH, therefore the limit for the executable's path is MAX_PATH - 1 -// (not including null terminator). This method attempts to convert the input -// `path` to a short format to fit it into the MAX_PATH - 1 limit. +// Unless CreateProcessW's lpApplicationName is populated, the null-terminated +// executable path in CreateProcessW's lpCommandLine has to fit into MAX_PATH, +// therefore the limit for the executable's path is MAX_PATH - 1 (not including +// null terminator). This method attempts to convert the input `path` to a short +// format to fit it into the MAX_PATH - 1 limit. // // `path` must be either an absolute, normalized, Windows-style path with drive // letter (e.g. "c:\foo\bar.exe", but no "\foo\bar.exe"), or must be just a file @@ -149,17 +152,23 @@ wstring AsShortPath(wstring path, wstring* result); // In both cases, `path` must be unquoted. // // If this function succeeds, it returns an empty string (indicating no error), -// and sets `result` to the resulting path, which is always quoted, and is +// and sets `quoted_path` to the resulting path, which is always quoted, and is // always at most MAX_PATH + 1 long (MAX_PATH - 1 without null terminator, plus -// two quotes). If there's any error, this function returns the error message. +// two quotes), unless the `extended_path` fallback below applies. If there's +// any error, this function returns the error message. // // If `path` is at most MAX_PATH - 1 long (not including null terminator), the // result will be that (plus quotes). // Otherwise this method attempts to compute an 8dot3 style short name for // `path`, and if that succeeds and the result is at most MAX_PATH - 1 long (not // including null terminator), then that will be the result (plus quotes). +// Otherwise, if `path` is an absolute, normalized path to a plain executable +// (not a batch file), `extended_path` is set to its extended-length form (the +// "\\?\" prefix plus `path`) for use as CreateProcessW's lpApplicationName, and +// `quoted_path` to the quoted `path`. // Otherwise this function fails and returns an error message. -wstring AsExecutablePathForCreateProcess(wstring path, wstring* result); +wstring AsExecutablePathForCreateProcess(wstring path, wstring* quoted_path, + wstring* extended_path); } // namespace windows } // namespace bazel diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java index f01c236c043d15..ff7aa656580e1d 100644 --- a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java +++ b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java @@ -327,6 +327,25 @@ public void testExecutableNotFound() throws Exception { assertThat(readStdout(buf, 0, 1)).isEqualTo(0); } + @Test + public void testExecutableWithExtendedLengthPath() throws Exception { + StringBuilder dir = new StringBuilder(System.getenv("TEST_TMPDIR")); + for (char c = 'a'; c <= 'z'; c++) { + dir.append('\\').repeat(c, 8).append('.').repeat(c, 3); + } + String exe = dir + "\\cmd.exe"; + assertThat(exe.length()).isGreaterThan(260); // Windows MAX_PATH + Files.createDirectories(Paths.get("\\\\?\\" + dir)); + Files.copy( + Paths.get(System.getenv("SystemRoot"), "System32", "cmd.exe"), + Paths.get("\\\\?\\" + exe)); + + process = WindowsProcesses.createProcess(exe, "/c exit 42", null, null, null, null); + assertNoProcessError(); + assertThat(WindowsProcesses.waitFor(process, -1)).isEqualTo(0); + assertThat(WindowsProcesses.getExitCode(process)).isEqualTo(42); + } + @Test public void testReadingAndWritingAfterTermination() throws Exception { process = diff --git a/src/test/native/windows/util_test.cc b/src/test/native/windows/util_test.cc index 2d4cd543ffedf0..09cbde19ba65b8 100644 --- a/src/test/native/windows/util_test.cc +++ b/src/test/native/windows/util_test.cc @@ -236,21 +236,40 @@ static void DeleteDirsUnder(const wstring& basedir, } // This is a macro so the assertions will have the correct line number. -#define ASSERT_SHORTENING_FAILS(/* const WCHAR* */ input, \ - /* const WCHAR* */ error_msg) \ - { \ - wstring actual; \ - wstring result(AsExecutablePathForCreateProcess(input, &actual)); \ - ASSERT_CONTAINS(result, error_msg); \ +#define ASSERT_SHORTENING_FAILS(/* const WCHAR* */ input, \ + /* const WCHAR* */ error_msg) \ + { \ + wstring actual; \ + wstring extended_path; \ + wstring result( \ + AsExecutablePathForCreateProcess(input, &actual, &extended_path)); \ + ASSERT_CONTAINS(result, error_msg); \ + ASSERT_EQ(extended_path, L""); \ } // This is a macro so the assertions will have the correct line number. -#define ASSERT_SHORTENING_SUCCEEDS(/* const WCHAR* */ input, \ - /* const wstring& */ expected_result) \ - { \ - wstring actual; \ - ASSERT_EQ(AsExecutablePathForCreateProcess(input, &actual), L""); \ - ASSERT_EQ(actual, expected_result); \ +#define ASSERT_SHORTENING_SUCCEEDS(/* const WCHAR* */ input, \ + /* const wstring& */ expected_result) \ + { \ + wstring actual; \ + wstring extended_path = L"stale"; \ + ASSERT_EQ( \ + AsExecutablePathForCreateProcess(input, &actual, &extended_path), \ + L""); \ + ASSERT_EQ(actual, expected_result); \ + ASSERT_EQ(extended_path, L""); \ + } + +// This is a macro so the assertions will have the correct line number. +#define ASSERT_FALLBACK_SUCCEEDS(/* const WCHAR* */ input) \ + { \ + wstring actual; \ + wstring extended_path; \ + ASSERT_EQ( \ + AsExecutablePathForCreateProcess(input, &actual, &extended_path), \ + L""); \ + ASSERT_EQ(extended_path, wstring(L"\\\\?\\") + input); \ + ASSERT_EQ(actual, wstring(L"\"") + input + L"\""); \ } TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessBadInputs) { @@ -271,7 +290,9 @@ TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessBadInputs) { // Relative paths are fine, they are absolutized. std::wstring rel(L"foo\\bar.exe"); std::wstring actual; - EXPECT_EQ(AsExecutablePathForCreateProcess(rel, &actual), L""); + std::wstring extended_path; + EXPECT_EQ(AsExecutablePathForCreateProcess(rel, &actual, &extended_path), + L""); EXPECT_GT(actual.size(), rel.size()); EXPECT_EQ(actual.rfind(rel), actual.size() - rel.size() - 1); } @@ -298,7 +319,7 @@ TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessConversions) { // 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 + // a non-existent path to CreateProcessW, then it'll fail, so we'll find out // about this error in production code. // When i>0 then `wfilename` is at least `kMaxPath` long, so // `AsExecutablePathForCreateProcess` will attempt to shorten it, but @@ -349,5 +370,23 @@ TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessConversions) { DeleteDirsUnder(tmpdir, short_root); } +TEST(WindowsUtilTest, TestAsExecutablePathForCreateProcessFallback) { + wstring dir = L"c:\\" + wstring(kMaxPath, L'a'); + + // A deep, absolute, normalized executable path that can't be shortened (here + // because it doesn't exist) takes the fallback: `extended_path` gets the + // extended-length form and `quoted_path` the quoted path. + ASSERT_FALLBACK_SUCCEEDS(dir + L"\\foo.exe"); + + // A batch file is denied the fallback even with an absolute, normalized path: + // CreateProcessW's lpApplicationName can only take a plain executable. + ASSERT_SHORTENING_FAILS(dir + L"\\foo.bat", L"GetShortPathNameW"); + + // A non-normalized path is denied the fallback because the "\\?\" prefix + // disables path normalization, so "." and ".." would reach the filesystem + // verbatim instead of being resolved. + ASSERT_SHORTENING_FAILS(dir + L"\\..\\foo.exe", L"path is not normalized"); +} + } // namespace windows } // namespace bazel