-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
base: master
Are you sure you want to change the base?
Conversation
bddeea1
to
5f7301b
Compare
@@ -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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
5f7301b
to
e8f1be1
Compare
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.