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

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

@@ -751,6 +751,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')
@@ -409,7 +410,7 @@ def testWindowsJavaExeLauncher(self):
     ])
     self.ScratchFile('foo/Main.java', [
         'public class Main {',
-        '  public static void main(String[] args) {'
+        '  public static void main(String[] args) {',
         '    System.out.println("helloworld");',
         '  }',
         '}',
@@ -502,7 +503,7 @@ def testWindowsJavaExeLauncher(self):
     self.AssertExitCode(exit_code, 0, stderr)
     self.assertIn('local_jdk/bin/java.exe', ''.join(stdout))
 
-    my_tmp_dir = self.ScratchDir('my/temp/dir')
+    my_tmp_dir = self.ScratchDir('my/temp/dir').replace('\\', '/')
     exit_code, stdout, stderr = self.RunProgram(
         [binary, print_cmd], env_add={'TEST_TMPDIR': my_tmp_dir})
     self.AssertExitCode(exit_code, 0, stderr)
@@ -651,6 +652,97 @@ 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', [r"""
+java_binary(
+    name = "foo",
+    srcs = ["Main.java"],
+    main_class = "Main",
+    jvm_flags = [
+        '-Darg0=A',
+        '-Darg1="A"',
+        '-Darg2=\'"a"\'',
+        '-Darg3=\'B\'',
+        '-Darg4="\'b\'"',
+        '-Darg5="\\"b\\""',
+        '-Darg6=\'D E\'',
+        '-Darg7=\'"d e"\'',
+        '-Darg8=\'F"G\'',
+        '-Darg9=\'"F"G"\'',
+        '-Darg10=\'C:\\H I\'',
+        '-Darg11=\'"C:\\h i"\'',
+        '-Darg12=\'C:\\J"K\'',
+        '-Darg13=\'"C:\\j"k"\'',
+        '-Darg14=\'C:\\L\\"M\'',
+        '-Darg15=\'"C:\\l\\"m"\'',
+        '-Darg16=\'C:\\N O \'',
+        '-Darg17=\'"C:\\n o "\'',
+        '-Darg18=\'C:\\P Q\\\'',
+        '-Darg19=\'"C:\\p q\\"\'',
+        '-Darg20=\'C:\\R S\\ \'',
+        '-Darg21=\'"C:\\r s\\ "\'',
+        '-Darg22=\'C:\\T\\U\\\'',
+        '-Darg23=\'"C:\\t\\u\\"\'',
+        '-Darg24=\'C:\\V W\\X\\\'',
+        '-Darg25=\'"C:\\v w\\x\\"\'',
+    ],
+)"""])
+    self.ScratchFile('Main.java', ["""
+public class Main {
+  public static void main(String[] args) {
+    for (int i = 0; ; ++i) {
+      String value = System.getProperty("arg" + i);
+      if (value == null) {
+        break;
+      } else {
+        System.out.printf("arg%d=(%s)%n", i, value);
+      }
+    }
+  }
+}"""])
+    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=(A)",
+        "arg2=(\"a\")",
+        "arg3=(B)",
+        "arg4=('b')",
+        "arg5=(\"b\")",
+        "arg6=(D E)",
+        "arg7=(\"d e\")",
+        "arg8=(F\"G)",
+        "arg9=(\"F\"G\")",
+        "arg10=(C:\\H I)",
+        "arg11=(\"C:\\h i\")",
+        "arg12=(C:\\J\"K)",
+        "arg13=(\"C:\\j\"k\")",
+        "arg14=(C:\\L\\\"M)",
+        "arg15=(\"C:\\l\\\"m\")",
+        "arg16=(C:\\N O )",
+        "arg17=(\"C:\\n o \")",
+        "arg18=(C:\\P Q\\)",
+        "arg19=(\"C:\\p q\\\")",
+        "arg20=(C:\\R S\\ )",
+        "arg21=(\"C:\\r s\\ \")",
+        "arg22=(C:\\T\\U\\)",
+        "arg23=(\"C:\\t\\u\\\")",
+        "arg24=(C:\\V W\\X\\)",
+        "arg25=(\"C:\\v w\\x\\\")"], 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);