Windows: fix java_binary.jvm_flags escaping

Bazel on Windows now correctly forwards
java_binary.jvm_flags to the binary.

Bazel Bash-tokenizes java_binary.jvm_flags.
On Linux/macOS, this is done by embedding the
flags into the Java stub script, which is a Bash
script, so Bash itself does the tokenization.

On Windows, java_binary is launched by a native
C++ binary. Therefore nothing could Bash-tokenize
the flags until now. From now on, Bazel itself
Bash-tokenizes the flags before embedding them in
the launcher. The launcher then escapes these
flags for CreateProcessW.

This PR also adds a new, CreateProcessW-aware
escaping method called CreateProcessEscapeArg, to
the launcher.

The pre-existing GetEscapedArgument escaped only
with Bash semantics in mind. This method is now
renamed to BashEscapeArg, and used only for the
Bash launcher. For the Python and Java launcher,
the new CreateProcessEscapeArg method is used.

Fixes bazelbuild#7072

Change-Id: I48d675fcce39e4f6c95e3bad3e8569f0274f662a

src/main/java/com/google/devtools/build/lib/BUILD

@@ -749,6 +749,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/rules/java:java-rules",
         "//src/main/java/com/google/devtools/build/lib/rules/objc",
         "//src/main/java/com/google/devtools/build/lib/rules/platform",
+        "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/apple",

src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java

@@ -43,6 +43,7 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.rules.cpp.CcInfo;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
@@ -66,6 +67,7 @@
 import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
 import com.google.devtools.build.lib.rules.java.JavaUtil;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
