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

Figure out final solution for volume-relative windows paths #11856

Closed
lilyball opened this issue Jan 27, 2014 · 9 comments
Closed

Figure out final solution for volume-relative windows paths #11856

lilyball opened this issue Jan 27, 2014 · 9 comments
Labels
O-windows Operating system: Windows

Comments

@lilyball
Copy link
Contributor

As extensively documented in #11579, volume-relative paths on Windows are a relic of the past. We either need to remove support for it, or we need to figure out how ls, etc running in cmd.exe are emulating volume-relative cwds and teach getcwd() about that.

If we remove support for volume-relative paths, we should probably not make it a condition, because that's pretty crappy. We may want to just treat it as a path without a prefix.

@lilyball
Copy link
Contributor Author

Volume-relative paths make a lot of things more complicated. And notably, it means os::make_absolute(foo).is_absolute() can no longer be guaranteed to return true.

@mitsuhiko
Copy link
Contributor

I want to bump this a bit because I think this is security related.

Currently the WindowsPath::is_absolute() behavior is downright dangerous. The generic path behavior for is_absolute() defines this as such:

Returns whether self represents an absolute path. An absolute path is defined as one that, when joined to another path, will yield back the same absolute path.

Different working directories on different drives is a concept long gone. The only software that still has this is cmd.exe and it emulates it. (http://blogs.msdn.com/b/oldnewthing/archive/2010/10/11/10073890.aspx)

The reason the current behavior is dangerous because you can end up trusting is_absolute() to the current behavior to implement path manipulation algorithms where you expect the end result to not escape a certain folder when used in a certain context. (Obviously that by itself does not help you but you might make wrong conclusions from it).

@steveklabnik
Copy link
Member

@aturon @alexcrichton how did this fall out with path reform?

@retep998
Copy link
Member

This issue isn't done yet, because we still need to implement a way to turn volume relative paths into true absolute paths. This means getting the current working directory for that drive and joining the paths.

While cmd.exe has a variable current directory for each drive, Win32 has only a single variable current directory. Unfortunately Win32 still has immutable current directories set at process startup for each drive which it respects when interpreting volume relative paths.

At first I thought we were doomed because GetCurrentDirectory only returns that single current directory and not the drive specific current directories. Furthermore if we don't want to be limited by MAX_PATH we can't call GetFullPathName with the full path. As it turns out, however, we can simply call GetFullPathName with just the prefix like C: and it'll give us the correct current directory on which to join the rest of the path.

@aturon
Copy link
Member

aturon commented Feb 16, 2015

@retep998 Would you be interested in taking this project on?

@retep998
Copy link
Member

@aturon Sure. Just point me at what functions need doing and I'll do them.

@huonw
Copy link
Member

huonw commented Jan 5, 2016

Has anything happened here? (@aturon?)

@retep998
Copy link
Member

retep998 commented Jan 5, 2016

There was a PR to turn relative paths and absolute paths into verbatim paths. #27916
There were lots of concerns in it regarding various edge cases.
I think what should happen at this point is someone should develop such a function in an outside crate first and then it can be pulled into std once people seem satisfied by how it handles various edge cases.

@aturon
Copy link
Member

aturon commented Jan 5, 2016

I agree with @retep998. I'm going to close this issue, and we can address it via an eventual addition to std as @retep998 is suggesting.

@aturon aturon closed this as completed Jan 5, 2016
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 1, 2023
[`missing_asserts_for_indexing`]: work with bodies instead of blocks separately

Fixes rust-lang#11856

Before this change, this lint would check blocks independently of each other, which means that it misses `assert!()`s from parent blocks.
```rs
// check_block
assert!(x.len() > 1);

{
  // check_block
  // no assert here
  let _ = x[0] + x[1];
}
```

This PR changes it to work with bodies rather than individual blocks. That means that a function will be checked in one go and we can remember if an `assert!` occurred anywhere.

Eventually it would be nice to have a more control flow-aware analysis, possibly by rewriting it as a MIR lint, but that's more complicated and I wanted this fixed first.

changelog: [`missing_asserts_for_indexing`]: accept `assert!`s from parent blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows
Projects
None yet
Development

No branches or pull requests

7 participants