Use raw_arg on Windows to fix handling of spaces in commands #2689

jhasse · 2025-03-31T22:18:41Z

On Windows Rust escapes arguments for security reasons, but since we are passing an argument to a shell (i.e. can execute arbitrary code anyway), this should be disabled in order to correctly support quoting and spaces in commands.

https://doc.rust-lang.org/std/process/index.html#windows-argument-splitting

Fixes #1639.

On Windows Rust escapes arguments for security reasons, but since we are passing an argument to a shell (i.e. can execute arbitrary code anyway), this should be disabled in order to correctly support quoting and spaces in commands. https://doc.rust-lang.org/std/process/index.html#windows-argument-splitting Fixes casey#1639.

casey

Nice, thank you for the PR!

Can you add a test for this? I want to make sure I understand the issue being fixed, and since it's subtle, make sure that there isn't a regression in the future.
Should we use this for all arguments? If not, why not?
See comments.

src/recipe.rs

jhasse · 2025-04-01T07:29:12Z

I'm not good at Rust, but I can try :) In which file would such a test go and is there a similar test I can look at?
Hm ... yes I think so, although I can't think of a scenario where it would matter. Maybe when someone has a shell with a parameter that needs spaces. I guess 99% of the time the shell parameter is /c or something similar though.

…n for raw_arg Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

casey · 2025-04-03T19:16:28Z

@jhasse The test can go in tests/windows.rs. I don't understand what the issue is myself, since windows argument handling seems extremely arcane to me, so I can't give much guidance about what an existing test to copy is. Check out any test that uses the Test::new() constructor to create a new test. If 99% of the time the shell parameter doesn't contain a space, that means 1% of the time it will break, which is too often ;) So we should definitely do whatever is the right thing.

jhasse · 2025-04-05T12:40:59Z

I've tried to run the test in windows.rs but it failed:

› cargo test bare_bash_in_shebang
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.14s
     Running unittests src\lib.rs (target\debug\deps\just-601a7f99ed7e8a65.exe)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 499 filtered out; finished in 0.00s

     Running tests\lib.rs (target\debug\deps\integration-a255eaaa12765568.exe)

running 1 test
test windows::bare_bash_in_shebang ... FAILED

failures:

---- windows::bare_bash_in_shebang stdout ----
Bad status: Diff < left / right > :
 Some(
<    127,
>    0,
 )

Bad stdout: Diff < left / right > :
>FOO
>

Bad stderr: Diff < left / right > :
</bin/bash: C:UsersjhasseAppDataLocalTempjust-cUgCqDdefault: No such file or directory
<error: Recipe `default` failed with exit code 127
<


thread 'windows::bare_bash_in_shebang' panicked at tests\windows.rs:14:6:
Output mismatch.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    windows::bare_bash_in_shebang

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 885 filtered out; finished in 0.12s

error: test failed, to rerun pass `--test integration`

Is there anything I need to set up on Windows to run the tests? Do I need to install Cygwin?

casey · 2025-04-05T20:14:19Z

Can you commit and push your failing test to the branch, so I can see it?

…ey#1639

jhasse · 2025-04-06T13:12:50Z

bare_bash_in_shebang was an existing test which I didn't touch, so there seems to be something fundamentally wrong with my setup.

I've pushed my attempt for a test for this issue.

casey · 2025-04-06T18:40:46Z

Nice, thank you! I think the issue is that && isn't valid syntax for cmd.exe, that's a bash-ism.

jhasse · 2025-04-07T07:00:45Z

No, && works the same way as in bash: https://superuser.com/a/1695562/114883

casey · 2025-04-07T17:01:20Z

@jhasse TIL! Check out the test logs, I'm seeing some failures in the unit tests which I don't understand.

jhasse · 2025-04-07T19:17:41Z

Hm ... me neither :(

I've tried setting RUST_BACKTRACE=1 to get a better understanding. No idea if that is the right way forward.

casey · 2025-04-07T19:33:03Z

I think that in order to fix this, you'll need to be able to run the tests locally, otherwise the debug loop will be really slow if you have to run the tests on CI. Are you set up to do Windows Rust development? I have a Windows laptop I use for stuff like this, but I'm pretty busy so I don't know when I'll be able to get it out to take a look.

jhasse · 2025-04-07T20:10:38Z

I have a Windows setup, but the test fail, even on the master branch (see #2689 (comment)).

casey requested changes Mar 31, 2025

View reviewed changes

src/recipe.rs Outdated Show resolved Hide resolved

Add use std::os::windows::process::CommandExt; to allow dot notatio…

b65de1b

…n for raw_arg Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

Add a test which quotes an argument with a space for cmd.exe, see cas…

e3498fa

…ey#1639

Merge branch 'master' into windows-raw-arg

00c9bb0

Set RUST_BACKTRACE=1 to understand CI failure

1ac3af7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use raw_arg on Windows to fix handling of spaces in commands #2689

Use raw_arg on Windows to fix handling of spaces in commands #2689

Use raw_arg on Windows to fix handling of spaces in commands #2689

Are you sure you want to change the base?

Use raw_arg on Windows to fix handling of spaces in commands #2689

Conversation

Choose a reason for hiding this comment