Prevent accidentally splitting file paths with spaces in RUSTFLAGS

[fschutt]

Closes #6139

[rust-highfive]

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

[fschutt]

There are a few caveats:

1. This has to be tested with the correct file path format on the platform it's intended to run on (``Path("C://").has_root()` will fail on Linux CI)
2. Only works for absolute paths for now
3. I didn't add any test cases yet

In the worst case, the function does nothing, but in the usual case it merges broken paths in RUSTFLAGS like:

[
    "--print=file-names",
    "--sysroot",
    "'C:\\Users\\John",
    "Doe\\Test",
    "Path\\file'",
    "-Z",
    "force-unstable-if-unmarked",
]

to:

[
    "--print=file-names",
    "--sysroot",
    "'C:\\Users\\John Doe\\Test Path\\file'",
    "-Z",
    "force-unstable-if-unmarked",
]

I just wanted this in to get Xargo working on my machine without having to go through the hassle of creating a new user account without spaces.

[alexcrichton]

Thanks for the PR, but dealing with shell quoting and things like that is something that we specifically try to avoid in Cargo since shell quoting is system-specific and shell-specific. Are you able to set flags in ~/.cargo/config.toml? There you can use an array-of-strings which handles spaces correctly.

[ehuss]

Thanks for the PR! Unfortunately, I don't think this is a direction we would like to go with interpreting something like RUSTFLAGS. It introduces an ad-hoc quoting mechanism that can cause subtle issues with escaping, shell interpretation, and the like.

There are several stable and unstable alternatives, and I was wondering if you would be willing to try them out and see if they are suitable for your use case? Some alternatives are:

• Use .cargo/config.toml files instead of environment variables. You can set build.rustflags to a TOML array that supports spaces.
• Use the --config CLI option with something like cargo build --config "build.rustflags = ['-L', 'path with spaces']" -Zunstable-options
• Use the -Z advanced-env option to use TOML in an environment variable. For example: CARGO_BUILD_RUSTFLAGS="['-L', 'path with spaces']" cargo build -Z advanced-env

[fschutt]

Well the real issue is that none of these workarounds work with Xargo, which is what I'm trying to achieve in the first place:

Running xargo +nightly build --release results in:

 xargo +nightly build --release

error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: 
`rustc - 
     --crate-name ___ 
     --print=file-names 
     --sysroot 'C:\Users\Felix' 'Schütt\.xargo\HOST' 
     -Z force-unstable-if-unmarked 
     --target x86_64-pc-windows-msvc 
     --crate-type bin 
     --crate-type rlib 
     --crate-type dylib 
     --crate-type cdylib 
     --crate-type staticlib 
     --crate-type proc-macro 
     --print=sysroot 
     --print=cfg` (exit code: 1)
  --- stderr
  error: multiple input filenames provided (
first two filenames are `-` and `Schütt\.xargo\HOST`)

error: `
    "cargo" 
    "build" 
    "--release" 
    "--manifest-path" "C:\\Users\\FELIXS~1\\AppData\\Local\\Temp\\xargo.x4gDEaIe6fCA\\Cargo.toml" 
    "--target" "x86_64-pc-windows-msvc" 
    "-p" "core"` failed with exit code: Some(101)
note: run with `RUST_BACKTRACE=1` for a backtrace

I can't apply the workarounds, because xargo doesn't respect the input arguments and always runs RUSFLAGS = $(rustc --print-sysroot), which contains the spaces. Even if I specify the short path:

 RUSTFLAGS='--sysroot "C:\Users\FELIXS~1\.rustup\toolchains\nightly-x86_64-pc-windows-msvc' \
xargo +nightly build --release -v

fails because option --sysroot is specified multiple times. So what should I do now?

This is a well known issue, but the xargo team doesn't want to fix it because "it has to be fixed in cargo", now the cargo team is telling me "noo, this has to be fixed in xargo or in external tools". So whos responsibility is it? Also, I'd have to edit the cargo/.config file manually on every new machine (???).

 xargo +nightly build --release -v
+ "rustc" "--print" "sysroot"
+ RUSTFLAGS="--sysroot C:\\Users\\Felix Schütt\\.xargo\\HOST -Z force-unstable-if-unmarked"
+ "cargo" "build" "--release" "--manifest-path" "C:\\Users\\FELIXS~1\\AppData\\Local\\Temp\\xargo.iG4TjDfZkXzL\\Cargo.toml" "--target" "x86_64-pc-windows-msvc" "-v" "-p" "core"

xargo ALWAYS exports RUSTFLAGS manually, I cannot unset it via any other method. I suspect, the problem is here:

let rustflags = cargo::rustflags(config.as_ref(), cmode.triple())?;

sysroot::update(
    &cmode,
    &home,
    &root,
    &rustflags,
    &meta,
    &src,
    &sysroot,
    verbose,
    args.message_format(),
    cargo_mode,
)?;

@ehuss I'm not sure what "vulnerabilities" you mean, it's at most a call to stat(2) or similar on Windows, what "potential code execution vulnerabilites" does that imply? If the file does not exists, no merging of strings will occur, so in the worst case it'll be the same behaviour as before

@alexcrichton "shell quoting is system-specific and shell-specific" - Okay, but this is not about shell quoting, it's about spaces. At the point that the RUSTFLAGS are read, the shell / env should have already parsed the strings correctly. The shell already gives you the correct argument as a . But then cargo is doing a .split(" ") (so cargo does already manipulate CLI args, independent of shell).

[alexcrichton]

If the problem is xargo setting RUSTFLAGS unconditionally then that can be fixed with the recently added feature of CARGO_ENCODED_RUSTFLAGS. This is Cargo's "fix" to the solution for xargo.

[alexcrichton]

Ok I'm going to go ahead and close this because I don't think we want to merge this. If CARGO_ENCODED_RUSTFLAGS doesn't work for xargo though please feel free to file an issue and we can try to figure out why and chart an alternate course.

[fschutt]

Yeah I am going to try changing it so that setting the RUSTFLAGS is done over CARGO_ENCODED_RUSTFLAGS. Then the splitting wouldn't be an issue anymore.

[RalfJung]

If the problem is xargo setting RUSTFLAGS unconditionally then that can be fixed with the recently added feature of CARGO_ENCODED_RUSTFLAGS. This is Cargo's "fix" to the solution for xargo.

FWIW, CARGO_ENCODED_RUSTFLAGS is not listed as an "environment variable cargo reads" in the docs. Is that a mistake? It does say in the RUSTFLAGS entry that one should set CARGO_ENCODED_RUSTFLAGS instead, but not having CARGO_ENCODED_RUSTFLAGS itself in that itemized list is a bit confusing.

[ehuss]

I filed #10555 to improve the documentation.