Repository: bazelbuild/bazel
Issue: Windows: bash-less run argument quoting is wrong
Labels: type: bug
P1
area-Windows
team-OSS

### Description of the problem / feature request:

The Bash-less "bazel run" (see https://github.com/bazelbuild/bazel/issues/8240) quotes arguments incorrectly.

It works correctly in `--batch` mode, but adds extra `" "` around escaped arguments in server mode.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

`foo/BUILD`:
```
py_binary(
    name = "x",
    srcs = ["a.py"],
    main = "a.py",
    args = ["foo", "' '", "bar"],
)
```

`foo/a.py`:
```
from __future__ import print_function
import sys
for i in range(1, len(sys.argv)):
    print("arg%d=(%s)" % (i, sys.argv[i]))
```

```
C:\src\gh8240>bazel --nobatch run foo:x --noincompatible_windows_bashless_run_command
(...)
arg1=(foo)
arg2=( )
arg3=(bar)

C:\src\gh8240>bazel --nobatch run foo:x --incompatible_windows_bashless_run_command
(...)
arg1=(foo)
arg2=(" ")
arg3=(bar)

C:\src\gh8240>bazel --batch run foo:x --noincompatible_windows_bashless_run_command
(...)
arg1=(foo)
arg2=( )
arg3=(bar)

C:\src\gh8240>bazel --batch run foo:x --incompatible_windows_bashless_run_command
(...)
arg1=(foo)
arg2=( )
arg3=(bar)
```

On Linux, the output is always the same for all flag combinations:
```
arg1=(foo)
arg2=( )
arg3=(bar)
```

### What operating system are you running Bazel on?

windows 10

### What's the output of `bazel info release`?

0.28.1

###  Have you found anything relevant by searching the web?

This is blocking https://github.com/bazelbuild/bazel/issues/8240

Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/cpp/BUILD b/src/main/cpp/BUILD
index 7de595dddebf4f..9e18a2b19121e5 100644
--- 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
index 5577d6a593835f..4b8e8f24d3ee6c 100644
--- 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
index e7f5c3e5024ec0..c7c3aeefd2a7d8 100644
--- 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
index c7ee7b6b3ffbff..87ba899180f1a5 100644
--- 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
index 05dd37384a5335..c76885e4bcaf2e 100644
--- a/src/main/cpp/blaze_util_windows.cc
+++ b/src/main/cpp/blaze_util_windows.cc
@@ -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,9 +478,6 @@ 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;
 
@@ -487,7 +485,7 @@ 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,7 +499,7 @@ 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;
@@ -509,42 +507,7 @@ static void CreateCommandLine(CmdLine* result, const string& exe,
     } 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
index de994231a0dbe6..47f4f68fa82390 100644
--- 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/tools/launcher/util/BUILD b/src/tools/launcher/util/BUILD
index 70a4c05f1332cc..c4a66ef90a1901 100644
--- 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__",
     ],


Test patch:
diff --git a/src/test/shell/integration/py_args_escaping_test.sh b/src/test/shell/integration/py_args_escaping_test.sh
index 59bf5b14bb2337..f3d4ed42c48d89 100755
--- 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() {