Skip to content

rename: Add error.CircularLoop #24159

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gooncreeper
Copy link
Contributor

@gooncreeper gooncreeper commented Jun 12, 2025

Adds error.CircularLoop to rename functions.

For POSIX systems: according to the man page for rename, "EINVAL The old pathname names an ancestor directory of the new pathname, or either pathname argument contains a final component that is dot or dot-dot." The later is always programmer error, so returning error.CircularLoop should always be correct.

For WASI systems: the error should be the same as POSIX. The documentation for INVAL says "Invalid argument, similar to EINVAL in POSIX." This holds true for wasmtime 33.0.0 on my system, however the zig CI returns PERM, so the test is skipped on WASI.

For Windows systems: based on empirical testing, SHARING_VIOLATION is returned on Windows 10 to indicate a circular loop. At some point on Windows 11, INVALID_PARAMATER started being used.

@gooncreeper gooncreeper force-pushed the rename-circular-error branch from bddeea1 to 5f7301b Compare June 12, 2025 16:17
@gooncreeper gooncreeper marked this pull request as draft June 12, 2025 21:31
@@ -2662,7 +2663,7 @@ pub fn renameZ(old_path: [*:0]const u8, new_path: [*:0]const u8) RenameError!voi
.BUSY => return error.FileBusy,
.DQUOT => return error.DiskQuota,
.FAULT => unreachable,
.INVAL => unreachable,
Copy link
Collaborator

@squeek502 squeek502 Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it's (surprisingly) not documented, INVAL is also used when the destination path contains characters that are invalid for the target filesystem (#15607).

Simple reproduction using a FAT filesystem
const std = @import("std");

pub fn main() !void {
    const file = try std.fs.cwd().createFile("blah.txt", .{});
    file.close();

    try std.fs.cwd().rename("blah.txt", "bl|ah.txt");
}
$ ~/Programming/zig/tmp/rename-inval
thread 8954 panic: reached unreachable code
/home/ryan/Programming/zig/zig/lib/std/posix.zig:2782:19: 0x10e0e56 in renameatZ (rename-inval)
        .INVAL => unreachable,
                  ^
/home/ryan/Programming/zig/zig/lib/std/posix.zig:2716:25: 0x10e0bba in renameat (rename-inval)
        return renameatZ(old_dir_fd, &old_path_c, new_dir_fd, &new_path_c);
                        ^
/home/ryan/Programming/zig/zig/lib/std/fs/Dir.zig:1752:26: 0x10df8cb in rename (rename-inval)
    return posix.renameat(self.fd, old_sub_path, self.fd, new_sub_path);
                         ^
/home/ryan/Programming/zig/tmp/rename-inval.zig:7:28: 0x10df6f4 in main (rename-inval)
    try std.fs.cwd().rename("blah.txt", "bl|ah.txt");
                           ^
/home/ryan/Programming/zig/zig/lib/std/start.zig:671:37: 0x10df578 in posixCallMainAndExit (rename-inval)
            const result = root.main() catch |err| {
                                    ^
/home/ryan/Programming/zig/zig/lib/std/start.zig:282:5: 0x10df11d in _start (rename-inval)
    asm volatile (switch (native_arch) {
    ^
???:?:?: 0x0 in ??? (???)
Aborted (core dumped)

I think it might be better to introduce something like error.InvalidLocation as a catch-all for this sort of error. For posix.RenameError I'd imagine it should be documented like so:

    /// Either:
    /// - The old pathname names an ancestor directory of the new pathname
    /// - Either pathname argument contains a final component that is dot or dot-dot
    /// - The new pathname contains characters that are disallowed by the underlying filesystem
    InvalidLocation,

(in my testing if the old path contains invalid characters, error.FileNotFound is returned)

I haven't checked what Windows returns in this scenario, but OBJECT_NAME_INVALID seems likely.

This also may not be a perfect solution though; addressing #15607 fully is likely going to be opening one can of worms after another.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely agree. I will expand this PR (since the test coverage and most of the changes carry over) soon to replace BadPathName with InvalidLocation. One question though, should the final component being . or .. be considered unreachable since it is a checkable condition and hence avoidable by the caller?

Copy link
Collaborator

@squeek502 squeek502 Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I'm aware, the strategy of using unreachable for system errors is no longer the plan: #6389

But either way, I'm unsure what treating it as unreachable would mean in this situation. If it means not documenting it as a possible cause of error.InvalidLocation, then IMO there's no reason to intentionally obfuscate the possibility of trailing . or .. being a cause.

Adds error.CircularLoop to rename functions.

For POSIX systems: according to the man page for rename, "EINVAL The
old pathname names an ancestor directory of the new pathname, or either
pathname argument contains a final component that is dot or dot-dot."
The later is always programmer error, so returning error.CircularLoop
should always be correct.

For WASI systems: the error should be the same as POSIX. The
documentation for INVAL says "Invalid argument, similar to `EINVAL` in
POSIX." This holds true for wasmtime 33.0.0 on my system, however the
zig CI returns PERM, so the test is skipped on WASI.

For Windows systems: based on empirical testing, SHARING_VIOLATION is
returned on Windows 10 to indicate a circular loop. At some point on
Windows 11, INVALID_PARAMATER started being used.
@gooncreeper gooncreeper force-pushed the rename-circular-error branch from 5f7301b to e8f1be1 Compare June 16, 2025 01:33
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.

2 participants