Try not to use verbatim paths in Command::current_dir
#138869
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
Noratrieb
reviewed
Mar 24, 2025
ChrisDenton
force-pushed
the
command-curdir
branch
from
c97807a to
9be8b3a
Compare
I'm honestly not sure, so choosing someone at random: |
tgross35
reviewed
Apr 8, 2025
Comment on lines
+354
to
+363
| /// Test that the path is absolute, fully qualified and unchanged when processed by the Windows API. | ||
| /// | ||
| /// For example: | ||
| /// | ||
| /// - `C:\path\to\file` will return true. | ||
| /// - `C:\path\to\nul` returns false because the Windows API will convert it to \\.\NUL | ||
| /// - `C:\path\to\..\file` returns false because it will be resolved to `C:\path\file`. | ||
| /// | ||
| /// This is a useful property because it means the path can be converted from and to and verbatim | ||
| /// path just by changing the prefix. |
There was a problem hiding this comment.
Verbatim paths can contain special names like COM while regular paths cannot right? That may be worth mentioning and adding a test for.
There was a problem hiding this comment.
The COM1, CON, AUX, etc cases is the same as the NUL case mentioned and tested. It can occur in paths but may be changed to e.g. \\.\COM1 for non-verbatim paths. I didn't add a test for the COM1 case because it is actually allowed on recent versions of Windows. Only the relative path COM1 would be converted to \\.\COM1 but that's not absolute in any case.
ChrisDenton
force-pushed
the
command-curdir
branch
from
4c05a91 to
2fa6b9f
Compare
tgross35
approved these changes
Apr 9, 2025
If possible, we should try not to use verbatim paths in Command::current_dir. It might work but it might also break code (including some Windows APIs) that assume the current directory isn't verbatim.
ChrisDenton
force-pushed
the
command-curdir
branch
from
2fa6b9f to
edfc747
Compare
|
@bors r=tgross35 |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Apr 9, 2025
…ss35 Try not to use verbatim paths in `Command::current_dir` If possible, we should try not to use verbatim paths in `Command::current_dir`. It might work but it might also break code in the subprocess that assume the current directory isn't verbatim (including Windows APIs). cc `@ehuss` Side note: we now have a lot of ad-hoc fixes like this spread about the place. It'd be good to make a proper `WindowsPath` type that handles all this in one place. But that's a bigger job for another PR.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 9, 2025
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#137412 (Ensure `swap_nonoverlapping` is really always untyped) - rust-lang#138869 (Try not to use verbatim paths in `Command::current_dir`) - rust-lang#138993 (Make `cfg_match!` a semitransparent macro) - rust-lang#139099 (Promise `array::from_fn` is generated in order of increasing indices) - rust-lang#139364 (Make the compiler suggest actual paths instead of visible paths if the visible paths are through any doc hidden path.) - rust-lang#139468 (Don't call `Span::with_parent` on the good path in `has_stashed_diagnostic`) - rust-lang#139481 (Add job summary links to post-merge report) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 9, 2025
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138869 (Try not to use verbatim paths in `Command::current_dir`) - rust-lang#138993 (Make `cfg_match!` a semitransparent macro) - rust-lang#139099 (Promise `array::from_fn` is generated in order of increasing indices) - rust-lang#139364 (Make the compiler suggest actual paths instead of visible paths if the visible paths are through any doc hidden path.) - rust-lang#139468 (Don't call `Span::with_parent` on the good path in `has_stashed_diagnostic`) - rust-lang#139481 (Add job summary links to post-merge report) - rust-lang#139573 (Miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 9, 2025
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138869 (Try not to use verbatim paths in `Command::current_dir`) - rust-lang#138993 (Make `cfg_match!` a semitransparent macro) - rust-lang#139099 (Promise `array::from_fn` is generated in order of increasing indices) - rust-lang#139364 (Make the compiler suggest actual paths instead of visible paths if the visible paths are through any doc hidden path.) - rust-lang#139468 (Don't call `Span::with_parent` on the good path in `has_stashed_diagnostic`) - rust-lang#139481 (Add job summary links to post-merge report) - rust-lang#139573 (Miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 9, 2025
Rollup merge of rust-lang#138869 - ChrisDenton:command-curdir, r=tgross35 Try not to use verbatim paths in `Command::current_dir` If possible, we should try not to use verbatim paths in `Command::current_dir`. It might work but it might also break code in the subprocess that assume the current directory isn't verbatim (including Windows APIs). cc ``@ehuss`` Side note: we now have a lot of ad-hoc fixes like this spread about the place. It'd be good to make a proper `WindowsPath` type that handles all this in one place. But that's a bigger job for another PR.
github-actions bot
pushed a commit
to model-checking/verify-rust-std
that referenced
this pull request
Apr 19, 2025
…ss35 Try not to use verbatim paths in `Command::current_dir` If possible, we should try not to use verbatim paths in `Command::current_dir`. It might work but it might also break code in the subprocess that assume the current directory isn't verbatim (including Windows APIs). cc ``@ehuss`` Side note: we now have a lot of ad-hoc fixes like this spread about the place. It'd be good to make a proper `WindowsPath` type that handles all this in one place. But that's a bigger job for another PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If possible, we should try not to use verbatim paths in
Command::current_dir. It might work but it might also break code in the subprocess that assume the current directory isn't verbatim (including Windows APIs). cc @ehussSide note: we now have a lot of ad-hoc fixes like this spread about the place. It'd be good to make a proper
WindowsPathtype that handles all this in one place. But that's a bigger job for another PR.