Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: Fix fs::canonicalize to work with legacy drivers #86447

Closed

Conversation

ChrisDenton
Copy link
Contributor

@ChrisDenton ChrisDenton commented Jun 18, 2021

Attempts to fix: #59392, #79449, #59107, #54875, #52440, #52377, #48249, #74327, #55812

(sorry if I missed any)

The problem

On Windows, certain virtual drives (e.g. legacy RAM drives) manually create drive letter associations and mount points rather than going through the system Mount Manager (either directly or indirectly). The result of this means that volume management functions may fail. Unfortunately these functions include looking up the drive name, which in turn means that std::fs::canonicalize won't work.

The workaround

The solution presented here is to detect when querying the drive name fails and fallback to using the NT kernel path. This can be used in win32 by prepending \\?\GLOBALROOT to the kernel path (as documented here). The downside of this solution is that it produces "weird" looking paths that a user may not recognise and other applications may not be able to use.

E.g. this path:

R:\path\to\file.txt

May be canonicalized like so:

\\?\GLOBALROOT\Device\ImDisk0\path\to\file.txt

But it is at least usable by std::fs and it's perhaps better than having the crashes reported in some of the issues above.

Conclusion

Working in the presence of these drivers can be tricky and normally I would just suggest this isn't Rust's problem (which it technically isn't). However, these drivers seem widespread enough that I think it makes sense to try to workaround them, at least in this specific case.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2021
@joshtriplett
Copy link
Member

@rustbot ping windows

@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2021

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra @wesleywiser

@rustbot rustbot added the O-windows Operating system: Windows label Jun 19, 2021
@CDirkx
Copy link
Contributor

CDirkx commented Jun 19, 2021

For context, all paths are converted to a verbatim path by canonicalize on Windows, so R:\path\to\file.txt would be converted to \\?\R:\path\to\file.txt which is already kind of a "weird" path.

One potential issue I could think of is because of this it is somewhat common (although bad practice) to just strip the prefix \\?\ from the output of canonicalize. This works fine with \\?\R:\path\to\file.txt, but won't result in the correct path with \\?\GLOBALROOT\Device\ImDisk0\path\to\file.txt. However always stripping the prefix will already lead to problems with other kinds of paths (such as UNC), and without this PR canonicalize won't work anyway so I don't think that should be much of a factor.

@lzybkr
Copy link
Contributor

lzybkr commented Jun 21, 2021

In principle, canonicalize is very useful, but there are numerous issues mentioned in #59117. This PR addresses one such issue, but the others likely require a new api or risk breaking backwards compatibility.

I don't see any fundamental issues merging this PR, but it would be nice to see a new normalize api introduced that gets used instead of canonicalize in most places.

Note the crate dunce is frequently used as a wrapper over std::fs::canonicalize to strip the prefix \\?\ prefix. If that crate doesn't work well with this PR, I'd suggest updating dunce in parallel.

@nagisa
Copy link
Member

nagisa commented Jun 21, 2021

I've a concern here around user support. Yes, we do output weird paths on non-broken windows, but those paths generally don't look too wrong and people who triage issues are likely familiar with the rationale behind this.

People complaining about the even weirder paths introduced by this PR will be rare enough to not allow for familiarity to become commonplace, but are also significantly more likely to file an issue upon encounter because these paths won't resemble anything people are used to.

Either way I support the approach, I think. I definitely would appreciate Rust going above and beyond to handle a situation to at least try to handle my broken use-case.

@ChrisDenton
Copy link
Contributor Author

ChrisDenton commented Jun 21, 2021

I've a concern here around user support...

Yeah, that's my main concern. I think on balance it's worth it but I'm not totally confident in that.

it would be nice to see a new normalize api

I agree and it's on my todo list, unless somebody else gets there first.

@bors
Copy link
Contributor

bors commented Jul 3, 2021

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

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2021
@bors
Copy link
Contributor

bors commented Aug 20, 2021

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

@inquisitivecrystal inquisitivecrystal added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 24, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2021
@riverar
Copy link
Contributor

riverar commented Dec 25, 2021

Neither the PR or comments in the code point to the volume manager interface mentioned as required. Can you provide more information?

@ChrisDenton ChrisDenton changed the title Windows: Fix fs::canonicalize to work with broken drivers Windows: Fix fs::canonicalize to work with legacy drivers Dec 27, 2021
@ChrisDenton
Copy link
Contributor Author

I've updated the OP and title to be more specific and less opinionated following our conversation.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2022
@ChrisDenton
Copy link
Contributor Author

I think at the moment I'd like to pursue other options for addressing this issue. For example, in some situations path::absolute would allow projects to use a fallback in case fs::canonicalize fails. Where that's not appropriate (i.e when symlinks must be resolved) then perhaps a third party crate or another nightly API could provide a way for projects to experiment with using VOLUME_NAME_NT or alternative ways of attempting to resolve the volume. That way the Rust ecosystem can experiment with different solutions without it being instantly stabilized in fs::canonicalize.

So I'll close this for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using 'cargo install xsv' on windows 10 triggers rustc internal error