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

std::path::posix::Path should not automatically normalize #14028

Closed
alanfalloon opened this issue May 8, 2014 · 5 comments · Fixed by #21759
Closed

std::path::posix::Path should not automatically normalize #14028

alanfalloon opened this issue May 8, 2014 · 5 comments · Fixed by #21759

Comments

@alanfalloon
Copy link

Because of symlinks, on POSIX systems, in general you can't claim that foo/bar/../baz is the same path as foo/baz. If foo/bar is a symlink with target flip/flop then the abspath of foo/bar/../baz is actually foo/flip/baz. Normalizing the path symbolically changes the meaning of the path. (reducing // to / is still valid though)

Other issues like #11650 are symptoms of this issue, execvp is not the only call that changes behaviour when path components are stripped out.

The std::path::posix::Path module needs to leave the paths un-normalized internally, and re-introduce an explicit normpath method (distinct from os::make_absolute for the reasons outlined above).

@huonw
Copy link
Member

huonw commented May 8, 2014

cc @kballard

@huonw huonw added the A-libs label May 8, 2014
@lilyball
Copy link
Contributor

lilyball commented May 8, 2014

Crap. You're right. Although I think automatically normalizing ./foo to foo is still valid, the only invalid operation is automatically interpreting .. segments. This means that #11650 is still valid for Process::new(), because it does need to treat ./foo and foo differently, although the dynamic_library::open_external part will work correctly with Paths if this is fixed.

This is unfortunate, because automatically normalizing paths made a lot of things work better in the path library. But I think we can leave the current implementation in place as long as we're ok with e.g. .filename() returning .. (which seems like reasonable behavior).

I guess this means the path library will partially-normalize on Path creation, making safe changes like squashing // to / and removing . components (e.g. everything it does now except for handling of .. components), and it will expose a .normalize() method that handles .. components.

@alanfalloon
Copy link
Author

Sounds right to me.

I can't think of a time when normalizing ./ components will bite you (except in cases like execvp where having any directory component at all has a different meaning than a single name). I suppose technically, the . entry could point to a different inode than the current directory, but I think we could reasonably consider that filesystem to be broken.

@alanfalloon
Copy link
Author

As a side-note, I made a comment on #13721 about testing being harder due to lack of Show impl.

I agree with the rationale for closing #13009, but if there is some way we can have a Show impl that is on with #[cfg(test)] we will then be able to use assert_eq! and friends in our testing, which would be a big help. I'm not sure if you can bubble up config options like test to external crates, so that might not be possible, but perhaps we can come up with something.

@lilyball
Copy link
Contributor

lilyball commented May 8, 2014

There's no way for libstd to vend a Show impl that's only with with #[cfg(test)]. Configurations don't work that way.

As I just suggested in #13721, you can still use assert_eq!() if you map your path to a byte vector (&[u8] or Vec<u8> depending on lifetime requirements).

alexcrichton added a commit to alexcrichton/rust that referenced this issue Feb 4, 2015
This PR implements [path reform](rust-lang/rfcs#474), and motivation and details for the change can be found there.

For convenience, the old path API is being kept as `old_path` for the time being. Updating after this PR is just a matter of changing imports to `old_path` (which is likely not needed, since the prelude entries still export the old path API).

This initial PR does not include additional normalization or platform-specific path extensions. These will be done in follow up commits or PRs.

[breaking-change]

Closes rust-lang#20034
Closes rust-lang#12056
Closes rust-lang#11594
Closes rust-lang#14028
Closes rust-lang#14049
Closes rust-lang#10035
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants