Windows Launcher: Fix argument passing

Make sure the actual binary recieves exactly the same argument passed to Windows exe launcher. Fixed #4001 Change-Id: I5db2d7c2f78de8865abc04a2d5b65d69685d43db PiperOrigin-RevId: 178610493
bazelbuild · Dec 11, 2017 · 373dcf9 · 373dcf9
1 parent 30f6d44
commit 373dcf9
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 39 deletions.
diff --git a/src/test/py/bazel/launcher_test.py b/src/test/py/bazel/launcher_test.py
@@ -188,6 +188,30 @@ def _buildPyTargets(self, bazel_bin, launcher_flag, binary_suffix):
     exit_code, _, stderr = self.RunBazel(['test', '//foo:test'] + launcher_flag)
     self.AssertExitCode(exit_code, 0, stderr)
 
+  def _buildAndCheckArgumentPassing(self, package, target_name):
+    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', '--windows_exe_launcher=1',
+        '//%s:%s' % (package, target_name)
+    ])
+    self.AssertExitCode(exit_code, 0, stderr)
+
+    bin_suffix = '.exe' if self.IsWindows() else ''
+    bin1 = os.path.join(bazel_bin, package, '%s%s' % (target_name, bin_suffix))
+    self.assertTrue(os.path.exists(bin1))
+    self.assertTrue(
+        os.path.isdir(
+            os.path.join(bazel_bin, '%s/%s%s.runfiles' % (package, target_name,
+                                                          bin_suffix))))
+
+    arguments = ['a', 'a b', '"b"', 'C:\\a\\b\\', '"C:\\a b\\c\\"']
+    exit_code, stdout, stderr = self.RunProgram([bin1] + arguments)
+    self.AssertExitCode(exit_code, 0, stderr)
+    self.assertEqual(stdout, arguments)
+
   def testJavaBinaryLauncher(self):
     self.ScratchFile('WORKSPACE')
     self.ScratchFile('foo/BUILD', [
@@ -223,6 +247,27 @@ def testJavaBinaryLauncher(self):
     self._buildJavaTargets(bazel_bin, ['--windows_exe_launcher=1'], '.exe'
                            if self.IsWindows() else '')
 
+  def testJavaBinaryArgumentPassing(self):
+    self.ScratchFile('WORKSPACE')
+    self.ScratchFile('foo/BUILD', [
+        'java_binary(',
+        '  name = "bin",',
+        '  srcs = ["Main.java"],',
+        '  main_class = "Main",',
+        ')',
+    ])
+    self.ScratchFile('foo/Main.java', [
+        'public class Main {',
+        '  public static void main(String[] args) {'
+        '    for (String arg : args) {',
+        '      System.out.println(arg);',
+        '    }'
+        '  }',
+        '}',
+    ])
+
+    self._buildAndCheckArgumentPassing('foo', 'bin')
+
   def testShBinaryLauncher(self):
     self.ScratchFile('WORKSPACE')
     self.ScratchFile(
@@ -294,26 +339,7 @@ def testShBinaryArgumentPassing(self):
     ])
     os.chmod(foo_sh, stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
 
-    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', '--windows_exe_launcher=1', '//foo:bin'])
-    self.AssertExitCode(exit_code, 0, stderr)
-
-    bin_suffix = '.exe' if self.IsWindows() else ''
-
-    bin1 = os.path.join(bazel_bin, 'foo', 'bin%s' % bin_suffix)
-    self.assertTrue(os.path.exists(bin1))
-    self.assertTrue(
-        os.path.isdir(
-            os.path.join(bazel_bin, 'foo/bin%s.runfiles' % bin_suffix)))
-
-    arguments = ['a', 'a b', '"b"', 'C:\\a\\b\\', '"C:\\a b\\c\\"']
-    exit_code, stdout, stderr = self.RunProgram([bin1] + arguments)
-    self.AssertExitCode(exit_code, 0, stderr)
-    self.assertEqual(stdout, arguments)
+    self._buildAndCheckArgumentPassing('foo', 'bin')
 
   def testPyBinaryLauncher(self):
     self.ScratchFile('WORKSPACE')
@@ -376,6 +402,22 @@ def testPyBinaryLauncher(self):
     self._buildPyTargets(bazel_bin, ['--windows_exe_launcher=1'], '.exe'
                          if self.IsWindows() else '')
 
+  def testPyBinaryArgumentPassing(self):
+    self.ScratchFile('WORKSPACE')
+    self.ScratchFile('foo/BUILD', [
+        'py_binary(',
+        '  name = "bin",',
+        '  srcs = ["bin.py"],',
+        ')',
+    ])
+    self.ScratchFile('foo/bin.py', [
+        'import sys',
+        'for arg in sys.argv[1:]:',
+        '  print(arg)',
+    ])
+
+    self._buildAndCheckArgumentPassing('foo', 'bin')
+
   def testWindowsJavaExeLauncher(self):
     # Skip this test on non-Windows platforms
     if not self.IsWindows():
@@ -452,7 +494,7 @@ def testWindowsJavaExeLauncher(self):
     exit_code, stdout, stderr = self.RunProgram(
         [binary, '--jvm_flag="--some_path="./a b/c""', print_cmd])
     self.AssertExitCode(exit_code, 0, stderr)
-    self.assertIn('--some_path="./a b/c"', stdout)
+    self.assertIn('"--some_path=\\"./a b/c\\""', stdout)
 
     exit_code, stdout, stderr = self.RunProgram(
         [binary, '--jvm_flags="--path1=a --path2=b"', print_cmd])

diff --git a/src/tools/launcher/bash_launcher.cc b/src/tools/launcher/bash_launcher.cc
@@ -39,15 +39,18 @@ ExitCode BashBinaryLauncher::Launch() {
   vector<string> origin_args = this->GetCommandlineArguments();
   ostringstream bash_command;
   string bash_main_file = GetBinaryPathWithoutExtension(origin_args[0]);
-  bash_command << GetEscapedArgument(bash_main_file);
+  bash_command << GetEscapedArgument(bash_main_file,
+                                     /*escape_backslash = */ true);
   for (int i = 1; i < origin_args.size(); i++) {
     bash_command << ' ';
-    bash_command << GetEscapedArgument(origin_args[i]);
+    bash_command << GetEscapedArgument(origin_args[i],
+                                       /*escape_backslash = */ true);
   }
 
   vector<string> args;
   args.push_back("-c");
-  args.push_back(bash_command.str());
+  args.push_back(
+      GetEscapedArgument(bash_command.str(), /*escape_backslash = */ true));
   return this->LaunchProcess(bash_binary, args);
 }
 

diff --git a/src/tools/launcher/java_launcher.cc b/src/tools/launcher/java_launcher.cc
@@ -296,7 +296,14 @@ ExitCode JavaBinaryLauncher::Launch() {
     arguments.push_back(arg);
   }
 
-  ExitCode exit_code = this->LaunchProcess(java_bin, arguments);
+  vector<string> escaped_arguments;
+  // Quote the arguments if having spaces
+  for (const auto& arg : arguments) {
+    escaped_arguments.push_back(
+        GetEscapedArgument(arg, /*escape_backslash = */ false));
+  }
+
+  ExitCode exit_code = this->LaunchProcess(java_bin, escaped_arguments);
 
   // Delete classpath jar file after execution.
   if (!classpath_jar.empty()) {

diff --git a/src/tools/launcher/launcher.cc b/src/tools/launcher/launcher.cc
@@ -125,9 +125,8 @@ void BinaryLauncherBase::CreateCommandLine(
     const vector<string>& arguments) const {
   ostringstream cmdline;
   cmdline << '\"' << executable << '\"';
-  bool first = true;
   for (const auto& s : arguments) {
-    cmdline << ' ' << GetEscapedArgument(s);
+    cmdline << ' ' << s;
   }
 
   string cmdline_str = cmdline.str();

diff --git a/src/tools/launcher/launcher.h b/src/tools/launcher/launcher.h
@@ -64,6 +64,7 @@ class BinaryLauncherBase {
   // exectuable: the binary to be executed.
   // arguments:  the command line arguments to be passed to the exectuable,
   //             it doesn't include the exectuable itself.
+  //             The arguments are expected to be quoted if having spaces.
   ExitCode LaunchProcess(const std::string& executable,
                          const std::vector<std::string>& arguments) const;
 

diff --git a/src/tools/launcher/python_launcher.cc b/src/tools/launcher/python_launcher.cc
@@ -39,6 +39,12 @@ ExitCode PythonBinaryLauncher::Launch() {
 
   // Replace the first argument with python zip file path
   args[0] = python_zip_file;
+
+  // Escape arguments that has spaces
+  for (int i = 1; i < args.size(); i++) {
+    args[i] = GetEscapedArgument(args[i], /*escape_backslash = */ false);
+  }
+
   return this->LaunchProcess(python_binary, args);
 }
 

diff --git a/src/tools/launcher/util/launcher_util.cc b/src/tools/launcher/util/launcher_util.cc
@@ -108,7 +108,7 @@ string GetBinaryPathWithExtension(const string& binary) {
   return GetBinaryPathWithoutExtension(binary) + ".exe";
 }
 
-string GetEscapedArgument(const string& argument) {
+string GetEscapedArgument(const string& argument, bool escape_backslash) {
   ostringstream escaped_arg;
   bool has_space = argument.find_first_of(' ') != string::npos;
 
@@ -126,8 +126,8 @@ string GetEscapedArgument(const string& argument) {
         break;
 
       case '\\':
-        // Escape back slashes
-        escaped_arg << "\\\\";
+        // Escape back slashes if escape_backslash is true
+        escaped_arg << (escape_backslash ? "\\\\" : "\\");
         break;
 
       default:

diff --git a/src/tools/launcher/util/launcher_util.h b/src/tools/launcher/util/launcher_util.h
@@ -43,9 +43,10 @@ std::string GetBinaryPathWithExtension(const std::string& binary);
 // Escape a command line argument.
 //
 // If the argument has space, then we quote it.
-// Escape \ to \\
 // Escape " to \"
-std::string GetEscapedArgument(const std::string& argument);
+// Escape \ to \\ if escape_backslash is true
+std::string GetEscapedArgument(const std::string& argument,
+                               bool escape_backslash);
 
 // Convert a path to an absolute Windows path with \\?\ prefix.
 // This method will print an error and exit if it cannot convert the path.

diff --git a/src/tools/launcher/util/launcher_util_test.cc b/src/tools/launcher/util/launcher_util_test.cc
@@ -73,14 +73,19 @@ TEST_F(LaunchUtilTest, GetBinaryPathWithExtensionTest) {
 }
 
 TEST_F(LaunchUtilTest, GetEscapedArgumentTest) {
-  ASSERT_EQ("foo", GetEscapedArgument("foo"));
-  ASSERT_EQ("\"foo bar\"", GetEscapedArgument("foo bar"));
-  ASSERT_EQ("\"\\\"foo bar\\\"\"", GetEscapedArgument("\"foo bar\""));
-  ASSERT_EQ("foo\\\\bar", GetEscapedArgument("foo\\bar"));
-  ASSERT_EQ("foo\\\"bar", GetEscapedArgument("foo\"bar"));
-  ASSERT_EQ("C:\\\\foo\\\\bar\\\\", GetEscapedArgument("C:\\foo\\bar\\"));
+  ASSERT_EQ("foo", GetEscapedArgument("foo", true));
+  ASSERT_EQ("\"foo bar\"", GetEscapedArgument("foo bar", true));
+  ASSERT_EQ("\"\\\"foo bar\\\"\"", GetEscapedArgument("\"foo bar\"", true));
+  ASSERT_EQ("foo\\\\bar", GetEscapedArgument("foo\\bar", true));
+  ASSERT_EQ("foo\\\"bar", GetEscapedArgument("foo\"bar", true));
+  ASSERT_EQ("C:\\\\foo\\\\bar\\\\", GetEscapedArgument("C:\\foo\\bar\\", true));
   ASSERT_EQ("\"C:\\\\foo foo\\\\bar\\\\\"",
-            GetEscapedArgument("C:\\foo foo\\bar\\"));
+            GetEscapedArgument("C:\\foo foo\\bar\\", true));
+
+  ASSERT_EQ("foo\\bar", GetEscapedArgument("foo\\bar", false));
+  ASSERT_EQ("C:\\foo\\bar\\", GetEscapedArgument("C:\\foo\\bar\\", false));
+  ASSERT_EQ("\"C:\\foo foo\\bar\\\"",
+            GetEscapedArgument("C:\\foo foo\\bar\\", false));
 }
 
 TEST_F(LaunchUtilTest, DoesFilePathExistTest) {