Skip to content

Relative paths on Windows: when to resolve . and .. ? #4659

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
squeek502 opened this issue Mar 6, 2020 · 6 comments · Fixed by #8879
Closed

Relative paths on Windows: when to resolve . and .. ? #4659

squeek502 opened this issue Mar 6, 2020 · 6 comments · Fixed by #8879
Labels
os-windows standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@squeek502
Copy link
Collaborator

squeek502 commented Mar 6, 2020

Related to #4605, #4655, #4658

From IRC (talking about #4655):

<daurnimator> squeek502_: so realpath is inherently racy
<daurnimator> you need to do it inside of the open call.
<daurnimator> (on windows essentially everything is done by opening)
<daurnimator> e.g. to delete a file you open a file; set a flag that says "delete this file when I close it" and then close the file handle
<squeek502_> which open call?
<squeek502_> the open within readFileAlloc?
<daurnimator> yep
<squeek502_> so you're saying readFileAlloc should handle the path resolution on its own?
<daurnimator> yes
<squeek502_> it does seem like there needs to be a decision about what's valid to pass to the various fs functions, its unclear if .\file is expected to always be a valid input to any fs function on Windows or if it should be resolved before certain fs functions
<daurnimator> indeed

In the following test code:

const std = @import("std");

test "windows open file" {
    var first = try std.fs.cwd().openFile("test.zig", .{});
    first.close();

    var second = try std.fs.cwd().openFile(".\\test.zig", .{});
    second.close();
}

If test.zig exists, the first open succeeds, but the second fails, even though they would resolve to the same path. This is because the file path is never resolved and when .\test.zig is passed as the ObjectName to NtCreateFile, it errors with STATUS_OBJECT_NAME_INVALID. The same thing happens if a path with .. is given as the ObjectName.

It's unclear where this resolution should happen, exactly, and the current fs docs don't specify which types of paths each function accepts.

@andrewrk andrewrk added os-windows standard library This issue involves writing Zig code for the standard library. labels Mar 7, 2020
@andrewrk andrewrk added this to the 0.7.0 milestone Mar 7, 2020
@BarabasGitHub
Copy link
Contributor

It seems to be broken still or again on 0.6.0+f595545c1.
Using

const file = try std.fs.cwd().openFile(file_name, .{ .read = true });

where file_name is ./local_file or .\local_file or ../other_file. I don't have time to investigate more, but this is the output I get from my program:

error: FileNotFound
C:\Users\Bas\lib\zig\std\os\windows.zig:166:39: 0x7ff6ffc8e9e6 in std.os.windows.OpenFile (DynamicStory.obj)
            .OBJECT_PATH_NOT_FOUND => return error.FileNotFound,
                                      ^
C:\Users\Bas\lib\zig\std\fs.zig:742:23: 0x7ff6ffc69beb in std.fs.Dir::std.fs.Dir.openFileW (DynamicStory.obj)
            .handle = try os.windows.OpenFile(sub_path_w, .{
                      ^
C:\Users\Bas\lib\zig\std\fs.zig:652:13: 0x7ff6ffc719c8 in std.fs.Dir::std.fs.Dir.openFile (DynamicStory.obj)
            return self.openFileW(path_w.span(), flags);
            ^
E:\Projects\DynamicStory\main.zig:21:18: 0x7ff6ffc6390f in main (DynamicStory.obj)
    const file = try std.fs.cwd().openFile(file_name, .{ .read = true });

@squeek502
Copy link
Collaborator Author

@BarabasGitHub this was never addressed; it is separate from #4605. Status quo is that you must resolve . and .. in paths before passing them to openFile (see the description of #4655 and the fix).

@BarabasGitHub
Copy link
Contributor

@squeek502 Oh jeez. Who could have thought that the 'simple' thing of opening a file can be so complicated?

And looking at it I don't even understand how it works. Am I supposed to pass the relative path to realpath or the absolute path? I can't see how the relative path would work in a sane way, but it seems that's what's done? The real path returns the absolute path, but it's still used in std.fs.cwd().readFileAlloc.

Sorry, I'm really confused and somewhere I believe it shouldn't be this hard to open a file with a relative path (which is the default you get on the command line in windows (powershell). It autocompletes to something like .\my_file).

@squeek502
Copy link
Collaborator Author

Seems like @andrewrk intends on allowing . and .. to be passed to any fs function, so this can basically be considered a bug. From #5187:

I also plan on looking into making "." and ".." work in the code that calls NtCreateFile but I agree with you that can be a separate issue.


The real path returns the absolute path, but it's still used in std.fs.cwd().readFileAlloc

This is just how std.fs works. If you look at something like std.fs.openFileAbsolute, you'll notice that it's actually just a wrapper for cwd().openFile. As I understand it, everything in fs basically works off having a Dir and the only Dir that is reliably available on all platforms is cwd, so that's what's used. The actual implementation for a given platform might not end up using the Dir when given absolute paths, but std.fs is designed to expect a valid Dir for all fs calls regardless.

@BarabasGitHub
Copy link
Contributor

This is just how std.fs works

Yes and it's weird and confusing if you ask me. The code in question looks like it's opening a file from the current working directory, but actually it has nothing whatsoever to do with the current working directory.

Seems like @andrewrk intends on allowing . and .. to be passed to any fs function, so this can basically be considered a bug. From #5187:

That's good. It should 'just work'. On the other hand... I can't find anything about how the C library handles it. It seems to be implementation defined. So it's clearly not an easy issue.

@squeek502
Copy link
Collaborator Author

This might be resolved with #7664. Could confirm by adding a test case that exercises the full std.fs API using . and .. in all the paths to confirm.

@andrewrk andrewrk modified the milestones: 0.10.0, 0.8.0 May 22, 2021
squeek502 added a commit to squeek502/zig that referenced this issue May 23, 2021
These tests check that . and .. are able to be handled by the std.fs functions. Before ziglang#7664, these tests would have failed on Windows.

Closes ziglang#4659
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os-windows standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants