Windows: fix "bazel run" argument quoting

There are two axes of variables: - server mode vs. batch mode (--[no]batch) - Windows only: Bash-less vs. Bash-ful bazel run (--[no]incompatible_windows_bashless_run_command) To "bazel run" a target, Bazel first builds the target then creates a run request. The request is a protobuf that contains the command line (argv) and environment (envvars, cwd). In server mode (--nobatch), the Bazel server sends the run request to the client, and the client executes the program (with CreateProcessW on Windows / execv on Unixes). In batch mode (--batch), the Bazel server itself executes the program using ProcessBuilder. In Bash-ful bazel run mode the run request is for "bash -c <program> <args>...", while in Bash-less more it is for "<program> <args>...". The argument escaping must be different in both cases, because bash.exe uses Bash-style quoting/escaping while most native Windows programs use the MSVC style. Fixes bazelbuild#9106 Fixes bazelbuild#9108 Closes bazelbuild#9123. PiperOrigin-RevId: 262519795
buchgr · Aug 9, 2019 · da557f9 · da557f9
1 parent c62c2a6
commit da557f9
Show file tree

Hide file tree

Showing 8 changed files with 124 additions and 61 deletions.
diff --git a/src/main/cpp/BUILD b/src/main/cpp/BUILD
@@ -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",
         ],

diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
@@ -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();
   }
@@ -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()

diff --git a/src/main/cpp/blaze_util_platform.h b/src/main/cpp/blaze_util_platform.h
@@ -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:

diff --git a/src/main/cpp/blaze_util_posix.cc b/src/main/cpp/blaze_util_posix.cc
@@ -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();
 
@@ -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) {

diff --git a/src/main/cpp/blaze_util_windows.cc b/src/main/cpp/blaze_util_windows.cc
@@ -18,12 +18,12 @@
 #include <windows.h>
 
 #include <fcntl.h>
-#include <io.h>              // _open
-#include <knownfolders.h>    // FOLDERID_Profile
-#include <lmcons.h>          // UNLEN
-#include <objbase.h>         // CoTaskMemFree
-#include <shlobj.h>          // SHGetKnownFolderPath
-#include <stdarg.h>          // va_start, va_end, va_list
+#include <io.h>            // _open
+#include <knownfolders.h>  // FOLDERID_Profile
+#include <lmcons.h>        // UNLEN
+#include <objbase.h>       // CoTaskMemFree
+#include <shlobj.h>        // SHGetKnownFolderPath
+#include <stdarg.h>        // va_start, va_end, va_list
 
 #include <algorithm>
 #include <cstdio>
@@ -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 {
 
@@ -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()) {
@@ -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();
@@ -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;
   {
@@ -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;
@@ -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) {

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -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));

diff --git a/src/test/shell/integration/py_args_escaping_test.sh b/src/test/shell/integration/py_args_escaping_test.sh
@@ -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() {

diff --git a/src/tools/launcher/util/BUILD b/src/tools/launcher/util/BUILD
@@ -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__",
     ],