Handle win32 separator for cygwin paths

[Berrysoft]

This PR handles a issue that cygwin actually supports Win32 path, so we need to handle the Win32 prefix and separaters.

r? @mati865

cc @jeremyd2019

Not sure if I should handle the prefix like the windows target... Cygwin does support win32 paths directly going through the APIs, but I think it's not the recommended way.

Here I just use cygwin_conv_path because it handles both cygwin and win32 paths correctly and convert them into absolute POSIX paths.

UPDATE: Windows path prefix is handled.

[jeremyd2019]

Here I just use cygwin_conv_path because it handles both cygwin and win32 paths correctly and convert them into absolute POSIX paths.

I was going to raise the issue of encoding, but looking at the code it doesn't seem to deal with WIN_A as a different encoding than POSIX paths, so I guess that's not an issue. I guess I need to have a conversation on the Cygwin list to better understand this.

[Berrysoft]

Finally I reuse the prefix parser from Windows. Now the cygwin Path can handle Win32 path correctly. It should behave a little different as it allows / as verbatim separater. Here is a simple test code:

use std::path::{absolute, Path};

fn main() {
    test_path("C:\\msys64");
    test_path("/dev/sda1");
    test_path("./sda1");
    test_path("/c/Users");
    test_path("\\\\?\\UNC/localhost/share");
}

fn test_path(p: impl AsRef<Path>) {
    let p = p.as_ref();
    let abs = absolute(p).unwrap();
    if p.is_absolute() {
        println!("{} == {}", p.display(), abs.display());
    } else {
        println!("{} -> {}", p.display(), abs.display());
    }
    println!("{:?}", p.components().collect::<Vec<_>>());
}

On Windows, it should output (the current drive is D:)

C:\msys64 == C:\msys64
[Prefix(PrefixComponent { raw: "C:", parsed: Disk(67) }), RootDir, Normal("msys64")]
/dev/sda1 -> D:\dev\sda1
[RootDir, Normal("dev"), Normal("sda1")]
./sda1 -> <<<current dir>>>\sda1
[CurDir, Normal("sda1")]
/c/Users -> D:\c\Users
[RootDir, Normal("c"), Normal("Users")]
\\?\UNC/localhost/share == \\?\UNC/localhost/share
[Prefix(PrefixComponent { raw: "\\\\?\\UNC/localhost/share", parsed: VerbatimUNC("localhost/share", "") })]

While on Cygwin, it should output

C:\msys64 == /c/msys64
[Prefix(PrefixComponent { raw: "C:", parsed: Disk(67) }), RootDir, Normal("msys64")]
/dev/sda1 == /dev/sda1
[RootDir, Normal("dev"), Normal("sda1")]
./sda1 -> <<<current dir>>>/sda1
[CurDir, Normal("sda1")]
/c/Users == /c/Users
[RootDir, Normal("c"), Normal("Users")]
\\?\UNC/localhost/share == //?/UNC/localhost/share
[Prefix(PrefixComponent { raw: "\\\\?\\UNC/localhost/share", parsed: VerbatimUNC("localhost", "share") })]

[Berrysoft]

@jeremyd2019

I was going to raise the issue of encoding, but looking at the code it doesn't seem to deal with WIN_A as a different encoding than POSIX paths, so I guess that's not an issue. I guess I need to have a conversation on the Cygwin list to better understand this.

Actually rust doesn't handle very well for non-UTF-8 encoding on Unix. OsStr and Path are all UTF-8 (because they could be constructed from a UTF-8 str without copy) and they are passed to OS APIs directly without encoding conversion. The only concerns, I think, should be command line input and console input. Do you know that whether Cygwin handles the encoding conversion of argv and stdin? I hope it does:)

[Berrysoft]

OK, seems that the command line is converted from UTF-16: https://github.com/cygwin/cygwin/blob/972872c0d396dbf0ce296b8280cee08ce7727b51/winsup/cygwin/dcrt0.cc#L902

And stdin handles the encoding conversion: https://github.com/cygwin/cygwin/blob/972872c0d396dbf0ce296b8280cee08ce7727b51/winsup/cygwin/fhandler/pty.cc#L3935

So I think all inputs are UTF-8 for a Cygwin program, and the developers could not construct a non-UTF-8 Win32 path if they aren't aware of encodings. If they are, they should know that Rust codes are UTF-8:)

[Berrysoft]

Now I'm a little worried about cygwin_conv_path then. If the maintainers someday find that it should handle code page encodings, the code here will be messed up:(

[pietroalbini]

This was originally reported by @Ry0taK through Rust's security disclosure process, and since this affects a still-in-development tier 3 target (with very few users right now) we agreed to let the development of the fix happen in the open.

[mati865]

This change is T-libs area, so it will require their review.

r? rust-lang/libs

I'm not familiar with Cygwin API at all, but I see what you want to achieve here, and I think it's sensible.

Now I'm a little worried about cygwin_conv_path then. If the maintainers someday find that it should handle code page encodings, the code here will be messed up:(

