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

Investigate replacing File.getCanonicalPath with Path.getRealPath() #2419

Open
quintesse opened this issue Nov 7, 2015 · 5 comments
Open
Milestone

Comments

@quintesse
Copy link
Member

See #2416 but in short @lukehutch mentioned:

"As an example of what could go wrong, imagine a project root at MyProject/source that has a symlink from MyProject/source/com/xyz/package to MyOtherProject/source/com/xyz/package. Calling getCanonicalPath() will cause the source links to be based in MyOtherProject, while the source root is still in MyProject (because the source root occurs above the symlink).

I think the right thing to do is to never follow symlinks when absolutizing paths. You still need to handle ".." etc.

The "One Right Way" to handle paths in JDK >= 1.7 is using Path, not File. I recently switched a bunch of my own code from File to Path, and it's significantly nicer, more robust, more portable and safer to do things the Path way.

In this case, I think what you want is Path.toRealPath(LinkOption.NOFOLLOW_LINKS)."

@lukehutch
Copy link

I just wanted to add that the current path handling code in Ceylon jumps numerous times back and forth between File and String while working with paths. This is exactly the use case that Path was created for.

You will find basePath.resolve(relativeOrAbsolutePath) useful too, it does a lot of work that is re-implemented in several places in the Ceylon code.

If you want to add the ability to resolve file:// URLs as paths, you need to do some trickier things to get correct Windows support while handling possibly-broken mixes of file:// URLs and system-dependent path formats. The correct recipe is:

basePath.resolve(Paths.get(new URL(pathStr).toURI())).toRealPath(LinkOption.NOFOLLOW_LINKS)

i.e. the recommended way of handling file:// URLs is to convert from URL -> URI -> Path. (Reference 1) ; (Reference 2)

@lukehutch
Copy link

I just realized my own project FastClasspathScanner has this same bug, where it resolved symlinks, and then tried to resolve paths relative to each other while expecting that the paths had not jumped to somewhere else in the file hierarchy. (Had to add NOFOLLOW_LINKS there.)

@quintesse
Copy link
Member Author

Thanks for the inf Luke!

@lukehutch
Copy link

I should add that you need to search the source for both "CanonicalPath" and "CanonicalFile" to find all the places that need to have symlink resolution removed. It was only about a dozen changes, last time I looked.

Switching the source loader and the module loader APIs to use Path objects rather than String or File objects should help find a lot of the places in the code that could be switched to using Path (in order to remove the duplicated code for doing Path.resolve() and related operations, and to make path handling uniform).

@quintesse
Copy link
Member Author

@lukehutch deprecated repository ;)

@ceylon ceylon locked and limited conversation to collaborators Mar 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants