From 237ff2c776bbc9af7974a4895d1532a9fca9e6a6 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Tue, 12 Feb 2019 13:15:53 +0100 Subject: [PATCH 1/4] Windows, launcher: add unit test for flag escaping Add a unit test to assert that: - WindowsEscapeArg escapes flags as expected - When we pass a WindowsEscapeArg-escaped flag to CreateProcessW, the subprocess receives the original flag. This is a follow-up to https://github.com/bazelbuild/bazel/pull/7395 Next I'll fix WindowsEscapeArg to correctly escape arguments. See https://github.com/bazelbuild/bazel/issues/7072 --- src/main/native/windows/BUILD | 1 + src/tools/launcher/util/BUILD | 11 ++ src/tools/launcher/util/launcher_util_test.cc | 179 +++++++++++++++++- src/tools/launcher/util/printarg.cc | 20 ++ 4 files changed, 208 insertions(+), 3 deletions(-) create mode 100644 src/tools/launcher/util/printarg.cc diff --git a/src/main/native/windows/BUILD b/src/main/native/windows/BUILD index d7429dcead5dcc..ff82bd21323ba1 100644 --- a/src/main/native/windows/BUILD +++ b/src/main/native/windows/BUILD @@ -35,6 +35,7 @@ cc_library( srcs = ["util.cc"], hdrs = ["util.h"], visibility = [ + "//src/tools/launcher/util:__pkg__", "//tools/test:__pkg__", ], ) diff --git a/src/tools/launcher/util/BUILD b/src/tools/launcher/util/BUILD index d8927c6f35a13d..68730ec9638ea3 100644 --- a/src/tools/launcher/util/BUILD +++ b/src/tools/launcher/util/BUILD @@ -27,8 +27,13 @@ cc_test( srcs = ["launcher_util_test.cc"], deps = [ ":util", + "//src/main/native/windows:lib-util", + "@bazel_tools//tools/cpp/runfiles", "@com_google_googletest//:gtest_main", ], + data = [ + ":printarg", + ], ) cc_test( @@ -40,6 +45,12 @@ cc_test( ], ) +cc_binary( + name = "printarg", + srcs = ["printarg.cc"], + testonly = 1, +) + test_suite( name = "windows_tests", tags = [ diff --git a/src/tools/launcher/util/launcher_util_test.cc b/src/tools/launcher/util/launcher_util_test.cc index 43d84e45034204..96d04e1aa99d00 100644 --- a/src/tools/launcher/util/launcher_util_test.cc +++ b/src/tools/launcher/util/launcher_util_test.cc @@ -16,15 +16,21 @@ #include #include #include +#include #include +#include +#include "src/main/native/windows/util.h" +#include "src/main/cpp/util/path_platform.h" #include "src/main/cpp/util/strings.h" #include "src/tools/launcher/util/launcher_util.h" +#include "tools/cpp/runfiles/runfiles.h" #include "gtest/gtest.h" namespace bazel { namespace launcher { +using bazel::tools::cpp::runfiles::Runfiles; using std::getenv; using std::ios; using std::ofstream; @@ -86,12 +92,179 @@ TEST_F(LaunchUtilTest, BashEscapeArgTest) { BashEscapeArg(L"C:\\foo foo\\bar\\")); } +// Asserts argument escaping for subprocesses. +// +// For each pair in 'args', this method: +// 1. asserts that WindowsEscapeArg(pair.first) == pair.second +// 2. asserts that passing pair.second to a subprocess results in the subprocess +// receiving pair.first +// +// The method performs the second assertion by running "printarg.exe" (a +// data-dependency of this test) once for each argument. +void AssertSubprocessReceivesArgsAsIntended( + const std::vector >& args) { + // Assert that the WindowsEscapeArg produces what we expect. + for (const auto& i : args) { + ASSERT_EQ(WindowsEscapeArg(i.first), i.second); + } + + // Create a Runfiles object. + string error; + std::unique_ptr runfiles( + bazel::tools::cpp::runfiles::Runfiles::CreateForTest(&error)); + ASSERT_NE(runfiles.get(), nullptr) << error; + + // Look up the path of the printarg.exe utility. + string printarg = + runfiles->Rlocation("io_bazel/src/tools/launcher/util/printarg.exe"); + ASSERT_NE(printarg, ""); + + // Convert printarg.exe's path to a wchar_t Windows path. + wstring wprintarg; + bool success = blaze_util::AsAbsoluteWindowsPath(printarg, &wprintarg, + &error); + ASSERT_TRUE(success) << error; + + // SECURITY_ATTRIBUTES for inheritable HANDLEs. + SECURITY_ATTRIBUTES sa; + sa.nLength = sizeof(sa); + sa.lpSecurityDescriptor = NULL; + sa.bInheritHandle = TRUE; + + // Open /dev/null that will be redirected into the subprocess' stdin. + bazel::windows::AutoHandle devnull( + CreateFileW(L"NUL", GENERIC_READ, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, &sa, + OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL)); + ASSERT_TRUE(devnull.IsValid()); + + // Create a pipe that the subprocess' stdout will be redirected to. + HANDLE pipe_read_h, pipe_write_h; + if (!CreatePipe(&pipe_read_h, &pipe_write_h, &sa, 0x10000)) { + DWORD err = GetLastError(); + ASSERT_EQ(err, 0); + } + bazel::windows::AutoHandle pipe_read(pipe_read_h), pipe_write(pipe_write_h); + + // Duplicate stderr, where the subprocess' stderr will be redirected to. + HANDLE stderr_h; + if (!DuplicateHandle(GetCurrentProcess(), GetStdHandle(STD_ERROR_HANDLE), + GetCurrentProcess(), &stderr_h, 0, TRUE, + DUPLICATE_SAME_ACCESS)) { + DWORD err = GetLastError(); + ASSERT_EQ(err, 0); + } + bazel::windows::AutoHandle stderr_dup(stderr_h); + + // Create the attribute object for the process creation. This object describes + // exactly which handles the subprocess shall inherit. + STARTUPINFOEXW startupInfo; + std::unique_ptr attrs; + wstring werror; + ASSERT_TRUE( + bazel::windows::AutoAttributeList::Create( + devnull, pipe_write, stderr_dup, &attrs, &werror)); + attrs->InitStartupInfoExW(&startupInfo); + + // MSDN says the maximum command line is 32767 characters, with a null + // terminator that is exactly 2^15 (= 0x8000). + static constexpr size_t kMaxCmdline = 0x8000; + wchar_t cmdline[kMaxCmdline]; + // Copy printarg.exe's escaped path into the 'cmdline', and append a space. + // We will append arguments to this command line in the for-loop below. + wcsncpy(cmdline, wprintarg.c_str(), wprintarg.size()); + wchar_t *pcmdline = cmdline + wprintarg.size(); + *pcmdline++ = L' '; + + // Run a subprocess for each of the arguments and assert that the argument + // arrived to the subprocess as intended. + for (const auto& i : args) { + // We already asserted for every element that WindowsEscapeArg(i.first) + // produces the same output as i.second, so just use i.second instead of + // converting i.first again. + wcsncpy(pcmdline, i.second.c_str(), wprintarg.size()); + pcmdline[wprintarg.size()] = 0; + + // Run the subprocess. + PROCESS_INFORMATION processInfo; + BOOL ok = CreateProcessW( + NULL, cmdline, NULL, NULL, TRUE, + CREATE_UNICODE_ENVIRONMENT | EXTENDED_STARTUPINFO_PRESENT, NULL, NULL, + &startupInfo.StartupInfo, &processInfo); + if (!ok) { + DWORD err = GetLastError(); + ASSERT_EQ(err, 0); + } + CloseHandle(processInfo.hThread); + bazel::windows::AutoHandle process(processInfo.hProcess); + + // Wait for the subprocess to exit. Timeout is 5 seconds, which should be + // more than enough for the subprocess to finish. + ASSERT_EQ(WaitForSingleObject(process, 5000), WAIT_OBJECT_0); + + // The subprocess printed its argv[1] (without a newline) to its stdout, + // which is redirected into the pipe. + // Let's write a null-terminator to the pipe to separate the output from the + // output of the subsequent subprocess. The null-terminator also yields + // null-terminated strings in the pipe, making it easy to read them out + // later. + DWORD dummy; + ASSERT_TRUE(WriteFile(pipe_write, "\0", 1, &dummy, NULL)); + } + + // Read the output of the subprocesses from the pipe. They are divided by + // null-terminators, so 'buf' will contain a sequence of null-terminated + // strings. We close the writing end so that ReadFile won't block until the + // desired amount of bytes is available. + DWORD total_output_len; + char buf[0x10000]; + pipe_write = INVALID_HANDLE_VALUE; + if (!ReadFile(pipe_read, buf, 0x10000, &total_output_len, NULL)) { + DWORD err = GetLastError(); + ASSERT_EQ(err, 0); + } + + // Assert that the subprocesses produced exactly the *unescaped* arguments. + size_t start = 0; + for (const auto& arg : args) { + // Assert that there was enough data produced by the subprocesses. + ASSERT_LT(start, total_output_len); + + // Find the output of the corresponding subprocess. Since all subprocesses + // printed into the same pipe and we added null-terminators between them, + // the output is already there, conveniently as a null-terminated string. + string actual_arg(buf + start); + start += actual_arg.size() + 1; + + // 'args' contains wchar_t strings, but the subprocesses printed ASCII + // (char) strings. To compare, we convert arg.first to a char-string. + string expected_arg; + expected_arg.reserve(arg.first.size()); + for (const auto& wc : arg.first) { + expected_arg.append(1, static_cast(wc)); + } + + // Assert that the subprocess printed exactly the *unescaped* argument. + EXPECT_EQ(expected_arg, actual_arg); + } +} + TEST_F(LaunchUtilTest, WindowsEscapeArgTest) { - ASSERT_EQ(L"foo\\bar", WindowsEscapeArg(L"foo\\bar")); - ASSERT_EQ(L"C:\\foo\\bar\\", WindowsEscapeArg(L"C:\\foo\\bar\\")); - ASSERT_EQ(L"\"C:\\foo foo\\bar\\\"", WindowsEscapeArg(L"C:\\foo foo\\bar\\")); + // List of arguments with their expected WindowsEscapeArg-encoded version. + AssertSubprocessReceivesArgsAsIntended({ + // Each pair is: + // - first: argument to pass (and expected output from subprocess) + // - second: expected WindowsEscapeArg-encoded string + {L"foo", L"foo"}, + {L"", L"\"\""}, + {L"foo\\bar", L"foo\\bar"}, + {L"C:\\foo\\bar\\", L"C:\\foo\\bar\\"}, + // TODO(laszlocsomor): fix WindowsEscapeArg to use correct escaping + // semantics (not Bash semantics) and add more tests. + }); } + TEST_F(LaunchUtilTest, DoesFilePathExistTest) { wstring file1 = GetTmpDir() + L"/foo"; wstring file2 = GetTmpDir() + L"/bar"; diff --git a/src/tools/launcher/util/printarg.cc b/src/tools/launcher/util/printarg.cc new file mode 100644 index 00000000000000..6b789f518e2065 --- /dev/null +++ b/src/tools/launcher/util/printarg.cc @@ -0,0 +1,20 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +int main(int argc, char** argv) { + printf(argv[1]); + return 0; +} From 3525dd0031bf8dff89148dbfaedbd28a3b0fcf7f Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Wed, 13 Feb 2019 08:29:03 +0100 Subject: [PATCH 2/4] Add more test cases. --- src/tools/launcher/util/launcher_util_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tools/launcher/util/launcher_util_test.cc b/src/tools/launcher/util/launcher_util_test.cc index 96d04e1aa99d00..4c707938ac173c 100644 --- a/src/tools/launcher/util/launcher_util_test.cc +++ b/src/tools/launcher/util/launcher_util_test.cc @@ -257,10 +257,13 @@ TEST_F(LaunchUtilTest, WindowsEscapeArgTest) { // - second: expected WindowsEscapeArg-encoded string {L"foo", L"foo"}, {L"", L"\"\""}, + {L" ", L"\" \""}, {L"foo\\bar", L"foo\\bar"}, {L"C:\\foo\\bar\\", L"C:\\foo\\bar\\"}, // TODO(laszlocsomor): fix WindowsEscapeArg to use correct escaping - // semantics (not Bash semantics) and add more tests. + // semantics (not Bash semantics) and add more tests. The example below is + // escaped incorrectly. + // {L"C:\\foo bar\\", L"\"C:\\foo bar\\\""}, }); } From 950487584df222ee2d2711667954d8e9bfd755d5 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Wed, 13 Feb 2019 08:49:58 +0100 Subject: [PATCH 3/4] wprintarg's path is now escaped --- src/tools/launcher/util/launcher_util_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tools/launcher/util/launcher_util_test.cc b/src/tools/launcher/util/launcher_util_test.cc index 4c707938ac173c..f575c96a629e83 100644 --- a/src/tools/launcher/util/launcher_util_test.cc +++ b/src/tools/launcher/util/launcher_util_test.cc @@ -170,8 +170,10 @@ void AssertSubprocessReceivesArgsAsIntended( // terminator that is exactly 2^15 (= 0x8000). static constexpr size_t kMaxCmdline = 0x8000; wchar_t cmdline[kMaxCmdline]; + // Copy printarg.exe's escaped path into the 'cmdline', and append a space. // We will append arguments to this command line in the for-loop below. + wprintarg = WindowsEscapeArg(wprintarg); wcsncpy(cmdline, wprintarg.c_str(), wprintarg.size()); wchar_t *pcmdline = cmdline + wprintarg.size(); *pcmdline++ = L' '; From b5dc214755b623f99a9cb6e45ab1d05e940027d9 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Wed, 13 Feb 2019 08:52:02 +0100 Subject: [PATCH 4/4] Fix string copying. --- src/tools/launcher/util/launcher_util_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/launcher/util/launcher_util_test.cc b/src/tools/launcher/util/launcher_util_test.cc index f575c96a629e83..55bf4aabb66f4e 100644 --- a/src/tools/launcher/util/launcher_util_test.cc +++ b/src/tools/launcher/util/launcher_util_test.cc @@ -184,8 +184,8 @@ void AssertSubprocessReceivesArgsAsIntended( // We already asserted for every element that WindowsEscapeArg(i.first) // produces the same output as i.second, so just use i.second instead of // converting i.first again. - wcsncpy(pcmdline, i.second.c_str(), wprintarg.size()); - pcmdline[wprintarg.size()] = 0; + wcsncpy(pcmdline, i.second.c_str(), i.second.size()); + pcmdline[i.second.size()] = 0; // Run the subprocess. PROCESS_INFORMATION processInfo;