Add warning when PATH env separator is in project path #11318

RyanAD · 2022-11-01T00:30:36Z

Adds a check during cargo init and cargo new to check if the path contains an invalid PATH character (:, ;, or "). These characters can break things in various ways (including cargo test). These characters are not valid in certain environment variables and cannot be escaped.

For more information see:
https://github.com/rust-lang/rust/blob/7feb003882ecf7699e6705b537673e20985accff/library/std/src/sys/unix/os.rs#L233
https://man7.org/linux/man-pages/man8/ld.so.8.html
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
https://doc.rust-lang.org/std/env/fn.join_paths.html#errors

To test cargo new and cargo init:
cargo new --name testing test:ing
mkdir test:ing2 && cd 'test:ing2' && cargo init --name testing

To test the updated error message when in a directory with invalid characters:
cargo new testing3 && mv testing3 test:ing3 && cd 'test:ing3' && cargo test

cc @weihanglo

rustbot · 2022-11-01T00:30:42Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon.

Please see the contribution instructions for more information.

weihanglo

Thanks for the contribution!

crates/cargo-util/src/paths.rs

tests/testsuite/init/path_contains_separator/in/test:ing/mod.rs

crates/cargo-util/src/paths.rs

ChrisDenton · 2022-11-01T14:52:00Z

How does this work on Windows? The : character is highly likely to be in the PATH variable (it's the drive separator). And ; can be escaped with double quotes ("). Although admittedly not all applications handle that.

RyanAD · 2022-11-03T01:28:12Z

How does this work on Windows? The : character is highly likely to be in the PATH variable (it's the drive separator). And ; can be escaped with double quotes ("). Although admittedly not all applications handle that.

Fair question. I don't typically use Windows, let me set up a VM and do some testing and validation.

ChrisDenton · 2022-11-03T09:32:20Z

Looking at the CI failure, I'd also add that : isn't valid in a NTFS file name (and should be avoided in other Windows filesystems). It's used as:

The drive separator (e.g. C:\path\to\file)
The NTFS stream separator. So test:ing is the data stream called ing on the file called test

A normal file or directory with a : in the name cannot (or at least should not) occur. Therefore including such a file name in the repo makes it uncloneable on Windows.

weihanglo · 2022-11-03T09:33:38Z

How does this work on Windows? The : character is highly likely to be in the PATH variable (it's the drive separator). And ; can be escaped with double quotes ("). Although admittedly not all applications handle that.

Thank you for pointing it out! It was my bad providing an imprecise demo error message 🙇🏾‍♂️. We definitely can have : or ; as a separator for the whole $PATH variable, but not for a single item of $PATH. See my previous comment #3736 (comment) about why (semi)colon might be illegal on Unix for Cargo.

IIUC, : is an illegal character on Windows for paths¹. And you're correct. On Windows ; is legal when escaped. Here is my overall observation (not yet verified 😆 ) :

	Unix	Windows
`"`	✅	❌ ¹
`:`	❌²	❌ ¹
`;`	❌³	✅

To continue, I'd suggest diverging check_path by each platform accordingly, and polish the message a bit.

https://learn.microsoft.com/en-gb/windows/win32/fileio/naming-a-file#naming-conventions ↩ ↩² ↩³
It's totally legal to construct file path with a colon, but it's not legal to used in either $PATH or $LD_LIBRARY_PATH ↩
Same as : though std::env::join_paths doesn't check this. It is only manpage of ld.so saying so. Not sure about the consequence of including a semicolon in $LD_LIBRARY_PATH. ↩

ChrisDenton · 2022-11-03T09:51:51Z

When handling PATH on Windows, I'd say that the only disallowed character is ". Because that's unencodable in the PATH environment variable. Everything else is fine. Whether or not a character is allowed in a file name should be considered a separate concern.

weihanglo · 2022-11-03T10:03:19Z

You're right! If : or any character is inherently impossible to construct a path on a certain platform, then we may not need to warn for it.

FWIW, the situation has been improved over time, as the original issue is quite old and std::env::join_paths now has a different algorithm on each platform.

RyanAD · 2022-11-05T20:41:37Z

How does this work on Windows? The : character is highly likely to be in the PATH variable (it's the drive separator). And ; can be escaped with double quotes ("). Although admittedly not all applications handle that.

Thank you for pointing it out! It was my bad providing an imprecise demo error message 🙇🏾‍♂️. We definitely can have : or ; as a separator for the whole $PATH variable, but not for a single item of $PATH. See my previous comment #3736 (comment) about why (semi)colon might be illegal on Unix for Cargo.

IIUC, : is an illegal character on Windows for paths1. And you're correct. On Windows ; is legal when escaped. Here is my overall observation (not yet verified 😆 ) :
Unix Windows
" ✅ ❌ 1
: ❌2 ❌ 1
; ❌3 ✅

To continue, I'd suggest diverging check_path by each platform accordingly, and polish the message a bit.

Considering the differences in existing platforms, instead of diverging check_path by platform, what do you think about having it make a call to paths::join_paths, and if that fails showing the warning then? That keeps the platform specific pieces in one location. Something like this:
paths::join_paths(slice::from_ref(&OsStr::new(path)), "")... // handle Error and print warning

weihanglo · 2022-11-05T21:20:57Z

Considering the differences in existing platforms, instead of diverging check_path by platform, what do you think about having it make a call to paths::join_paths, and if that fails showing the warning then? That keeps the platform specific pieces in one location. Something like this:
paths::join_paths(slice::from_ref(&OsStr::new(path)), "")... // handle Error and print warning

@RyanAD Sounds like a good idea! Just remember to add a comment explaining why does that.

RyanAD · 2022-11-10T01:32:46Z

@weihanglo @ChrisDenton When you get a chance, here is an updated PR. I didn't add any tests for Windows because I couldn't consistently create a paths with invalid characters. I could set an ENV manually but that wasn't really the point of testing cargo new and cargo init. In any case, now that this just delegates to paths::join_paths I think the unix tests should be sufficient to show that the error is caught and a warning is shown.

The last remaining item is a test for paths::join_paths, I asked for clarity here: #11318 (comment)

ChrisDenton · 2022-11-10T04:59:13Z

Looks good to me. I guess you could add the reverse test for Windows, i.e. that it doesn't spuriously warn about \, ; or :.

weihanglo

Thank you for the update! The rest looks good to me.

crates/cargo-util/src/paths.rs

epage · 2022-11-10T16:00:07Z

r? weihanglo

since you've already been reviewing this (thanks!)

bors · 2022-11-11T04:46:14Z

☔ The latest upstream changes (presumably #11321) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo

Looks great! Thank @RyanAD for the contributions, and @ChrisDenton for the expert knowledge of Windows platform!

weihanglo · 2022-11-13T09:16:06Z

@bors r+

bors · 2022-11-13T09:16:08Z

📌 Commit eb8d3e2 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2022-11-13T09:16:14Z

⌛ Testing commit eb8d3e2 with merge 64e46b0...

bors · 2022-11-13T10:16:53Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 64e46b0 to master...

5 commits in a3dfea71ca0c888a88111086898aa833c291d497..16b097879b6f117c8ae698aab054c87f26ff325e 2022-11-11 03:50:47 +0000 to 2022-11-14 23:28:16 +0000 - improve error message for cargo add/remove (rust-lang/cargo#11375) - Bump crate versions of `cargo-util` and `crates-io` (rust-lang/cargo#11369) - doc(changelog): suggestions of cargo fix are nightly only (rust-lang/cargo#11373) - Add warning when PATH env separator is in project path (rust-lang/cargo#11318) - Fix git2 safe-directory disable (rust-lang/cargo#11366)

Update cargo 5 commits in a3dfea71ca0c888a88111086898aa833c291d497..16b097879b6f117c8ae698aab054c87f26ff325e 2022-11-11 03:50:47 +0000 to 2022-11-14 23:28:16 +0000 - improve error message for cargo add/remove (rust-lang/cargo#11375) - Bump crate versions of `cargo-util` and `crates-io` (rust-lang/cargo#11369) - doc(changelog): suggestions of cargo fix are nightly only (rust-lang/cargo#11373) - Add warning when PATH env separator is in project path (rust-lang/cargo#11318) - Fix git2 safe-directory disable (rust-lang/cargo#11366) r? `@ghost`

rustbot assigned epage Nov 1, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 1, 2022

weihanglo reviewed Nov 1, 2022

View reviewed changes

weihanglo reviewed Nov 10, 2022

View reviewed changes

crates/cargo-util/src/paths.rs Outdated Show resolved Hide resolved

crates/cargo-util/src/paths.rs Outdated Show resolved Hide resolved

rustbot assigned weihanglo and unassigned epage Nov 10, 2022

RyanAD added 5 commits November 12, 2022 18:43

Warn if project path contains an invalid PATH env character

82a92a2

Improve error message in join_paths when env contains a path separator

23c1a51

make call to paths::join_paths and warn if path contains invalid chars

730e8ca

keep empty test directory

f1fed1b

simplify join_paths error message and add test

eb8d3e2

RyanAD force-pushed the 3736-improve-invalid-char-error branch from 969ea02 to eb8d3e2 Compare November 13, 2022 01:46

weihanglo approved these changes Nov 13, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2022

bors merged commit 64e46b0 into rust-lang:master Nov 13, 2022

weihanglo mentioned this pull request Nov 16, 2022

Update cargo rust-lang/rust#104468

Merged

ehuss added this to the 1.67.0 milestone Dec 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning when PATH env separator is in project path #11318

Add warning when PATH env separator is in project path #11318

RyanAD commented Nov 1, 2022

rustbot commented Nov 1, 2022

weihanglo left a comment

ChrisDenton commented Nov 1, 2022

RyanAD commented Nov 3, 2022

ChrisDenton commented Nov 3, 2022

weihanglo commented Nov 3, 2022

ChrisDenton commented Nov 3, 2022

weihanglo commented Nov 3, 2022

RyanAD commented Nov 5, 2022

weihanglo commented Nov 5, 2022

RyanAD commented Nov 10, 2022 •

edited

Loading

ChrisDenton commented Nov 10, 2022

weihanglo left a comment

epage commented Nov 10, 2022

bors commented Nov 11, 2022

weihanglo left a comment

weihanglo commented Nov 13, 2022

bors commented Nov 13, 2022

bors commented Nov 13, 2022

bors commented Nov 13, 2022

Add warning when PATH env separator is in project path #11318

Add warning when PATH env separator is in project path #11318

Conversation

RyanAD commented Nov 1, 2022

rustbot commented Nov 1, 2022

weihanglo left a comment

Choose a reason for hiding this comment

ChrisDenton commented Nov 1, 2022

RyanAD commented Nov 3, 2022

ChrisDenton commented Nov 3, 2022

weihanglo commented Nov 3, 2022

Footnotes

ChrisDenton commented Nov 3, 2022

weihanglo commented Nov 3, 2022

RyanAD commented Nov 5, 2022

weihanglo commented Nov 5, 2022

RyanAD commented Nov 10, 2022 • edited Loading

ChrisDenton commented Nov 10, 2022

weihanglo left a comment

Choose a reason for hiding this comment

epage commented Nov 10, 2022

bors commented Nov 11, 2022

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Nov 13, 2022

bors commented Nov 13, 2022

bors commented Nov 13, 2022

bors commented Nov 13, 2022

RyanAD commented Nov 10, 2022 •

edited

Loading