From c63897b0d5bf192ac8bf81366ae0bf9ca86b3f3b Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Tue, 25 Apr 2023 15:40:58 -0700 Subject: [PATCH 1/2] fs.Dir.deleteTree: Add configurable max_retries to avoid infinite loops Previously, it was possible for deleteTree to get into a state where it would continually retry deleting the same directory over and over infinitely, since it would think all of the children were successfully deleted but when deleting the directory it would hit `error.DirNotEmpty`. Now, there is a (user-configurable) limit to the number of times it will retry deleting directories, and return `error.TooManyRetries` if the limit is exceeded. Closes #15465 --- doc/docgen.zig | 2 +- lib/std/Build.zig | 2 +- lib/std/Build/RemoveDirStep.zig | 2 +- lib/std/fs.zig | 57 ++++++++++++++++++++++++++------- lib/std/fs/test.zig | 52 +++++++++++++++++++++++++----- lib/std/fs/watch.zig | 2 +- lib/std/testing.zig | 4 +-- src/Package.zig | 2 +- src/link.zig | 4 +-- tools/update_cpu_features.zig | 2 +- 10 files changed, 99 insertions(+), 30 deletions(-) diff --git a/doc/docgen.zig b/doc/docgen.zig index 07636fd1527a..c2fef8234984 100644 --- a/doc/docgen.zig +++ b/doc/docgen.zig @@ -99,7 +99,7 @@ pub fn main() !void { var toc = try genToc(allocator, &tokenizer); try fs.cwd().makePath(tmp_dir_name); - defer fs.cwd().deleteTree(tmp_dir_name) catch {}; + defer fs.cwd().deleteTree(tmp_dir_name, .{}) catch {}; try genHtml(allocator, &tokenizer, &toc, buffered_writer.writer(), zig_exe, opt_zig_lib_dir, do_code_tests); try buffered_writer.flush(); diff --git a/lib/std/Build.zig b/lib/std/Build.zig index 34806adf488f..ca043b2c770b 100644 --- a/lib/std/Build.zig +++ b/lib/std/Build.zig @@ -776,7 +776,7 @@ fn makeUninstall(uninstall_step: *Step, prog_node: *std.Progress.Node) anyerror! if (self.verbose) { log.info("rm {s}", .{full_path}); } - fs.cwd().deleteTree(full_path) catch {}; + fs.cwd().deleteTree(full_path, .{}) catch {}; } // TODO remove empty directories diff --git a/lib/std/Build/RemoveDirStep.zig b/lib/std/Build/RemoveDirStep.zig index a5bf3c32565d..fbbf69a51238 100644 --- a/lib/std/Build/RemoveDirStep.zig +++ b/lib/std/Build/RemoveDirStep.zig @@ -28,7 +28,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { const b = step.owner; const self = @fieldParentPtr(RemoveDirStep, "step", step); - b.build_root.handle.deleteTree(self.dir_path) catch |err| { + b.build_root.handle.deleteTree(self.dir_path, .{}) catch |err| { if (b.build_root.path) |base| { return step.fail("unable to recursively delete path '{s}/{s}': {s}", .{ base, self.dir_path, @errorName(err), diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 290eb151f7bf..d1c17c3bdb8b 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2089,6 +2089,10 @@ pub const Dir = struct { FileBusy, DeviceBusy, + /// The number of retries to delete thought-to-be-empty directories + /// exceeded `max_retries`. + TooManyRetries, + /// One of the path components was not a directory. /// This error is unreachable if `sub_path` does not contain a path separator. NotDir, @@ -2101,17 +2105,33 @@ pub const Dir = struct { BadPathName, } || os.UnexpectedError; + pub const DeleteTreeOptions = struct { + /// After successfully deleting all of a directories children, it is possible for + /// the directory to not actually be empty (either from a new child being added while + /// iterating or a child being in a `DELETE_PENDING` state on Windows). In this scenario, + /// the directory will be iterated again and then will be attempted to be deleted again. + /// This provides a limit to the total number of such retries. + /// In `deleteTree`, the retry limit is per-directory. + /// In `deleteTreeMinStackSize`, the retry limit is function-wide. + max_retries: usize = 4, + + /// When initially trying to delete `sub_path`, it will first try and delete it as + /// the provided kind. + kind_hint: File.Kind = .File, + }; + /// Whether `full_path` describes a symlink, file, or directory, this function /// removes it. If it cannot be removed because it is a non-empty directory, /// this function recursively removes its entries and then tries again. /// This operation is not atomic on most file systems. - pub fn deleteTree(self: Dir, sub_path: []const u8) DeleteTreeError!void { - var initial_iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, .File)) orelse return; + pub fn deleteTree(self: Dir, sub_path: []const u8, options: DeleteTreeOptions) DeleteTreeError!void { + var initial_iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, options.kind_hint)) orelse return; const StackItem = struct { name: []const u8, parent_dir: Dir, iter: IterableDir.Iterator, + retries_remaining: usize, }; var stack = std.BoundedArray(StackItem, 16){}; @@ -2125,6 +2145,7 @@ pub const Dir = struct { .name = sub_path, .parent_dir = self, .iter = initial_iterable_dir.iterateAssumeFirstIteration(), + .retries_remaining = options.max_retries, }); process_stack: while (stack.len != 0) { @@ -2162,10 +2183,14 @@ pub const Dir = struct { .name = entry.name, .parent_dir = top.iter.dir, .iter = iterable_dir.iterateAssumeFirstIteration(), + .retries_remaining = options.max_retries, }); continue :process_stack; } else |_| { - try top.iter.dir.deleteTreeMinStackSizeWithKindHint(entry.name, entry.kind); + try top.iter.dir.deleteTreeMinStackSize(entry.name, .{ + .max_retries = options.max_retries, + .kind_hint = entry.kind, + }); break :handle_entry; } } else { @@ -2207,6 +2232,7 @@ pub const Dir = struct { // pop the value from the stack. const parent_dir = top.parent_dir; const name = top.name; + const retries_remaining = top.retries_remaining; _ = stack.pop(); var need_to_retry: bool = false; @@ -2217,6 +2243,7 @@ pub const Dir = struct { }; if (need_to_retry) { + if (retries_remaining == 0) return error.TooManyRetries; // Since we closed the handle that the previous iterator used, we // need to re-open the dir and re-create the iterator. var iterable_dir = iterable_dir: { @@ -2282,6 +2309,7 @@ pub const Dir = struct { .name = name, .parent_dir = parent_dir, .iter = iterable_dir.iterateAssumeFirstIteration(), + .retries_remaining = retries_remaining - 1, }); continue :process_stack; } @@ -2290,13 +2318,10 @@ pub const Dir = struct { /// Like `deleteTree`, but only keeps one `Iterator` active at a time to minimize the function's stack size. /// This is slower than `deleteTree` but uses less stack space. - pub fn deleteTreeMinStackSize(self: Dir, sub_path: []const u8) DeleteTreeError!void { - return self.deleteTreeMinStackSizeWithKindHint(sub_path, .File); - } - - fn deleteTreeMinStackSizeWithKindHint(self: Dir, sub_path: []const u8, kind_hint: File.Kind) DeleteTreeError!void { + pub fn deleteTreeMinStackSize(self: Dir, sub_path: []const u8, options: DeleteTreeOptions) DeleteTreeError!void { + var retries_remaining = options.max_retries; start_over: while (true) { - var iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, kind_hint)) orelse return; + var iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, options.kind_hint)) orelse return; var cleanup_dir_parent: ?IterableDir = null; defer if (cleanup_dir_parent) |*d| d.close(); @@ -2386,14 +2411,22 @@ pub const Dir = struct { if (cleanup_dir_parent) |d| { d.dir.deleteDir(dir_name) catch |err| switch (err) { // These two things can happen due to file system race conditions. - error.FileNotFound, error.DirNotEmpty => continue :start_over, + error.FileNotFound, error.DirNotEmpty => { + if (retries_remaining == 0) return error.TooManyRetries; + retries_remaining -= 1; + continue :start_over; + }, else => |e| return e, }; continue :start_over; } else { self.deleteDir(sub_path) catch |err| switch (err) { error.FileNotFound => return, - error.DirNotEmpty => continue :start_over, + error.DirNotEmpty => { + if (retries_remaining == 0) return error.TooManyRetries; + retries_remaining -= 1; + continue :start_over; + }, else => |e| return e, }; return; @@ -2834,7 +2867,7 @@ pub fn deleteTreeAbsolute(absolute_path: []const u8) !void { var dir = try cwd().openDir(dirname, .{}); defer dir.close(); - return dir.deleteTree(path.basename(absolute_path)); + return dir.deleteTree(path.basename(absolute_path), .{}); } /// Same as `Dir.readLink`, except it asserts the path is absolute. diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index c953b529fa18..bd3322a1fc49 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -304,7 +304,7 @@ test "Dir.Iterator but dir is deleted during iteration" { // This is a contrived reproduction, but this could happen outside of the program, in another thread, etc. // If we get an error while trying to delete, we can skip this test (this will happen on platforms // like Windows which will give FileBusy if the directory is currently open for iteration). - tmp.dir.deleteTree("subdir") catch return error.SkipZigTest; + tmp.dir.deleteTree("subdir", .{}) catch return error.SkipZigTest; // Now, when we try to iterate, the next call should return null immediately. const entry = try iterator.next(); @@ -696,7 +696,7 @@ test "makePath, put some files in it, deleteTree" { try tmp.dir.makePath("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c" ++ fs.path.sep_str ++ "file.txt", "nonsense"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "file2.txt", "blah"); - try tmp.dir.deleteTree("os_test_tmp"); + try tmp.dir.deleteTree("os_test_tmp", .{}); if (tmp.dir.openDir("os_test_tmp", .{})) |dir| { _ = dir; @panic("expected error"); @@ -712,7 +712,7 @@ test "makePath, put some files in it, deleteTreeMinStackSize" { try tmp.dir.makePath("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c" ++ fs.path.sep_str ++ "file.txt", "nonsense"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "file2.txt", "blah"); - try tmp.dir.deleteTreeMinStackSize("os_test_tmp"); + try tmp.dir.deleteTreeMinStackSize("os_test_tmp", .{}); if (tmp.dir.openDir("os_test_tmp", .{})) |dir| { _ = dir; @panic("expected error"); @@ -721,12 +721,48 @@ test "makePath, put some files in it, deleteTreeMinStackSize" { } } +test "deleteTree max retries" { + // While TooManyRetries is relevant for non-Windows platforms, it is trickier to + // reproduce the necessary conditions, as getting DirNotEmpty when deleting a directory + // that `deleteTree` thinks is now empty would likely mean something like a file/dir being + // created while the directory is being iterated and its children deleted. + if (builtin.os.tag != .windows) return error.SkipZigTest; + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + + // Make a subdirectory with a file in it and keep the file handle open. + var leaked_handle = blk: { + var dir = try tmp.dir.makeOpenPath("dir", .{}); + defer dir.close(); + + var file = try dir.createFile("neverclose", .{}); + break :blk file; + }; + + { + // We always want to close the file handle even on test failure, but we want it to be + // open for these expectError calls. + defer leaked_handle.close(); + + // Since the file handle was never closed, it will enter into a `DELETE_PENDING` + // state which will cause deleteTree to continually delete the file and directory, + // getting 'success' from the file delete but then DirNotEmpty from the directory + // delete. + try std.testing.expectError(error.TooManyRetries, tmp.dir.deleteTree("dir", .{})); + try std.testing.expectError(error.TooManyRetries, tmp.dir.deleteTreeMinStackSize("dir", .{})); + } + + // Now that the leaked handle has been closed, deleteTree should succeed. + try tmp.dir.deleteTree("dir", .{}); +} + test "makePath in a directory that no longer exists" { if (builtin.os.tag == .windows) return error.SkipZigTest; // Windows returns FileBusy if attempting to remove an open dir var tmp = tmpDir(.{}); defer tmp.cleanup(); - try tmp.parent_dir.deleteTree(&tmp.sub_path); + try tmp.parent_dir.deleteTree(&tmp.sub_path, .{}); try testing.expectError(error.FileNotFound, tmp.dir.makePath("sub-path")); } @@ -751,7 +787,7 @@ fn testFilenameLimits(iterable_dir: IterableDir, maxed_filename: []const u8) !vo } // ensure that we can delete the tree - try iterable_dir.dir.deleteTree(maxed_filename); + try iterable_dir.dir.deleteTree(maxed_filename, .{}); } test "max file name component lengths" { @@ -874,7 +910,7 @@ test "access file" { try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "file.txt", ""); try tmp.dir.access("os_test_tmp" ++ fs.path.sep_str ++ "file.txt", .{}); - try tmp.dir.deleteTree("os_test_tmp"); + try tmp.dir.deleteTree("os_test_tmp", .{}); } test "sendfile" { @@ -882,7 +918,7 @@ test "sendfile" { defer tmp.cleanup(); try tmp.dir.makePath("os_test_tmp"); - defer tmp.dir.deleteTree("os_test_tmp") catch {}; + defer tmp.dir.deleteTree("os_test_tmp", .{}) catch {}; var dir = try tmp.dir.openDir("os_test_tmp", .{}); defer dir.close(); @@ -947,7 +983,7 @@ test "copyRangeAll" { defer tmp.cleanup(); try tmp.dir.makePath("os_test_tmp"); - defer tmp.dir.deleteTree("os_test_tmp") catch {}; + defer tmp.dir.deleteTree("os_test_tmp", .{}) catch {}; var dir = try tmp.dir.openDir("os_test_tmp", .{}); defer dir.close(); diff --git a/lib/std/fs/watch.zig b/lib/std/fs/watch.zig index 469244fcd640..24174f7107c7 100644 --- a/lib/std/fs/watch.zig +++ b/lib/std/fs/watch.zig @@ -643,7 +643,7 @@ test "write a file, watch it, write it again, delete it" { if (builtin.single_threaded) return error.SkipZigTest; try std.fs.cwd().makePath(test_tmp_dir); - defer std.fs.cwd().deleteTree(test_tmp_dir) catch {}; + defer std.fs.cwd().deleteTree(test_tmp_dir, .{}) catch {}; return testWriteWatchWriteDelete(std.testing.allocator); } diff --git a/lib/std/testing.zig b/lib/std/testing.zig index 37e15ff08b26..64803e1a7062 100644 --- a/lib/std/testing.zig +++ b/lib/std/testing.zig @@ -519,7 +519,7 @@ pub const TmpDir = struct { pub fn cleanup(self: *TmpDir) void { self.dir.close(); - self.parent_dir.deleteTree(&self.sub_path) catch {}; + self.parent_dir.deleteTree(&self.sub_path, .{}) catch {}; self.parent_dir.close(); self.* = undefined; } @@ -535,7 +535,7 @@ pub const TmpIterableDir = struct { pub fn cleanup(self: *TmpIterableDir) void { self.iterable_dir.close(); - self.parent_dir.deleteTree(&self.sub_path) catch {}; + self.parent_dir.deleteTree(&self.sub_path, .{}) catch {}; self.parent_dir.close(); self.* = undefined; } diff --git a/src/Package.zig b/src/Package.zig index 7d98ddaba3b1..721a70648386 100644 --- a/src/Package.zig +++ b/src/Package.zig @@ -719,7 +719,7 @@ fn renameTmpIntoCache( }, error.PathAlreadyExists, error.AccessDenied => { // Package has been already downloaded and may already be in use on the system. - cache_dir.deleteTree(tmp_dir_sub_path) catch |del_err| { + cache_dir.deleteTree(tmp_dir_sub_path, .{}) catch |del_err| { std.log.warn("unable to delete temp directory: {s}", .{@errorName(del_err)}); }; }, diff --git a/src/link.zig b/src/link.zig index 672a53999f5b..9d7637634895 100644 --- a/src/link.zig +++ b/src/link.zig @@ -937,7 +937,7 @@ pub const File = struct { // Work around windows `renameW` can't fail with `PathAlreadyExists` // See https://github.com/ziglang/zig/issues/8362 if (cache_directory.handle.access(o_sub_path, .{})) |_| { - try cache_directory.handle.deleteTree(o_sub_path); + try cache_directory.handle.deleteTree(o_sub_path, .{}); continue; } else |err| switch (err) { error.FileNotFound => {}, @@ -961,7 +961,7 @@ pub const File = struct { o_sub_path, ) catch |err| switch (err) { error.PathAlreadyExists => { - try cache_directory.handle.deleteTree(o_sub_path); + try cache_directory.handle.deleteTree(o_sub_path, .{}); continue; }, else => |e| return e, diff --git a/tools/update_cpu_features.zig b/tools/update_cpu_features.zig index dd1b96fa7c95..fd7ab48bef7c 100644 --- a/tools/update_cpu_features.zig +++ b/tools/update_cpu_features.zig @@ -1198,7 +1198,7 @@ fn processOneTarget(job: Job) anyerror!void { if (all_features.items.len == 0) { // We represent this with an empty file. - try target_dir.deleteTree(zig_code_basename); + try target_dir.deleteTree(zig_code_basename, .{}); return; } From bd5080c0dad38044b1b19e49634752a4ce5c6e56 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 26 Apr 2023 03:56:14 -0700 Subject: [PATCH 2/2] Make deleteTreeMinStackSize's max_retries effectively per-directory It's not quite the same as deleteTree since it doesn't keep track of the stack of directories that have been tried, but in normal conditions (i.e. directories aren't being created within the to-be-deleted tree) it should behave similarly. --- lib/std/fs.zig | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index d1c17c3bdb8b..ce16c698adb2 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2111,9 +2111,12 @@ pub const Dir = struct { /// iterating or a child being in a `DELETE_PENDING` state on Windows). In this scenario, /// the directory will be iterated again and then will be attempted to be deleted again. /// This provides a limit to the total number of such retries. - /// In `deleteTree`, the retry limit is per-directory. - /// In `deleteTreeMinStackSize`, the retry limit is function-wide. - max_retries: usize = 4, + /// The retry limit is per-directory. + /// + /// Note: A sufficiently adversarial process could still cause an effectively + /// infinite loop by doing something like continually creating deeply nested directory + /// structures within the to-be-deleted tree as it is being deleted. + max_retries: usize = 3, /// When initially trying to delete `sub_path`, it will first try and delete it as /// the provided kind. @@ -2418,6 +2421,9 @@ pub const Dir = struct { }, else => |e| return e, }; + // Reset retries after successfully deleting a directory so that the + // retries are effectively per-directory. + retries_remaining = options.max_retries; continue :start_over; } else { self.deleteDir(sub_path) catch |err| switch (err) {