Windows: backslashes passed to subprocess are unnecessarily unescaped.

[yilei]

Please provide the following information. The more we know about your system and use case, the more easily and likely we can help.

Description of the problem / feature request / question:

I am running the generated .exe file of a py_binary, which is a data deps of another py_test. The backslashes in the arguments passed to the .exe file end up unnecessarily unescaped in the py_binary.

If possible, provide a minimal example to reproduce the problem:

BUILD:

py_binary(
    name = "sub",
    srcs = ["sub.py"],
)
py_test(
    name = "main",
    srcs = ["main.py"],
    data = [":sub"],
)

main.py:

import os
import subprocess
import unittest
class MainTest(unittest.TestCase):
  def testMain(self):
    test_srcdir = os.getenv('TEST_SRCDIR')
    with open(os.path.join(test_srcdir, 'MANIFEST'), 'r') as f:
      for line in f:
        tokens = line.strip().split(' ')
        if len(tokens) == 2 and tokens[0] == '__main__/sub.exe':
          sub_path = tokens[1]
          break
    subprocess.check_call([sub_path, r'path\to\dir'])
if __name__ == '__main__':
  unittest.main()

sub.py:

import sys
if sys.argv[1] != r'path\to\dir':
  raise RuntimeError('Unexpected sys.argv[1]: {}\n'.format(sys.argv[1]))

bazel test :main fails, because sys.argv[1] is actually r'path\\to\\dir' instead of r'path\to\dir'.

Environment info

• Operating System:
  Windows Server 2016

• Bazel version (output of bazel info release):
  release 0.7.0

[laszlocsomor]

/cc @laszlocsomor

[yilei]

Any update on this?

[laszlocsomor]

I can take a look.

[meteorcloudy]

I think the problem is we always escape \ in this function:

bazel/src/tools/launcher/util/launcher_util.cc

Lines 111 to 142 in a5833a8

	string GetEscapedArgument(const string& argument) {
	ostringstream escaped_arg;
	bool has_space = argument.find_first_of(' ') != string::npos;
	if (has_space) {
	escaped_arg << '\"';
	}
	string::const_iterator it = argument.begin();
	while (it != argument.end()) {
	char ch = *it++;
	switch (ch) {
	case '"':
	// Escape double quotes
	escaped_arg << "\\\"";
	break;
	case '\\':
	// Escape back slashes
	escaped_arg << "\\\\";
	break;
	default:
	escaped_arg << ch;
	}
	}
	if (has_space) {
	escaped_arg << '\"';
	}
	return escaped_arg.str();
	}

But GetEscapedArgument is not only used in python launcher, but also the Java and Bash launcher. I remember at least in Bash launcher, we have to always escape \ for passing identical arguments.

[meteorcloudy]

We need a similar test for Python and Java

bazel/src/test/py/bazel/launcher_test.py

Lines 276 to 316 in a5833a8

	def testShBinaryArgumentPassing(self):
	self.ScratchFile('WORKSPACE')
	self.ScratchFile('foo/BUILD', [
	'sh_binary(',
	' name = "bin",',
	' srcs = ["bin.sh"],',
	')',
	])
	foo_sh = self.ScratchFile('foo/bin.sh', [
	'#!/bin/bash',
	'# Store arguments in a array',
	'args=("$@")',
	'# Get the number of arguments',
	'N=${#args[@]}',
	'# Echo each argument',
	'for (( i=0;i<$N;i++)); do',
	' echo ${args[${i}]}',
	'done',
	])
	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)

[laszlocsomor]

@meteorcloudy , I think we always have to follow Command Prompt escaping rules, not Bash escaping, because we pass the escaped arguments to CreateProcess.

I have a pending change that fixes this.

[meteorcloudy]

Right, but when we launch the exact binary
Bash: bash.exe -c "foo/bin.sh arg1 arg2"
Python: python.exe foo/bin.zip arg1 arg2(an executable python zip file)
So we have to escape arguments for bash.exe but not python.exe

Let's see if your change will break testShBinaryArgumentPassing or not.

[laszlocsomor]

You're right, my change did break testShBinaryArgumentPassing, among others.
I think we have to escape differently in the Bash and in the Java/Python launchers.

[laszlocsomor]

So we have to escape arguments for bash.exe but not python.exe

Ah, now I understand what you meant by this ;)

[meteorcloudy]

I think we have to escape differently in the Bash and in the Java/Python launchers.

I agree :)

[yilei]

Kindly ping?

[laszlocsomor]

I haven't been able to work on this lately.
@meteorcloudy , do you want to take over?

[meteorcloudy]

@yilei Sorry for the delay.
@laszlocsomor Sure, I can take over. ;)

[meteorcloudy]

Just sent a fix to review:
https://bazel-review.googlesource.com/#/c/bazel/+/26710/