sh_binary launcher does not quote arguments correctly on Windows
- Dominant language
- Java
- Stars
- 25.8k
- Forks
- 4.6k
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 72
Description
### Description of the bug:
When using the `sh_binary` rule to invoke a _bash_ program on Windows, the `BashBinaryLauncher` C++ class used to invoke _bash_ [composes the command-line arguments](https://cs.opensource.google/bazel/bazel/+/refs/tags/6.0.0:src/tools/launcher/bash_launcher.cc;l=61-64) incorrectly, [quoting only those arguments that include at least one space character and escaping only double quotation and backslash characters](https://cs.opensource.google/bazel/bazel/+/refs/tags/6.0.0:src/tools/launcher/util/launcher_util.cc;l=153-182). The launcher fails to quote arguments that include other characters that _bash_ winds up misinterpreting.
In short, even when the `sh_binary` author quotes the command-line arguments as necessary to satisfy _bash_, the `BashBinaryLauncher` program consumes those arguments but then fails to emit them again in a manner that will satisfy _bash_.
### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
Find [attached here an archive of a Bazel repository](https://github.com/bazelbuild/bazel/files/10734992/bazel-sh-binary-test.tar.gz) named _bazel-sh-binary-test_. After extracting that archive, run the following command:
_bazel test //test:print_args_test_
This test compares the would-be printed output with expected "golden" output. This test succeeds on macOS and Linux.
When run on Windows, though, we never even make it as far as invoking the _bash_ program in file _test/print-args_. Instead, _bash_ fails while parsing the command-line arguments passed via the `-c` flag by the `BashBinaryLauncher` program.
In the `genrule` target named "capture_printed_args," we invoke the _print-args_ program as follows:
_print_args a 'b c' 'f(x).y'_
Note that the final argument is `f(x).y` surrounded by single quotation marks. Quoting the argument like that is necessary because the parentheses are shell metacharacters. We don't intend for _bash_ to interpret those parentheses; they're meant for the _print-args_ program to receive and consume.
Bazel's `genrule` implementation invokes the program created by the `sh_binary` rule as follows:
_.../bin/test/print_args.exe a 'b c' 'f(x).y' > "bazel-out/.../bin/test/printed-args.txt"_
Note that the second `b c` and third `f(x).y` arguments are both surrounded by single quotation marks. Next, the _print_args.exe_ program invokes _bash_ as follows:
_...\usr\bin\bash.exe -c '...\\bin\\test\\print_args a "b c" f(x).y'_
Note that while the second `b c` argument is surrounded with double quotation marks, the third `f(x).y` argument is not quoted at all. `BashBinaryLauncher` decided that that third argument didn't warrant quoting, but then _bash_ fails as follows:
> /usr/bin/bash: -c: line 1: syntax error near unexpected token `('
### Which operating system are you running Bazel on?
Windows
### What is the output of `bazel info release`?
release 6.0.0
### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.
_No response_
### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?
_No response_
### Have you found anything relevant by searching the web?
Somewhat related issues:
- #4778
- #7122
- #9106
- #9108
Somewhat related PRs:
- #9123
### Any other information, logs, or outputs that you want to share?
There have been a few discussions in the "Bazel" Slack workspace on this subject, [most recently focused on interpreting the failures](https://bazelbuild.slack.com/archives/C01SB78HS4T/p1676313550221839) with the test case supplied here. As part of that discussion, @fmeum submitted #17484 to probe the argument inspection and escaping done in [the `BashEscapeArg` function](https://cs.opensource.google/bazel/bazel/+/refs/tags/6.0.0:src/tools/launcher/util/launcher_util.cc;l=149-184).
The _bash_ source code offers [the `sh_contains_shell_metas` function](https://github.com/bminor/bash/blob/74091dd4e8086db518b30df7f222691524469998/lib/sh/shquote.c#L395-L410) that shows which characters _bash_ itself considers to warrant special treatment. Among other things, _bash_ uses that function to determine when to quote words when printing commands. Looking there, space characters are just one of twenty-five possible characters that may warrant quoting.
Without us being more conservative in which arguments we quote before passing on to the `sh_binary`-wrapped program, we can't pass many otherwise valid arguments through, leaving us unable to support invocations on Windows that work fine in other operating systems, even though we have _bash_ at our disposal.
Contributor guide
Research direction
Start with src/tools/launcher/bash_launcher.cc and src/tools/launcher/util/launcher_util.cc, then run bazel test //test:print_args_test in the attached minimal repository on Windows. Trace how BashBinaryLauncher composes the -c command and how BashEscapeArg handles metacharacters. Done means arguments such as f(x).y reach bash unchanged and the golden test passes on Windows without regressing macOS or Linux.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- bash, cpp
- Domain
- build-system, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100