Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

run_shell quoting broken on Windows #7122

Closed
mboes opened this issue Jan 15, 2019 · 11 comments
Closed

run_shell quoting broken on Windows #7122

mboes opened this issue Jan 15, 2019 · 11 comments
Assignees
Labels
area-Windows Windows-specific issues and feature requests P1 I'll work on this now. (Assignee required) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug

Comments

@mboes
Copy link
Contributor

mboes commented Jan 15, 2019

Description of the problem

Quoting of the command in ctx.actions.run_shell() does not work the same way on Windows and on Unix.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Consider the following BUILD file:

load(":args_test.bzl", "args_test")
args_test(name = "y")

with args_test.bzl:

def _impl(ctx):
    foo = ctx.actions.declare_file("foo")
    ctx.actions.run_shell(
        outputs = [foo],
        command = '${1+"$@"}',
        arguments = ['echo', 'a', 'b', 'c'],
    )
    return [DefaultInfo(executable = foo)]

args_test = rule(
    implementation = _impl,
    test = True,
)

On Linux, we get "a b c" printed on the console, before Bazel complains that executing the command did not create the file "foo". But on Windows, Bash complains that program '$' does not exist. Clearly, the quoting of the command doesn't work the same way.

Now for something even more odd. If instead of ${1+"$@"}, the command becomes ${1+"$@"} (mind the extra whitespace character), then the Windows behaviour matches the Linux one. That is, it takes there being at least one whitespace character in the command for the quoting to kick in properly. Now, it's very hard to debug these quoting issues because quotes are never displayed in the --verbose_failures output (not pasted here because hard to extract from my Windows VM).

More bizarre still, say the arguments read:

        arguments = ['echo', 'a', 'b"123"', 'c'],

Then the output is as follows on Linux:

a b"123" c

but the following on Windows:

a b\123" c

That is to say, the first quote gets replaced with a backslash...

The same odd behaviour can also be seen when rerouting the output of the command to a file (e.g. foo).

What operating system are you running Bazel on?

Linux NixOS 18.09 and Windows Server 2016

What's the output of bazel info release?

0.20.0

@gdeest
Copy link

gdeest commented Jan 15, 2019

Confirmed on Bazel 0.21.

bash.exe"-3.1$ ../bazel-0.21.0-windows-x86_64.exe build ...
Starting local Bazel server and connecting to it...
INFO: Invocation ID: f4135611-0c23-42cd-a339-729892e69d29
INFO: Analysed target //:y (13 packages loaded, 234 targets configured).
INFO: Found 1 target...
ERROR: C:/users/user/repro/BUILD:2:1: error executing shell command: 'C:/Users/user/scoop/apps/msys-rev13/current/bin/bash.exe -c ${1+"$@"}  echo a b c' failed (Exit 127)
"C:/Users/user/scoop/apps/msys-rev13/current/bin/bash.exe": $: command not found
Target //:y failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 58.901s, Critical Path: 0.10s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

@laurentlb
Copy link
Contributor

cc @laszlocsomor

@laurentlb laurentlb added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug labels Jan 17, 2019
@laszlocsomor laszlocsomor self-assigned this Jan 17, 2019
@laszlocsomor laszlocsomor added area-Windows Windows-specific issues and feature requests and removed team-Starlark untriaged labels Jan 17, 2019
@laszlocsomor
Copy link
Contributor

My unconfirmed suspicion is a bug in the Windows process creation logic.

@laszlocsomor
Copy link
Contributor

Reason for this suspicion is my recent discovery about the broken escaping of CreateProcess arguments, while I was debugging #7072.

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Jan 17, 2019

