Correctly convert an NT path to a Win32 path in `read_link` #107978

ChrisDenton · 2023-02-13T03:39:47Z

This can be done by simply changing the \??\ prefix to \\?\.

Currently it strips off the prefix which could lead to the wrong path being returned (e.g. if it's not a drive path or if the path contains trailing spaces, etc).

r? libs

rustbot · 2023-02-13T03:39:54Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

m-ou-se · 2023-02-13T14:19:03Z

e.g. if it's not a drive path or if the path contains trailing spaces, etc

Would it make sense to only add the prefix in those cases? Then simple paths like C:\ProgramData will remain unchanged.

ChrisDenton · 2023-02-13T15:09:58Z

To be clear the absolute path returned from the OS API is like \??\C:\path which is an NT kernel path. This can be trivially and losslessly changed into a win32 verbatim path simply by modifying the prefix to \\?\C:\path. This is similar to how paths returned by canonicalize work. And I think it makes some sense for canonicalize and read_link to work similarly.

We could maybe transform the NT prefix into the non-verbatim win32 equivalent while taking care also to avoid doing so if the transformation might be lossy. But this is more difficult because there are a lot of things to be aware of (like if the filename happens to match DOS device name). Hm, I guess the easiest thing would be to attempt to pass the modified path via path::absolute and see if it changes at all. It's just a bit more complex and an extra API call.

vthib · 2023-04-26T11:15:21Z

I have stumbled upon this bug recently, which causes some real issues. When a link is made with a UNC target, the read_link implement returns a buggy path.

For example, a mklink /d foo \\server\path command will create a symlink pointing to a network server, and will generate a reparse point with this substitute name: \??\UNC\server\path. However the current implementation of read_link will incorrectly strip the \??\ prefix, and return an invalid path: UNC\server\path.

This can cause issues as this path can actually exist, so the read_link call can point to a completely different file, which I think can be a source for security issues. Also, it makes it impossible for the caller to find out if the path is towards a network server, or whether it is absolute or relative.

I was pondering creating an issue for this, but I found this MR which would solve the issue. Is there anything blocking it?

m-ou-se · 2023-05-01T14:43:21Z

@bors r+

bors · 2023-05-01T14:43:23Z

📌 Commit 178f7bb has been approved by m-ou-se

It is now in the queue for this repository.

Correctly convert an NT path to a Win32 path in `read_link` This can be done by simply changing the `\??\` prefix to `\\?\`. Currently it strips off the prefix which could lead to the wrong path being returned (e.g. if it's not a drive path or if the path contains trailing spaces, etc). r? libs

Dylan-DPC · 2023-05-03T07:31:02Z

@bors r-

failed in rollup

This can be done by simply changing the `\??\` prefix to `\\?\` and then attempting to convert to a user path. Currently it simply strips off the prefix which could lead to the wrong path being returned (e.g. if it's not a drive path or if the path contains trailing spaces, etc).

ChrisDenton · 2023-05-03T10:22:27Z

Oh, the function this is using changed in another PR. Rebasing...

Dylan-DPC · 2023-05-03T10:29:45Z

@bors r=m-ou-se

bors · 2023-05-03T10:29:47Z

📌 Commit 6d0a0836f654f2ffedbbbb0caff34bd17e898b04 has been approved by m-ou-se

It is now in the queue for this repository.

ChrisDenton · 2023-05-03T12:17:11Z

Sorry, I needed to rustfmt.

@bors r-
@bors r=m-ou-se

bors · 2023-05-03T12:17:15Z

📌 Commit 109a47f has been approved by m-ou-se

It is now in the queue for this repository.

Rollup of 11 pull requests Successful merges: - rust-lang#107978 (Correctly convert an NT path to a Win32 path in `read_link`) - rust-lang#110436 (Support loading version information from xz tarballs) - rust-lang#110791 (Implement negative bounds for internal testing purposes) - rust-lang#110874 (Adjust obligation cause code for `find_and_report_unsatisfied_index_impl`) - rust-lang#110908 (resolve: One more attempt to simplify `module_children`) - rust-lang#110943 (interpret: fail more gracefully on uninit unsized locals) - rust-lang#111062 (Don't bail out early when checking invalid `repr` attr) - rust-lang#111069 (remove pointless `FIXME` in `bootstrap::download`) - rust-lang#111086 (Remove `MemEncoder`) - rust-lang#111097 (Avoid ICEing miri on layout query cycles) - rust-lang#111112 (Add some triagebot notifications for nnethercote.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Correctly convert an NT path to a Win32 path in `read_link` This can be done by simply changing the `\??\` prefix to `\\?\`. Currently it strips off the prefix which could lead to the wrong path being returned (e.g. if it's not a drive path or if the path contains trailing spaces, etc). r? libs

rustbot assigned m-ou-se Feb 13, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 13, 2023

ChrisDenton force-pushed the nt-to-win32 branch from 0b3fff1 to 178f7bb Compare February 18, 2023 19:15

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 May 1, 2023

Dylan-DPC mentioned this pull request May 3, 2023

Rollup of 9 pull requests #111122

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 3, 2023

ChrisDenton force-pushed the nt-to-win32 branch from 178f7bb to 6d0a083 Compare May 3, 2023 10:23

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 3, 2023

This comment has been minimized.

Sign in to view

Use from_wide_to_user_path in read_link

109a47f

ChrisDenton force-pushed the nt-to-win32 branch from 6d0a083 to 109a47f Compare May 3, 2023 10:31

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 3, 2023

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 3, 2023

Dylan-DPC mentioned this pull request May 3, 2023

Rollup of 11 pull requests #111153

Merged

Manishearth mentioned this pull request May 3, 2023

Rollup of 10 pull requests #111163

Closed

Manishearth mentioned this pull request May 3, 2023

Rollup of 11 pull requests #111165

Closed

bors merged commit 068807a into rust-lang:master May 3, 2023

rustbot added this to the 1.71.0 milestone May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly convert an NT path to a Win32 path in `read_link` #107978

Correctly convert an NT path to a Win32 path in `read_link` #107978

ChrisDenton commented Feb 13, 2023

rustbot commented Feb 13, 2023

m-ou-se commented Feb 13, 2023 •

edited

Loading

ChrisDenton commented Feb 13, 2023

vthib commented Apr 26, 2023

m-ou-se commented May 1, 2023

bors commented May 1, 2023

Dylan-DPC commented May 3, 2023

ChrisDenton commented May 3, 2023 •

edited

Loading

Dylan-DPC commented May 3, 2023

bors commented May 3, 2023

This comment has been minimized.

ChrisDenton commented May 3, 2023

bors commented May 3, 2023

Correctly convert an NT path to a Win32 path in read_link #107978

Correctly convert an NT path to a Win32 path in read_link #107978

Conversation

ChrisDenton commented Feb 13, 2023

rustbot commented Feb 13, 2023

m-ou-se commented Feb 13, 2023 • edited Loading

ChrisDenton commented Feb 13, 2023

vthib commented Apr 26, 2023

m-ou-se commented May 1, 2023

bors commented May 1, 2023

Dylan-DPC commented May 3, 2023

ChrisDenton commented May 3, 2023 • edited Loading

Dylan-DPC commented May 3, 2023

bors commented May 3, 2023

This comment has been minimized.

ChrisDenton commented May 3, 2023

bors commented May 3, 2023

Correctly convert an NT path to a Win32 path in `read_link` #107978

Correctly convert an NT path to a Win32 path in `read_link` #107978

m-ou-se commented Feb 13, 2023 •

edited

Loading

ChrisDenton commented May 3, 2023 •

edited

Loading