They wouldn't want to break that API as a normal release, right? I'd expected a documented API to remain backwards compatible.

@pietroalbini I'm out of the loop, could you shed some light on your comment?

[mati865]

Also, there is a typo in both the title and the first commit 😉

[jeremyd2019]

So I think all inputs are UTF-8 for a Cygwin program, and the developers could not construct a non-UTF-8 Win32 path if they aren't aware of encodings. If they are, they should know that Rust codes are UTF-8:)

Cygwin's encoding may not always be UTF-8 - one can set the locale environment variables like they can on Unix to set the encoding, and then the conversions of paths and terminal input will use that encoding instead of UTF-8.

[Berrysoft]

In such locale, Rust doesn't even work on Linux. That's fair:)

[pietroalbini]

@pietroalbini I'm out of the loop, could you shed some light on your comment?

@mati865 this was originally reported to security@rust-lang.org as it could defeat path traversal mitigations like this:

if p.components().into_iter().any(|x| x == Component::ParentDir) {
    panic!("path traversal");
}

By only parsing unix-like paths, it would be possible to sneak in a path traversal using win32 paths. Given the current status of the target we agreed it would be ok to develop the fix in the open.

[Berrysoft]

@tgross35 ping?

[tgross35]

Looks pretty reasonable to me, a few small comments/questions

[tgross35]

@pietroalbini I'm out of the loop, could you shed some light on your comment?

@mati865 this was originally reported to security@rust-lang.org as it could defeat path traversal mitigations like this:

if p.components().into_iter().any(|x| x == Component::ParentDir) {
    panic!("path traversal");
}

By only parsing unix-like paths, it would be possible to sneak in a path traversal using win32 paths. Given the current status of the target we agreed it would be ok to develop the fix in the open.

@Berrysoft could you add a regression test to this effect that fails without this change? Probably in library/std/tests/path.rs.

[tgross35]

One test request then lgtm with a squash

[jeremyd2019]

Should there be some testing of // prefixes, like //?/C:/foo/bar and //server/share/foo.txt?

[fmease]

@bors r=ChrisDenton

[bors]

📌 Commit 3cb0cba has been approved by ChrisDenton

It is now in the queue for this repository.

[Berrysoft]

Will this PR catch up 1.89?😢

[mati865]

It should make it for 1.89 considering the state of the queue: https://bors.rust-lang.org/queue/rust

This was originally reported by @Ry0taK through Rust's security disclosure process, and since this affects a still-in-development tier 3 target (with very few users right now) we agreed to let the development of the fix happen in the open.

Even though this is tier 3 target, maybe this security fix should warrant a priority bump?

[Berrysoft]

Yes, I would like a priority bump if possible:)

[pietroalbini]

@bors p=10

Security fix.

[ChrisDenton]

I'm not against prioritising this but does it actually matter if it is merged after the beta branches? It's a tier 3 target so it's not something we build or distribute with our releases.

[pietroalbini]

I assume people will want to still use it from source. Given that it's a security fix I'd like to err on the side of caution.

[mati865]

TBH, I was a bit surprised tier 3 target got this treatment, but since we are there already, I think it would be sensible to get to the finish line.

It's a tier 3 target so it's not something we build or distribute with our releases.

Downstreams can distribute it, though. Some of them could be unhappy if a known security vulnerability wasn't either fixed or at least mentioned (so they can backport the fix themselves).

[bors]

⌛ Testing commit 3cb0cba with merge 8de4c72...

[bors]

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing 8de4c72 to master...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 2fcf177 (parent) -> 8de4c72 (this PR)

Test differences

Show 102 test diffs

102 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 8de4c7234dd9b97c9d76b58671343fdbbc9a433e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-aarch64-msvc: 8563.2s -> 14983.1s (75.0%)
2. dist-aarch64-linux: 5453.3s -> 8163.6s (49.7%)
3. dist-apple-various: 6344.2s -> 8283.5s (30.6%)
4. x86_64-apple-1: 6425.4s -> 7805.5s (21.5%)
5. dist-aarch64-apple: 5659.8s -> 6723.7s (18.8%)
6. dist-x86_64-apple: 9884.2s -> 11510.2s (16.4%)
7. dist-various-2: 3135.2s -> 3377.5s (7.7%)
8. x86_64-gnu-distcheck: 7457.4s -> 7951.4s (6.6%)
9. i686-gnu-nopt-2: 7633.9s -> 8110.6s (6.2%)
10. dist-powerpc64-linux: 4982.5s -> 5289.4s (6.2%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (8de4c72): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.8%, -0.4%]	5
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -4.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.1%	[-5.8%, -2.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -1.9%, secondary 4.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.6%	[4.6%, 4.6%]	1
Improvements ✅ (primary)	-1.9%	[-1.9%, -1.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.9%	[-1.9%, -1.9%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 692.244s -> 691.904s (-0.05%)
Artifact size: 371.99 MiB -> 372.02 MiB (0.01%)