Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: fix "bazel run" argument quoting #9123

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/cpp/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ cc_library(
"//src/main/cpp/util:logging",
] + select({
"//src/conditions:windows": [
"//src/tools/launcher/util",
"//src/main/native/windows:lib-file",
"//src/main/native/windows:lib-process",
],
Expand Down
4 changes: 2 additions & 2 deletions src/main/cpp/blaze.cc
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,7 @@ static void RunBatchMode(

{
WithEnvVars env_obj(PrepareEnvironmentForJvm());
ExecuteProgram(server_exe, jvm_args_vector);
ExecuteServerJvm(server_exe, jvm_args_vector);
BAZEL_DIE(blaze_exit_code::INTERNAL_ERROR)
<< "execv of '" << server_exe << "' failed: " << GetLastErrorString();
}
Expand Down Expand Up @@ -2014,7 +2014,7 @@ unsigned int BlazeServer::Communicate(
// Execute the requested program, but before doing so, flush everything
// we still have to say.
fflush(NULL);
ExecuteProgram(request.argv(0), argv);
ExecuteRunRequest(request.argv(0), argv);
}

// We'll exit with exit code SIGPIPE on Unixes due to PropagateSignalOnExit()
Expand Down
20 changes: 16 additions & 4 deletions src/main/cpp/blaze_util_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,23 @@ std::string GetSystemJavabase();
// Return the path to the JVM binary relative to a javabase, e.g. "bin/java".
std::string GetJavaBinaryUnderJavabase();

// Replace the current process with the given program in the current working
// directory, using the given argument vector.
// Start the Bazel server's JVM in the current directory.
//
// Note on Windows: 'server_jvm_args' is NOT expected to be escaped for
// CreateProcessW.
//
// This function does not return on success.
void ExecuteServerJvm(const std::string& exe,
const std::vector<std::string>& server_jvm_args);

// Execute the "bazel run" request in the current directory.
//
// Note on Windows: 'run_request_args' IS expected to be escaped for
// CreateProcessW.
//
// This function does not return on success.
void ExecuteProgram(const std::string& exe,
const std::vector<std::string>& args_vector);
void ExecuteRunRequest(const std::string& exe,
const std::vector<std::string>& run_request_args);

class BlazeServerStartup {
public:
Expand Down
13 changes: 12 additions & 1 deletion src/main/cpp/blaze_util_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,8 @@ class CharPP {
char** charpp_;
};

void ExecuteProgram(const string& exe, const vector<string>& args_vector) {
static void ExecuteProgram(const string& exe,
const vector<string>& args_vector) {
BAZEL_LOG(INFO) << "Invoking binary " << exe << " in "
<< blaze_util::GetCwd();

Expand All @@ -289,6 +290,16 @@ void ExecuteProgram(const string& exe, const vector<string>& args_vector) {
execv(exe.c_str(), argv.get());
}

void ExecuteServerJvm(const string& exe,
const std::vector<string>& server_jvm_args) {
ExecuteProgram(exe, server_jvm_args);
}

void ExecuteRunRequest(const string& exe,
const std::vector<string>& run_request_args) {
ExecuteProgram(exe, run_request_args);
}

const char kListSeparator = ':';

bool SymlinkDirectories(const string &target, const string &link) {
Expand Down
89 changes: 45 additions & 44 deletions src/main/cpp/blaze_util_windows.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#include "src/main/native/windows/file.h"
#include "src/main/native/windows/process.h"
#include "src/main/native/windows/util.h"
#include "src/tools/launcher/util/launcher_util.h"

namespace blaze {

Expand Down Expand Up @@ -477,17 +478,14 @@ namespace {

// Max command line length is per CreateProcess documentation
// (https://msdn.microsoft.com/en-us/library/ms682425(VS.85).aspx)
//
// Quoting rules are described here:
// https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/

static const int MAX_CMDLINE_LENGTH = 32768;

struct CmdLine {
WCHAR cmdline[MAX_CMDLINE_LENGTH];
};
static void CreateCommandLine(CmdLine* result, const string& exe,
const std::vector<string>& args_vector) {
const std::vector<std::wstring>& wargs_vector) {
std::wstringstream cmdline;
string short_exe;
if (!exe.empty()) {
Expand All @@ -501,50 +499,15 @@ static void CreateCommandLine(CmdLine* result, const string& exe,
}

bool first = true;
for (const auto& s : args_vector) {
for (const std::wstring& wa : wargs_vector) {
if (first) {
// Skip first argument, it is equal to 'exe'.
first = false;
continue;
} else {
cmdline << L' ';
}

bool has_space = s.find(" ") != string::npos;

if (has_space) {
cmdline << L'\"';
}

wstring ws = blaze_util::CstringToWstring(s.c_str()).get();
std::wstring::const_iterator it = ws.begin();
while (it != ws.end()) {
wchar_t ch = *it++;
switch (ch) {
case L'"':
// Escape double quotes
cmdline << L"\\\"";
break;

case L'\\':
if (it == ws.end()) {
// Backslashes at the end of the string are quoted if we add quotes
cmdline << (has_space ? L"\\\\" : L"\\");
} else {
// Backslashes everywhere else are quoted if they are followed by a
// quote or a backslash
cmdline << (*it == L'"' || *it == L'\\' ? L"\\\\" : L"\\");
}
break;

default:
cmdline << ch;
}
}

if (has_space) {
cmdline << L'\"';
}
cmdline << wa;
}

wstring cmdline_str = cmdline.str();
Expand Down Expand Up @@ -722,8 +685,16 @@ int ExecuteDaemon(const string& exe,
STARTUPINFOEXW startupInfoEx = {0};
lpAttributeList->InitStartupInfoExW(&startupInfoEx);

std::vector<std::wstring> wesc_args_vector;
wesc_args_vector.reserve(args_vector.size());
for (const string& a : args_vector) {
std::wstring wa = blaze_util::CstringToWstring(a.c_str()).get();
std::wstring wesc = bazel::launcher::WindowsEscapeArg2(wa);
wesc_args_vector.push_back(wesc);
}

CmdLine cmdline;
CreateCommandLine(&cmdline, exe, args_vector);
CreateCommandLine(&cmdline, exe, wesc_args_vector);

BOOL ok;
{
Expand Down Expand Up @@ -771,11 +742,12 @@ int ExecuteDaemon(const string& exe,
// Run the given program in the current working directory, using the given
// argument vector, wait for it to finish, then exit ourselves with the exitcode
// of that program.
void ExecuteProgram(const string& exe, const std::vector<string>& args_vector) {
static void ExecuteProgram(const string& exe,
const std::vector<std::wstring>& wargs_vector) {
std::wstring wexe = blaze_util::CstringToWstring(exe.c_str()).get();

CmdLine cmdline;
CreateCommandLine(&cmdline, "", args_vector);
CreateCommandLine(&cmdline, "", wargs_vector);

bazel::windows::WaitableProcess proc;
std::wstring werror;
Expand All @@ -796,6 +768,35 @@ void ExecuteProgram(const string& exe, const std::vector<string>& args_vector) {
exit(x);
}

void ExecuteServerJvm(const string& exe,
const std::vector<string>& server_jvm_args) {
std::vector<std::wstring> wargs;
wargs.reserve(server_jvm_args.size());
for (const string& a : server_jvm_args) {
std::wstring wa = blaze_util::CstringToWstring(a.c_str()).get();
std::wstring wesc = bazel::launcher::WindowsEscapeArg2(wa);
wargs.push_back(wesc);
}

ExecuteProgram(exe, wargs);
}

void ExecuteRunRequest(const string& exe,
const std::vector<string>& run_request_args) {
std::vector<std::wstring> wargs;
wargs.reserve(run_request_args.size());
std::wstringstream joined;
for (const string& a : run_request_args) {
std::wstring wa = blaze_util::CstringToWstring(a.c_str()).get();
// The arguments are already escaped (Bash-style or Windows-style, depending
// on --[no]incompatible_windows_bashless_run_command).
wargs.push_back(wa);
joined << L' ' << wa;
}

ExecuteProgram(exe, wargs);
}

const char kListSeparator = ';';

bool SymlinkDirectories(const string &posix_target, const string &posix_name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -519,9 +519,16 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
return BlazeCommandResult.exitCode(ExitCode.COMMAND_LINE_ERROR);
}

String shellEscaped = ShellEscaper.escapeJoinAll(cmdLine);
if (OS.getCurrent() == OS.WINDOWS) {
// On Windows, we run Bash as a subprocess of the client (via CreateProcessW).
// Bash uses its own (Bash-style) flag parsing logic, not the default logic for which
// ShellUtils.windowsEscapeArg escapes, so we escape the flags once again Bash-style.
shellEscaped = "\"" + shellEscaped.replace("\\", "\\\\").replace("\"", "\\\"") + "\"";
}

ImmutableList<String> shellCmdLine =
ImmutableList.<String>of(
shExecutable.getPathString(), "-c", ShellEscaper.escapeJoinAll(cmdLine));
ImmutableList.<String>of(shExecutable.getPathString(), "-c", shellEscaped);

for (String arg : shellCmdLine) {
execDescription.addArgv(ByteString.copyFrom(arg, StandardCharsets.ISO_8859_1));
Expand Down
34 changes: 32 additions & 2 deletions src/test/shell/integration/py_args_escaping_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,41 @@ function test_args_escaping() {
create_py_file_that_prints_args "$ws"
create_build_file_with_many_args "$ws"

# On all platforms, the target prints good output.
# Batch mode, Bash-less run command.
if $is_windows; then
( cd "$ws"
bazel --batch run --incompatible_windows_bashless_run_command \
--verbose_failures :x &>"$TEST_log" || fail "expected success"
)
assert_good_output_of_the_program_with_many_args
rm "$TEST_log"
fi

# Batch mode, Bash-ful run command.
( cd "$ws"
bazel --batch run --noincompatible_windows_bashless_run_command \
--verbose_failures :x &>"$TEST_log" || fail "expected success"
)
assert_good_output_of_the_program_with_many_args
rm "$TEST_log"

# Server mode, Bash-less run command.
if $is_windows; then
( cd "$ws"
bazel run --incompatible_windows_bashless_run_command \
--verbose_failures :x &>"$TEST_log" || fail "expected success"
)
assert_good_output_of_the_program_with_many_args
rm "$TEST_log"
fi

# Server mode, Bash-ful run command.
( cd "$ws"
bazel run --verbose_failures :x &>"$TEST_log" || fail "expected success"
bazel run --noincompatible_windows_bashless_run_command \
--verbose_failures :x &>"$TEST_log" || fail "expected success"
)
assert_good_output_of_the_program_with_many_args
rm "$TEST_log"
}

function test_untokenizable_args() {
Expand Down
1 change: 1 addition & 0 deletions src/tools/launcher/util/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ win_cc_library(
srcs = ["launcher_util.cc"],
hdrs = ["launcher_util.h"],
visibility = [
"//src/main/cpp:__pkg__",
"//src/tools/launcher:__subpackages__",
"//tools/test:__pkg__",
],
Expand Down