Windows Command: Don't run batch files using verbatim paths

[ChrisDenton]

Fixes #95178

Note that the first commit does some minor refactoring (moving command line argument building to args.rs). The actual changes are in the second.

[rust-highfive]

r? @joshtriplett

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

[ChrisDenton]

This is also a first step toward #94743. Rust no longer relies on the undocumented behaviour of CreateProcessW to run batch files. Instead it knows how to run them explicitly.

I also want to eventually do further refactoring of argument building. Given the growing number of options, a builder struct may be more appropriate. But I don't want to do that all in one PR.

EDIT: Oh and this PR lays the ground work for users being able to set argv[0] which would be nice to implement at some point.

[joshtriplett]

LGTM, r=me with the message nit fixed.

[joshtriplett]

@bors r+

[bors]

📌 Commit 7200afa has been approved by joshtriplett

[bors]

⌛ Testing commit 7200afa with merge b1651f12488d9ae50ccab10fce5c90e92a938d40...

[bors]

💔 Test failed - checks-actions

[ChrisDenton]

Hm, the failure is caused by https://github.com/rust-lang/rust/blob/master/src/test/ui-fulldeps/issue-15149.rs.
Which is a test added for #15149. The test ends up specifically testing if argv[0] ends with .exe. It should probably be testing env::current_exe() instead.

[ChrisDenton]

Oh, looks like my format-on-save has been to work on that file. The actual changes are lines 8, 22 and 23.

[joshtriplett]

@bors r+

[bors]

📌 Commit 4a0ec50 has been approved by joshtriplett

[bors]

⌛ Testing commit 4a0ec50 with merge 756ffb8...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 756ffb8 to master...

[rust-timer]

Finished benchmarking commit (756ffb8): comparison url.

Summary:

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	1	0	0	0	1
mean2	0.3%	N/A	N/A	N/A	0.3%
max	0.3%	N/A	N/A	N/A	0.3%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩

[shuffle2]

hi @ChrisDenton . I'm writing some rust code that needs argv identically to the way a C/C++ application would receive them (on modern windows). I was thrown off for some time because of the comments above the parse_lp_cmd_line seem to point to the wrong things. In actuality, "normal" CRT-based apps on windows use ucrtbase.dll!_configure_wide_argv (or the narrow one) to parse the command line. The source for this gets installed into e.g. C:\Program Files (x86)\Windows Kits\10\Source\10.0.22000.0\ucrt\startup\argv_parsing.cpp when you install the build tools. I just thought I'd mention it because I was confused about the apparent implementation by trial and error in rust to adhere to this behavior for which the source is actually available - I thought maybe you weren't aware.

afaict at least the daviddeley.com link and the mention of shell32 are therefor out-of-date and/or wrong.

I'll probably still reimplement it for my project...(it just makes sense for me to not implicitly depend on windows std :))

[ChrisDenton]

hi @shuffle2, thanks for the comment!

I'm aware that the ucrt source is available but I've avoided looking at it because I'm unclear if that would cause licensing issues. I'm not a lawyer and haven't hired one so I've taken the cautious approach as I don't want to cause problems for the Rust project in the future.

For now I'm going on public information and my own experimentation. If anyone wanted to write a more accurate description then that would be very welcome!

[shuffle2]

[continuing this conversion here... :)]

after looking at stuff a bit more, I realized that rust executables are already linking against and executing this code in ucrt. If you dynamic link against crt it's obvious by looking for imports from "api-ms-win-crt-*" libraries (these all get routed to ucrtbase.dll under the hood). In either case (dynamic or static link against crt), the function pre_c_initialization will be called before main (which is where the actual rust code begins). pre_c_initialization will (among other things) perform the initialization of argv. As far as I can tell, rust is also always compiling against ucrt in non-unicode mode (the argv and etc are using the "narrow" methods).

So, this created more questions than answers unfortunately:

1. Is rust knowingly linking against ucrt? Since target-feature=+crt-static is well documented I assume it's intentional, even though it's not yet clear to me where rust actually depends on it. Any idea what the reasoning is here? (maybe rust depends on ucrt to provide some machinery like TLS/stack cookie/other OS integrations?)
2. If rust already depends on ucrt for some reason, can we make it use wide characters (probably it should anyway, just to lessen possible confusion/footguns), and then just use the argv it has prepared? It's not clear to me why rust would let ucrt do the work but ignore the result.

[ChrisDenton]

The standard library does not decide on the program's entry point. That's the job of the Rust compiler. If you think it should instead use wmain and pass arguments to the std, then you should definitely open a new issue about that rather than discussing it on an unrelated and closed PR that's likely only being read by me. 🙂

Is rust knowingly linking against ucrt?

MSVC Rust needs the vcruntime for mem* functions and C++ exceptions. This in turn depends on having a crt. Rust also uses the C runtime for process startup/shutdown code and running pre-main initializers. So yes, it's "knowingly" linking against ucrt but it's incidental and could even change in the future (e.g. if we decide to reimplement the needed bits in pure Rust for some reason).

MinGW Rust does not link against the UCRT and so that toolchain will have different parsing rules. There can also be third-party toolchains that do their own thing (or skip arguments entirely). And even MSVC allows changing or disabling the parsing using special object files.

It may be surprising if std::env::args varied between Rust programs. Then again, this may be something you'd want. Either way I do think this should be a new issue.

[shuffle2]

The standard library does not decide on the program's entry point. That's the job of the Rust compiler.

Sure, I was mostly surprised that ucrt is involved at all - assumed it would be completely replaced, if replaced at all.

If you think it should instead use wmain and pass arguments to the std, then you should definitely open a new issue about that rather than discussing it on an unrelated and closed PR that's likely only being read by me. 🙂

yes well, I stalked the git blame and picked someone who had modified the code recently :D I'm not really sure where is a good forum for this topic - I checked discord and zulip and they seem unrelated and/or inactive. If just filing a github issue is the way, then I'll do that (eventually). Thanks!

[ChrisDenton]

Zulip would also be a great place to ask. Many people involved with Rust (both compiler and libs) are active there.

In general closed PRs are somewhat "hidden" because few people will notice new comments.