+import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.OS;
@@ -265,7 +267,7 @@ public Artifact createStubAction(
       List<String> jvmFlags,
       Artifact executable,
       String javaStartClass,
-      String javaExecutable) {
+      String javaExecutable) throws RuleErrorException {
     return createStubAction(
         ruleContext,
         javaCommon,
@@ -288,7 +290,7 @@ public Artifact createStubAction(
       String coverageStartClass,
       NestedSetBuilder<Artifact> filesBuilder,
       String javaExecutable,
-      boolean createCoverageMetadataJar) {
+      boolean createCoverageMetadataJar) throws RuleErrorException {
     Preconditions.checkState(ruleContext.getConfiguration().hasFragment(JavaConfiguration.class));
 
     Preconditions.checkNotNull(jvmFlags);
@@ -399,12 +401,25 @@ public Artifact createStubAction(
     arguments.add(Substitution.ofSpaceSeparatedList("%jvm_flags%", jvmFlagsList));
 
     if (OS.getCurrent() == OS.WINDOWS) {
+      // Bash-tokenize the JVM flags. On Linux/macOS tokenization is achieved by embedding the
+      // 'jvmFlags' into the java_binary stub script (which is a Bash script) and letting Bash
+      // tokenize them.
+      List<String> tokenizedJvmFlags = new ArrayList<>(jvmFlagsList.size());
+      for (String s : jvmFlags) {
+        try {
+          ShellUtils.tokenize(tokenizedJvmFlags, s);
+        } catch (ShellUtils.TokenizationException e) {
+          ruleContext.attributeError(
+              "jvm_flags", "Could not Bash-tokenize \"" + s + "\": " + e.getMessage());
+          throw new RuleErrorException();
+        }
+      }
       return createWindowsExeLauncher(
           ruleContext,
           javaExecutable,
           classpath,
           javaStartClass,
-          jvmFlagsList,
+          tokenizedJvmFlags,
           executable);
     }
 
@@ -418,7 +433,7 @@ private static Artifact createWindowsExeLauncher(
       String javaExecutable,
       NestedSet<Artifact> classpath,
       String javaStartClass,
-      ImmutableList<String> jvmFlags,
+      List<String> bashTokenizedJvmFlags,
       Artifact javaLauncher) {
 
     LaunchInfo launchInfo =
@@ -440,7 +455,13 @@ private static Artifact createWindowsExeLauncher(
                 "classpath",
                 ";",
                 Iterables.transform(classpath, Artifact.ROOT_RELATIVE_PATH_STRING))
-            .addJoinedValues("jvm_flags", " ", jvmFlags)
+            // TODO(laszlocsomor): joining flags on \t means no flag may contain this character,
+            // otherwise the launcher would split it into two flags. This solution is good enough
+            // because flags typically don't contain tabs, but we can't rule out the possibility
+            // that they do.  Let's find a better way to embed the JVM flags to the launcher, e.g.
+            // by using one jvm_flags entry per flag instead of joining all flags as one jvm_flags
+            // value.
+            .addJoinedValues("jvm_flags", "\t", bashTokenizedJvmFlags)
             .build();
 
     LauncherFileWriteAction.createAndRegister(ruleContext, javaLauncher, launchInfo);

src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java

@@ -304,7 +304,7 @@ Artifact createStubAction(
       Artifact executable,
       String javaStartClass,
       String javaExecutable)
-      throws InterruptedException;
+      throws InterruptedException, RuleErrorException;
 
   /**
    * Same as {@link #createStubAction(RuleContext, JavaCommon, List, Artifact, String, String)}.
@@ -326,7 +326,7 @@ public Artifact createStubAction(
       NestedSetBuilder<Artifact> filesBuilder,
       String javaExecutable,
       boolean createCoverageMetadataJar)
-      throws InterruptedException;
+      throws InterruptedException, RuleErrorException;
 
   /**
    * Returns true if {@code createStubAction} considers {@code javaExecutable} as a substitution.

src/test/py/bazel/launcher_test.py

@@ -200,9 +200,10 @@ def _buildAndCheckArgumentPassing(self, package, target_name):
                                                           bin_suffix))))
 
     arguments = ['a', 'a b', '"b"', 'C:\\a\\b\\', '"C:\\a b\\c\\"']
+    parenthesized_arguments = ['(%s)' % a for a in arguments]
     exit_code, stdout, stderr = self.RunProgram([bin1] + arguments)
     self.AssertExitCode(exit_code, 0, stderr)
-    self.assertEqual(stdout, arguments)
+    self.assertEqual(stdout, parenthesized_arguments)
 
   def testJavaBinaryLauncher(self):
     self.ScratchFile('WORKSPACE')
@@ -248,7 +249,7 @@ def testJavaBinaryArgumentPassing(self):
         'public class Main {',
         '  public static void main(String[] args) {'
         '    for (String arg : args) {',
-        '      System.out.println(arg);',
+        '      System.out.printf("(%s)%n", arg);',
         '    }'
         '  }',
         '}',
@@ -316,7 +317,7 @@ def testShBinaryArgumentPassing(self):
         'N=${#args[@]}',
         '# Echo each argument',
         'for (( i=0;i<$N;i++)); do',
-        ' echo ${args[${i}]}',
+        ' echo "(${args[${i}]})"',
         'done',
     ])
     os.chmod(foo_sh, stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
@@ -389,7 +390,7 @@ def testPyBinaryArgumentPassing(self):
     self.ScratchFile('foo/bin.py', [
         'import sys',
         'for arg in sys.argv[1:]:',
-        '  print(arg)',
+        '  print("(%s)" % arg)',
     ])
 
     self._buildAndCheckArgumentPassing('foo', 'bin')
@@ -651,6 +652,45 @@ def AssertRunfilesManifestContains(self, manifest, entry):
           return
     self.fail('Runfiles manifest "%s" did not contain "%s"' % (manifest, entry))
 
+  def testJvmFlagsFromBuildFile(self):
+    self.ScratchFile('WORKSPACE')
+    self.ScratchFile('BUILD', ["""
+java_binary(
+    name = "foo",
+    srcs = ["Main.java"],
+    main_class = "Main",
+    jvm_flags = [
+        "-Darg0=a",
+        "-Darg1=\"b\\\"\"\\ c",
+        "-Darg2='d\\\"'\\ e",
+        "-Darg3=f\"\\\\\"\\ '\\\\\"\\\"g '",
+    ],
+)"""
+        ])
+    self.ScratchFile('Main.java', ["""
+public class Main {
+  public static void main(String[] args) {
+    for (int i = 0; i < 4; ++i) {
+      System.out.printf("arg%d=(%s)%n", i, System.getProperty("arg" + i));
+    }
+  }
+}"""])
+    exit_code, stdout, stderr = self.RunBazel(['info', 'bazel-bin'])
+    self.AssertExitCode(exit_code, 0, stderr)
+    bazel_bin = stdout[0]
+
+    exit_code, _, stderr = self.RunBazel(['build', '//:foo'])
+    self.AssertExitCode(exit_code, 0, stderr)
+
+    if self.IsWindows():
+      foo_path = os.path.abspath(os.path.join(bazel_bin, 'foo.exe'))
+    else:
+      foo_path = os.path.abspath(os.path.join(bazel_bin, 'foo'))
+    exit_code, stdout, stderr = self.RunProgram([foo_path])
+    self.AssertExitCode(exit_code, 0, stderr)
+    self.assertListEqual(['arg0=(a)', 'arg1=(b" c)', 'arg2=(d\\" e)',
+                          'arg3=(f\\ \\\\"\\"g )'], stdout)
+
 
 if __name__ == '__main__':
   unittest.main()

src/tools/launcher/bash_launcher.cc

@@ -45,18 +45,15 @@ ExitCode BashBinaryLauncher::Launch() {
   vector<wstring> origin_args = this->GetCommandlineArguments();
   wostringstream bash_command;
   wstring bash_main_file = GetBinaryPathWithoutExtension(origin_args[0]);
-  bash_command << GetEscapedArgument(bash_main_file,
-                                     /*escape_backslash = */ true);
+  bash_command << BashEscapeArg(bash_main_file);
   for (int i = 1; i < origin_args.size(); i++) {
     bash_command << L' ';
-    bash_command << GetEscapedArgument(origin_args[i],
-                                       /*escape_backslash = */ true);
+    bash_command << BashEscapeArg(origin_args[i]);
   }
 
   vector<wstring> args;
   args.push_back(L"-c");
-  args.push_back(
-      GetEscapedArgument(bash_command.str(), /*escape_backslash = */ true));
+  args.push_back(BashEscapeArg(bash_command.str()));
   return this->LaunchProcess(bash_binary, args);
 }
 

src/tools/launcher/java_launcher.cc

@@ -359,7 +359,7 @@ ExitCode JavaBinaryLauncher::Launch() {
     jvm_flags.push_back(flag);
   }
   wstringstream jvm_flags_launch_info_ss(this->GetLaunchInfoByKey(JVM_FLAGS));
-  while (getline(jvm_flags_launch_info_ss, flag, L' ')) {
+  while (getline(jvm_flags_launch_info_ss, flag, L'\t')) {
     jvm_flags.push_back(flag);
   }
 
@@ -411,8 +411,7 @@ ExitCode JavaBinaryLauncher::Launch() {
   vector<wstring> escaped_arguments;
   // Quote the arguments if having spaces
   for (const auto& arg : arguments) {
-    escaped_arguments.push_back(
-        GetEscapedArgument(arg, /*escape_backslash = */ false));
+    escaped_arguments.push_back(CreateProcessEscapeArg(arg));
   }
 
   ExitCode exit_code = this->LaunchProcess(java_bin, escaped_arguments);

src/tools/launcher/python_launcher.cc

@@ -49,7 +49,7 @@ ExitCode PythonBinaryLauncher::Launch() {
 
   // Escape arguments that has spaces
   for (int i = 1; i < args.size(); i++) {
-    args[i] = GetEscapedArgument(args[i], /*escape_backslash = */ false);
+    args[i] = CreateProcessEscapeArg(args[i]);
   }
 
   return this->LaunchProcess(python_binary, args);

src/tools/launcher/util/launcher_util.cc

@@ -128,7 +128,7 @@ wstring GetBinaryPathWithExtension(const wstring& binary) {
   return GetBinaryPathWithoutExtension(binary) + L".exe";
 }
 
-wstring GetEscapedArgument(const wstring& argument, bool escape_backslash) {
+wstring BashEscapeArg(const wstring& argument) {
   wstring escaped_arg;
   // escaped_arg will be at least this long
   escaped_arg.reserve(argument.size());
@@ -150,8 +150,7 @@ wstring GetEscapedArgument(const wstring& argument, bool escape_backslash) {
         break;
 
       case L'\\':
-        // Escape back slashes if escape_backslash is true
-        escaped_arg += (escape_backslash ? L"\\\\" : L"\\");
+        escaped_arg += L"\\\\";
         break;
 
       default:
@@ -165,6 +164,87 @@ wstring GetEscapedArgument(const wstring& argument, bool escape_backslash) {
   return escaped_arg;
 }
 
+std::wstring CreateProcessEscapeArg(const std::wstring& s) {
+  if (s.empty()) {
+    return L"\"\"";
+  } else {
+    bool needs_escaping = false;
+    for (const auto& c : s) {
+      if (c == ' ' || c == '\\' || c == '"') {
+        needs_escaping = true;
+        break;
+      }
+    }
+    if (!needs_escaping) {
+      return s;
+    }
+  }
+
+  std::wostringstream result;
+  result << L'"';
+  int start = 0;
+  for (int i = 0; i < s.size(); ++i) {
+    char c = s[i];
+    if (c == '"' || c == '\\') {
+      // Copy the segment since the last special character.
+      if (start >= 0) {
+        result << s.substr(start, i - start);
+        start = -1;
+      }
+
+      // Handle the current special character.
+      if (c == '"') {
+        // This is a quote character. Escape it with a single backslash.
+        result << L"\\\"";
+      } else {
+        // This is a backslash (or the first one in a run of backslashes).
+        // Whether we escape it depends on whether the run ends with a quote.
+        int run_len = 1;
+        int j = i + 1;
+        while (j < s.size() && s[j] == '\\') {
+          run_len++;
+          j++;
+        }
+        if (j == s.size()) {
+          // The run of backslashes goes to the end.
+          // We have to escape every backslash with another backslash.
+          for (int k = 0; k < run_len * 2; ++k) {
+            result << L'\\';
+          }
+          break;
+        } else if (j < s.size() && s[j] == '"') {
+          // The run of backslashes is terminated by a quote.
+          // We have to escape every backslash with another backslash, and
+          // escape the quote with one backslash.
+          for (int k = 0; k < run_len * 2; ++k) {
+            result << L'\\';
+          }
+          result << L"\\\"";
+          i += run_len;  // 'i' is also increased in the loop iteration step
+        } else {
+          // No quote found. Each backslash counts for itself, they must not be
+          // escaped.
+          for (int k = 0; k < run_len; ++k) {
+            result << L'\\';
+          }
+          i += run_len - 1;  // 'i' is also increased in the loop iteration step
+        }
+      }
+    } else {
+      // This is not a special character. Start the segment if necessary.
+      if (start < 0) {
+        start = i;
+      }
+    }
+  }
+  // Save final segment after the last special character.
+  if (start != -1) {
+    result << s.substr(start);
+  }
+  result << L'"';
+  return result.str();
+}
+
 // An environment variable has a maximum size limit of 32,767 characters
 // https://msdn.microsoft.com/en-us/library/ms683188.aspx
 static const int BUFFER_SIZE = 32767;

src/tools/launcher/util/launcher_util.h

@@ -44,10 +44,11 @@ std::wstring GetBinaryPathWithExtension(const std::wstring& binary);
 // Escape a command line argument.
 //
 // If the argument has space, then we quote it.
-// Escape " to \"
-// Escape \ to \\ if escape_backslash is true
-std::wstring GetEscapedArgument(const std::wstring& argument,
-                                bool escape_backslash);
+// Escape " to \" .
+// Escape \ to \\ .
+std::wstring BashEscapeArg(const std::wstring& argument);
+
+std::wstring CreateProcessEscapeArg(const std::wstring& arg);
 
 // Convert a path to an absolute Windows path with \\?\ prefix.
 // This method will print an error and exit if it cannot convert the path.