Skip to content

fmt: Fix relative paths with . and .. on Windows #4655

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

Closed
wants to merge 1 commit into from

Conversation

squeek502
Copy link
Collaborator

@squeek502 squeek502 commented Mar 6, 2020

This is one possible fix for #4605. It's sort-of a band-aid fix due to NtCreateFile failing on paths with . or .. in them, so this resolves the paths via realpath before formatting them. The other possible fix would be to allow . and .. to be passed to NtCreateFile callers and deal with them appropriately within those functions (see #4659).

Note: using realpath has a potential side-benefit of avoiding double-formatting paths that would resolve to the same path, since the real path is now used in the seen map (so zig fmt ./test.zig test.zig will now skip the second path since they both resolve to the same path).

On Windows, here is before:

> zig fmt .\test.zig
unable to open '.\test.zig': error.FileNotFound
> zig fmt ./test.zig
unable to open './test.zig': error.BadPathName

and after:

> zig fmt .\test.zig
.\test.zig
> zig fmt ./test.zig
unable to open './test.zig': error.BadPathName

Closes #4605 if merged.


EDIT: See #4655 (comment) for "an attempt to make it easier to make a decision about [merging this]"

@daurnimator
Copy link
Contributor

This is not correct: you need to actually process things with NtOpenPath and navigating up a directory. Otherwise symlinks get entirely different behaviour.

@squeek502
Copy link
Collaborator Author

squeek502 commented Mar 6, 2020

@daurnimator would that be a bug in the Windows realpath implementation, then? See the realpath doc comments:

zig/lib/std/os.zig

Lines 3027 to 3032 in eaccfff

/// Return the canonicalized absolute pathname.
/// Expands all symbolic links and resolves references to `.`, `..`, and
/// extra `/` characters in `pathname`.
/// The return value is a slice of `out_buffer`, but not necessarily from the beginning.
/// See also `realpathC` and `realpathW`.
pub fn realpath(pathname: []const u8, out_buffer: *[MAX_PATH_BYTES]u8) RealPathError![]u8 {

To be sure, you're thinking of a situation where the .. points to a directory that is a symlink to a different directory? I'm not sure how exactly to set that situation up on Windows. Would appreciate help with that so I can test it.

@daurnimator
Copy link
Contributor

To be sure, you're thinking of a situation where the .. points to a directory that is a symlink to a different directory? I'm not sure how exactly to set that situation up on Windows. Would appreciate help with that so I can test it.

e.g. consider the path C:/foo/bar/../qux.
If bar is a symlink to C:/other then you would end up at C:/other/qux. With realpath you end up at C:/foo/qux instead.

@squeek502
Copy link
Collaborator Author

squeek502 commented Mar 6, 2020

e.g. consider the path C:/foo/bar/../qux.
If bar is a symlink to C:/other then you would end up at C:/other/qux.

Forgive my naivete, but is there precedence for this? cd C:/foo/bar/../qux seems to bring me to C:/foo/qux.

EDIT: Explorer brings me to C:/foo/qux too.

@daurnimator
Copy link
Contributor

e.g. consider the path C:/foo/bar/../qux.
If bar is a symlink to C:/other then you would end up at C:/other/qux.

Uh, I meant it'll take you to C:/qux.

Forgive my naivete, but is there precedence for this? cd C:/foo/bar/../qux seems to bring me to C:/foo/qux.

EDIT: Explorer brings me to C:/foo/qux too.

It's a classic escape technique for attacking e.g. web servers (first example I found on google: https://www.inputzero.io/2019/05/webrick-path-traversal.html)

How did you create the symlink?

Also, try it on a linux machine :)

@squeek502
Copy link
Collaborator Author

squeek502 commented Mar 6, 2020

realpath does seem to handle symlinks for . at least.

C:\tmp> mkdir target
C:\tmp> echo. > target\test.zig
C:\tmp> mklink /J link target
C:\tmp> cd link
C:\tmp\link> zig fmt .\test.zig
openFileWindows ObjectName=\??\C:\tmp\target\test.zig
.\test.zig

(the openFileWindows line is from some debug printing I have locally, it shows the path used in NtCreateFile, so realpath is resolving C:\tmp\link\.\test.zig to C:\tmp\target\test.zig)

I'm still confused about what you're describing. A concrete example would be really helpful (I'm not very knowledgeable about symlinks on any platform).

@squeek502
Copy link
Collaborator Author

squeek502 commented Mar 6, 2020

As far as I can tell, realpath is the correct function to be calling here, given what its doc comments say. Any weirdness with symlinks is something to be fixed in realpath and seems out-of-scope for this PR. I'll open a new issue regarding realpath. EDIT: #4658

@squeek502
Copy link
Collaborator Author

squeek502 commented Mar 29, 2020

This is mergable in my opinion. Here's an attempt to make it easier to make a decision about:

Pros

  • Fixes paths containing . and .. passed to zig fmt on Windows
  • Improves an edge case where paths could be double-formatted when multiple paths that resolve to the same file are passed to zig fmt (i.e. zig fmt test.zig ./test.zig previously would have formatted test.zig twice, whereas now it caches the absolute/realpath, so it recognizes that they are the same file)

Cons

  • Introduces some potential unnecessary work, since the path does not need to be resolved ahead-of-time on non-Windows platforms

Notes on symlinks

  • std.fs.realpath might handle symlinks differently in the future depending on how std.fs.realpath bugs/inconsistencies on Windows #4658 is resolved. Right now, the status quo is:
    • On Linux, std.fs.realpath resolves symlinks before ... This matches the system behavior as far as I can tell (cat link/../file will output the contents of linked/../file and fail if it doesn't exist), although there seem to be some edge cases (from my testing, if link is a symlink to linked, then cd link/../dir takes you to ./dir if it exists [ignoring the symlink], otherwise it will take you to linked/../dir [resolving the symlink before ..]).
    • On Windows, std.fs.realpath resolves .. before symlinks. This matches the system behavior as far as I can tell (cd link\..\dir will never take you to linked\..\dir; if .\dir does not exist, it will fail with "The system cannot find the path specified.". Same deal with type link\..\file). Please correct me if I'm wrong on this, but I'm not even sure if there exists a function in the Windows API that resolves symlinks before ... @daurnimator has mentioned that Zig might need something like RtlDosPathNameToRelativeNtPathName_U_WithStatus but from using the test code at the bottom of this article, that function does not resolve symlinks before .. either. If there is precedence for Linux-like symlink resolution on Windows, it would be helpful to get that information added to std.fs.realpath bugs/inconsistencies on Windows #4658

Overall, I think the con is fairly minor and the current symlink-handling of std.fs.realpath is good enough (i.e. it likely does what the user would expect for their platform).

This is a band-aid fix due to NtCreateFile failing on paths with . or .. in them.
@andrewrk
Copy link
Member

=> #5187

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 this pull request may close these issues.

Zig fmt fails to open local-path files
3 participants