@gdeest and @mboes , could it be that you are using MSYS1 or some custom Bash, instead of using MSYS2 64-bit? Do you see the same behavior if you install MSYS2 (https://www.msys2.org/) to c:\tools\msys64, set BAZEL_SH=c:\tools\msys64\usr\bin\bash.exe, run bazel shutdown, then try the build again?

@gdeest
Copy link

gdeest commented Jan 17, 2019

@laszlocsomor I am indeed using MSYS1 (because MSYS2 is not compatible with Docker, which is required for CI). AFAIK, @mboes is using MSYS2 64-bit.

I'll try with MSYS2 and report with the results.

@laszlocsomor
Copy link
Contributor

@gdeest : did you get a chance to try with MSYS2?

@mboes
Copy link
Contributor Author

mboes commented Jan 26, 2019

I am using MSYS2. Still seeing the bug there.

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Feb 8, 2019
Fix the quoting logic for command line arguments
passed to CreateProcessW.

Related to bazelbuild#7314

Fixes bazelbuild#7122
@laszlocsomor
Copy link
Contributor

This is a plain old Bazel bug. There was even a TODO to fix it... I have a fix pending.

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Feb 13, 2019
Add an integration test to assert that
subprocesses created with WindowsSubprocessFactory
receive arguments as intended.

Next steps:
- implement the same argument escaping logic in
  Bazel as windowsEscapeArg2 in bazelbuild#7411
- replace WindowsProcesses.quoteCommandLine with
  the new escaper

See bazelbuild#7122
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Feb 13, 2019
Add an integration test to assert that
subprocesses created with WindowsSubprocessFactory
receive arguments as intended.

Next steps:
- implement the same argument escaping logic in
  Bazel as windowsEscapeArg2 in bazelbuild#7411
- replace WindowsProcesses.quoteCommandLine with
  the new escaper

See bazelbuild#7122
@laszlocsomor laszlocsomor added P1 I'll work on this now. (Assignee required) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Feb 13, 2019
@laszlocsomor
Copy link
Contributor

I'm raising the priority, because the underlying bug is that argument escaping is similarly broken for all subprocesses, not just for run_shell.

bazel-io pushed a commit that referenced this issue Feb 13, 2019
Add an integration test to assert that
subprocesses created with WindowsSubprocessFactory
receive arguments as intended.

Next steps:
- implement the same argument escaping logic in
  Bazel as windowsEscapeArg2 in #7411
- replace WindowsProcesses.quoteCommandLine with
  the new escaper

See #7122

Closes #7413.

PiperOrigin-RevId: 233730449
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Feb 13, 2019
Add correct implementation of flag escaping for
subprocesses.

This is a follow-up to bazelbuild#7413

Next steps:
- replace WindowsProcesses.quoteCommandLine with
  the new logic

See bazelbuild#7122
bazel-io pushed a commit that referenced this issue Feb 14, 2019
Add correct implementation of flag escaping for
subprocesses.

This is a follow-up to #7413

Next steps:
- replace WindowsProcesses.quoteCommandLine with
  the new logic

See #7122

Closes #7420.

PiperOrigin-RevId: 233900149
bazel-io pushed a commit that referenced this issue Feb 15, 2019
Individual SubprocessBuilder instances can now use
a SubprocessFactory object other than the static
SubprocessBuilder.factory.

Also, WindowsSubprocessFactory is no longer a
singleton because it now stores state: whether to
use windows-style argument escaping or to use the
(broken) Bash-style escaping (causing #7122).

These two changes allow:
- a safer way to mock out SubprocessFactory in
  tests, because it's no longer necessary to
  change global state (i.e. the static
  SubprocessBuilder.factory member)
- testing old and new argument escaping semantics
  in WindowsSubprocessTest
- adding a flag that switches between old and new
  semantics in the SubprocessFactory, and thus
  allows rolling out the bugfix with just a flag
  flip

See #7122

PiperOrigin-RevId: 234105692
bazel-io pushed a commit that referenced this issue Feb 19, 2019
Add the --incompatible_windows_style_arg_escaping
flag (default: false). This flag has no effect on
platforms other than Windows.

Semantics:
- When true: subprocess arguments are escaped with
  ShellUtils.windowsEscapeArg. This is the correct
  behavior.
- When false: subprocess arguments are escaped
  with ShellUtils.quoteCommandLine. This is the
  default behavior on all platforms, but it is
  incorrect on Windows.

Incompatible flag: #7454

Related: #7122

RELNOTES[NEW]: Added --incompatible_windows_style_arg_escaping flag: enables correct subprocess argument escaping on Windows. (No-op on other platforms.)

PiperOrigin-RevId: 234581069
@laszlocsomor
Copy link
Contributor

Closing this, because it's fixed at HEAD, see #7454. That bug will be closed when the flag's default value is flipped to true at HEAD.

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 11, 2019
In this PR:

- Removed the
  `--incompatible_windows_style_arg_escaping` flag
  and all code for its "false" case.  The flag is
  flipped to true.

- In WindowsProcessesTest, the
  testDoesNotQuoteArgWithDoubleQuote method is
  replaced by testQuotesArgWithDoubleQuote, i.e.
  the test logic is reversed. The new test shows
  the correct behavior, the old test was wrong.

See bazelbuild#7122
See bazelbuild#7454

RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 18, 2019
In this PR:

- Removed the
  `--incompatible_windows_style_arg_escaping` flag
  and all code for its "false" case.  The flag is
  flipped to true.

- In WindowsProcessesTest, the
  testDoesNotQuoteArgWithDoubleQuote method is
  replaced by testQuotesArgWithDoubleQuote, i.e.
  the test logic is reversed. The new test shows
  the correct behavior, the old test was wrong.

See bazelbuild#7122
See bazelbuild#7454

RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.
bazel-io pushed a commit that referenced this issue Jul 1, 2019
In this PR:

- Removed the
  `--incompatible_windows_style_arg_escaping` flag
  and all code for its "false" case.  The flag is
  flipped to true.

- In WindowsProcessesTest, the
  testDoesNotQuoteArgWithDoubleQuote method is
  replaced by testQuotesArgWithDoubleQuote, i.e.
  the test logic is reversed. The new test shows
  the correct behavior, the old test was wrong.

See #7122
See #7454

RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.

Closes #8003.

PiperOrigin-RevId: 255929367
siberex pushed a commit to siberex/bazel that referenced this issue Jul 4, 2019
In this PR:

- Removed the
  `--incompatible_windows_style_arg_escaping` flag
  and all code for its "false" case.  The flag is
  flipped to true.

- In WindowsProcessesTest, the
  testDoesNotQuoteArgWithDoubleQuote method is
  replaced by testQuotesArgWithDoubleQuote, i.e.
  the test logic is reversed. The new test shows
  the correct behavior, the old test was wrong.

See bazelbuild#7122
See bazelbuild#7454

RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.

Closes bazelbuild#8003.

PiperOrigin-RevId: 255929367
irengrig pushed a commit to irengrig/bazel that referenced this issue Jul 15, 2019
In this PR:

- Removed the
  `--incompatible_windows_style_arg_escaping` flag
  and all code for its "false" case.  The flag is
  flipped to true.

- In WindowsProcessesTest, the
  testDoesNotQuoteArgWithDoubleQuote method is
  replaced by testQuotesArgWithDoubleQuote, i.e.
  the test logic is reversed. The new test shows
  the correct behavior, the old test was wrong.

See bazelbuild#7122
See bazelbuild#7454

RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.

Closes bazelbuild#8003.

PiperOrigin-RevId: 255929367
@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    Individual SubprocessBuilder instances can now use
    a SubprocessFactory object other than the static
    SubprocessBuilder.factory.

    Also, WindowsSubprocessFactory is no longer a
    singleton because it now stores state: whether to
    use windows-style argument escaping or to use the
    (broken) Bash-style escaping (causing bazelbuild/bazel#7122).

    These two changes allow:
    - a safer way to mock out SubprocessFactory in
      tests, because it's no longer necessary to
      change global state (i.e. the static
      SubprocessBuilder.factory member)
    - testing old and new argument escaping semantics
      in WindowsSubprocessTest
    - adding a flag that switches between old and new
      semantics in the SubprocessFactory, and thus
      allows rolling out the bugfix with just a flag
      flip

    See bazelbuild/bazel#7122

    PiperOrigin-RevId: 234105692
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    Add correct implementation of flag escaping for
    subprocesses.

    This is a follow-up to bazelbuild/bazel#7413

    Next steps:
    - replace WindowsProcesses.quoteCommandLine with
      the new logic

    See bazelbuild/bazel#7122

    Closes #7420.

    PiperOrigin-RevId: 233900149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests P1 I'll work on this now. (Assignee required) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug
Projects
None yet
Development

No branches or pull requests

6 participants