`OpenDirOptions.no_follow` behaves inconsistently between platforms

[squeek502]

trafficstars

The no_follow field was added to OpenDirOptions in e0b77a6b77614538d18b9f0b9e3e7e434ccee4ff by @kubkon to avoid deleting the target of symlinks during deleteTree:

Ensure Dir.deleteTree does not dereference symlinks

Otherwise, the behaviour can lead to unexpected results, resulting in removing an entire tree that's not necessarily under the root. Furthermore, this change is needed if are to properly handle dir symlinks on Windows. Without explicitly requiring that a directory or file is opened with FILE_OPEN_REPARSE_POINT, Windows automatically dereferences all symlinks along the way. This commit adds another option to OpenDirOptions, namely .no_follow, which defaults to false and can be used to specifically open a directory symlink on Windows or call openat with O_NOFOLLOW flag in POSIX.

However, O_NOFOLLOW and FILE_OPEN_REPARSE_POINT behave differently:

On Linux, it will cause opening a symlink to fail:

O_NOFOLLOW If pathname is a symbolic link, then the open fails.

On Windows, it will successfully return a HANDLE to the symlink itself:

FILE_FLAG_OPEN_REPARSE_POINT Normal reparse point processing will not occur; CreateFile will attempt to open the reparse point.

However, on Linux when O_NOFOLLOW is combined with O_PATH it is possible to get a file descriptor to the symlink itself (see "Obtaining a file descriptor that refers to a symbolic link" of https://man7.org/linux/man-pages/man7/symlink.7.html), but since Dir.openDir uses O_DIRECTORY, it will currently still fail with ENOTDIR.

On Darwin/OpenBSD, O_PATH doesn't exist and there is no way to get a file descriptor of a symlink itself AFAICT, so standardizing on getting no_follow to return a file descriptor for the symlink itself isn't viable.

---

It seems like the cross-platform way to go here would be to make no_follow always try to emulate the O_NOFOLLOW behavior. That is, it should return an error if the pathname is a symlink. This is not currently the case for Windows at least.

The path forward as I see it:

• Add tests for the desired deleteDir behavior around symlinks, assuming no_follow will return an error on all platforms if the path is a symlink
• Ensure that O_NOFOLLOW can be emulated in whatever ways are relevant for deleteDir on all supported platforms (it's not a POSIX thing AFAICT, so some POSIX systems may not support it)
• Make the tests pass for all supported platforms

[matu3ba]

Make the tests pass for all supported platforms

1. What tests? Sounds like you've already started.
2. What is the current behavior for hard links/junctions?

[squeek502]

Haven't started yet, but the test I'm imagining right now:

• Create a directory a and a directory b. Create a symlink at a/b pointing to the b directory. Call deleteTree on a and then make sure that the a directory and the symlink inside of it were deleted, but that the actual b directory was not deleted.

The idea is to test this part of the original motivation for no_follow:

Ensure Dir.deleteTree does not dereference symlinks

Otherwise, the behaviour can lead to unexpected results, resulting in removing an entire tree that's not necessarily under the root.

Not sure about hard links.

[squeek502]

Some potential test cases (added to std/fs/test.zig). These both currently pass on Linux. Haven't tested them on Windows yet.

test "deleteTree does not follow symlinks" {
    var tmp = tmpDir(.{});
    defer tmp.cleanup();

    try tmp.dir.makePath("b");
    {
        var a = try tmp.dir.makeOpenPath("a", .{});
        defer a.close();

        a.symLink("../b", "b", .{ .is_directory = true }) catch |err| switch (err) {
            // Symlink requires admin privileges on windows, so this test can legitimately fail.
            error.AccessDenied => return error.SkipZigTest,
            else => return err,
        };
    }

    try tmp.dir.deleteTree("a");

    try testing.expectError(error.FileNotFound, tmp.dir.access("a", .{}));
    try tmp.dir.access("b", .{});
}

test "deleteTree on a symlink" {
    var tmp = tmpDir(.{});
    defer tmp.cleanup();

    // Symlink to a file
    try tmp.dir.writeFile("file", "");
    tmp.dir.symLink("file", "filelink", .{}) catch |err| switch (err) {
        // Symlink requires admin privileges on windows, so this test can legitimately fail.
        error.AccessDenied => return error.SkipZigTest,
        else => return err,
    };

    try tmp.dir.deleteTree("filelink");
    try testing.expectError(error.FileNotFound, tmp.dir.access("filelink", .{}));
    try tmp.dir.access("file", .{});

    // Symlink to a directory
    try tmp.dir.makePath("dir");
    tmp.dir.symLink("dir", "dirlink", .{ .is_directory = true }) catch |err| switch (err) {
        // Symlink requires admin privileges on windows, so this test can legitimately fail.
        error.AccessDenied => return error.SkipZigTest,
        else => return err,
    };

    try tmp.dir.deleteTree("dirlink");
    try testing.expectError(error.FileNotFound, tmp.dir.access("dirlink", .{}));
    try tmp.dir.access("dir", .{});
}

[nacho00112]

I want that syntax hightlighting that GitHub provides for Zig in my Vim. Do anyone here know how can I achieve that?

[matu3ba]

I want that syntax hightlighting that GitHub provides for Zig in my Vim.

This is an issue tracker, not QA. Please ask future questions in a community place. https://github.com/ziglang/zig.vim

[squeek502]

The above two tests also pass on Windows currently (note that I initially mixed up the setting of is_directory, so make sure you get the updated version). This is because deleteTree doesn't actually depend on the .no_follow behavior at all in normal usage:

• deleteTree always tries to delete the initial path as a file first, which will succeed on symlinks because deleteFile doesn't follow symlinks
• deleteTree when iterating will get the type of directory symlinks as .sym_link, not as .directory, meaning it will never try to open it as a directory. This may not be true of all platforms, but it is true of Windows and Linux.

So the tests above aren't actually testing the .no_follow flag in a meaningful way in practice. They are still good tests to have, though. And no_follow might still be good to have set for deleteTree as a fail-safe against bugs/race-conditions that lead to following directory symlinks.

To that end, here's a test that tests .no_follow directly and will actually fail on Windows:

test "OpenDirOptions.no_follow" {
    var tmp = tmpDir(.{});
    defer tmp.cleanup();

    try tmp.dir.makePath("dir");
    tmp.dir.symLink("dir", "dirlink", .{ .is_directory = true }) catch |err| switch (err) {
        // Symlink requires admin privileges on windows, so this test can legitimately fail.
        error.AccessDenied => return error.SkipZigTest,
        else => return err,
    };

    // Open the symlink with only no_follow
    {
        var dir_or_err = tmp.dir.openDir("dirlink", .{ .no_follow = true });
        defer if (dir_or_err) |*dir| dir.close() else |_| {};
        // TODO: This might want to expect a different error. This NotDir is only returned
        // on Linux due to the O_PATH | O_NOFOLLOW | O_DIRECTORY combination currently.
        try std.testing.expectError(error.NotDir, dir_or_err);
    }

    // Open the symlink with no_follow and iteration permissions requested
    {
        var dir_or_err = tmp.dir.openDir("dirlink", .{ .no_follow = true, .iterate = true });
        defer if (dir_or_err) |*dir| dir.close() else |_| {};
        try std.testing.expectError(error.NotDir, dir_or_err);
    }
}

Both of these openDir("dirlink", ...) calls will unexpectedly succeed on Windows:

Test [41/60] test.OpenDirOptions.no_follow... expected error.NotDir, found fs.Dir{ .fd = anyopaque@cc }
Test [41/60] test.OpenDirOptions.no_follow... FAIL (TestUnexpectedError)
C:\Users\Ryan\Programming\Zig\zig\lib\std\testing.zig:40:9: 0x7ff7160eece5 in expectError__anon_21108 (test.exe.obj)
        return error.TestUnexpectedError;
        ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\fs\test.zig:244:9: 0x7ff7160eef68 in test.OpenDirOptions.no_follow (test.exe.obj)
        try std.testing.expectError(error.NotDir, dir_or_err);
        ^