From 911f74e93b15024c7a2a15ca6845bd0ddbdf9b3c Mon Sep 17 00:00:00 2001 From: xEgoist Date: Sun, 16 Apr 2023 00:38:25 -0500 Subject: [PATCH 1/5] windows: use NtSetInformationFile in DeleteFile. Using `FILE_DELETE_ON_CLOSE` can silently succeed without reporting any error on non-empty directory. This commit adds usage of NtSetInformationFile which will report `DIRECTORY_NOT_EMPTY`. --- lib/std/os/windows.zig | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/std/os/windows.zig b/lib/std/os/windows.zig index f8672bc58d25..e195989e7ede 100644 --- a/lib/std/os/windows.zig +++ b/lib/std/os/windows.zig @@ -870,6 +870,7 @@ pub const DeleteFileError = error{ Unexpected, NotDir, IsDir, + DirNotEmpty, }; pub const DeleteFileOptions = struct { @@ -879,9 +880,9 @@ pub const DeleteFileOptions = struct { pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFileError!void { const create_options_flags: ULONG = if (options.remove_dir) - FILE_DELETE_ON_CLOSE | FILE_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT + FILE_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT else - FILE_DELETE_ON_CLOSE | FILE_NON_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT; // would we ever want to delete the target instead? + FILE_NON_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT; // would we ever want to delete the target instead? const path_len_bytes = @intCast(u16, sub_path_w.len * 2); var nt_name = UNICODE_STRING{ @@ -924,7 +925,7 @@ pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFil 0, ); switch (rc) { - .SUCCESS => return CloseHandle(tmp_handle), + .SUCCESS => {}, .OBJECT_NAME_INVALID => unreachable, .OBJECT_NAME_NOT_FOUND => return error.FileNotFound, .OBJECT_PATH_NOT_FOUND => return error.FileNotFound, @@ -935,6 +936,22 @@ pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFil .CANNOT_DELETE => return error.AccessDenied, else => return unexpectedStatus(rc), } + var file_dispo = FILE_DISPOSITION_INFORMATION{ + .DeleteFile = TRUE, + }; + rc = ntdll.NtSetInformationFile( + tmp_handle, + &io, + &file_dispo, + @sizeOf(FILE_DISPOSITION_INFORMATION), + .FileDispositionInformation, + ); + CloseHandle(tmp_handle); + switch (rc) { + .SUCCESS => return, + .DIRECTORY_NOT_EMPTY => return error.DirNotEmpty, + else => return unexpectedStatus(rc), + } } pub const MoveFileError = error{ FileNotFound, AccessDenied, Unexpected }; @@ -2470,6 +2487,10 @@ pub const FILE_INFORMATION_CLASS = enum(c_int) { FileMaximumInformation, }; +pub const FILE_DISPOSITION_INFORMATION = extern struct { + DeleteFile: BOOLEAN, +}; + pub const FILE_FS_DEVICE_INFORMATION = extern struct { DeviceType: DEVICE_TYPE, Characteristics: ULONG, From 73c04d4b2891b8e1c32261b59c0d17f4447f9658 Mon Sep 17 00:00:00 2001 From: xEgoist Date: Sun, 16 Apr 2023 15:13:12 -0500 Subject: [PATCH 2/5] fs: Re-enable non-empty dir test on windows --- lib/std/fs/test.zig | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index 1fbd7dbd6348..bcac246e97ff 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -488,10 +488,7 @@ test "deleteDir" { dir.close(); // deleting a non-empty directory - // TODO: Re-enable this check on Windows, see https://github.com/ziglang/zig/issues/5537 - if (builtin.os.tag != .windows) { - try testing.expectError(error.DirNotEmpty, tmp_dir.dir.deleteDir("test_dir")); - } + try testing.expectError(error.DirNotEmpty, tmp_dir.dir.deleteDir("test_dir")); dir = try tmp_dir.dir.openDir("test_dir", .{}); try dir.deleteFile("test_file"); From 89334fae205d02874fec1571e0637cfa1894ff1e Mon Sep 17 00:00:00 2001 From: xEgoist Date: Mon, 17 Apr 2023 22:35:04 -0500 Subject: [PATCH 3/5] windows: better error handling for DeleteFile. --- lib/std/os/windows.zig | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/std/os/windows.zig b/lib/std/os/windows.zig index e195989e7ede..8d2df4c7ef4d 100644 --- a/lib/std/os/windows.zig +++ b/lib/std/os/windows.zig @@ -933,7 +933,7 @@ pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFil .FILE_IS_A_DIRECTORY => return error.IsDir, .NOT_A_DIRECTORY => return error.NotDir, .SHARING_VIOLATION => return error.FileBusy, - .CANNOT_DELETE => return error.AccessDenied, + .ACCESS_DENIED => return error.AccessDenied, else => return unexpectedStatus(rc), } var file_dispo = FILE_DISPOSITION_INFORMATION{ @@ -950,6 +950,10 @@ pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFil switch (rc) { .SUCCESS => return, .DIRECTORY_NOT_EMPTY => return error.DirNotEmpty, + .INVALID_PARAMETER => unreachable, + .CANNOT_DELETE => return error.AccessDenied, + .MEDIA_WRITE_PROTECTED => return error.AccessDenied, + .ACCESS_DENIED => return error.AccessDenied, else => return unexpectedStatus(rc), } } From 55b2456c1187b22b2b8e2d3643aff86815dae551 Mon Sep 17 00:00:00 2001 From: xEgoist Date: Tue, 18 Apr 2023 04:13:38 -0500 Subject: [PATCH 4/5] fs: add test for Windows ready-only file deletion. Deleting a read-only file should result in `AccessDenied` (`CANNOT_DELETE`). Note: This test was observed to fail when the file is closed then reopened before the change in permission due to the absence of `FILE_WRITE_ATTRIBUTES` when re-opened. (see #15316). --- lib/std/fs/test.zig | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index bcac246e97ff..c953b529fa18 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -1415,3 +1415,22 @@ test "File.PermissionsUnix" { try testing.expect(permissions_unix.unixHas(.user, .execute)); try testing.expect(!permissions_unix.unixHas(.other, .execute)); } + +test "delete a read-only file on windows" { + if (builtin.os.tag != .windows) return error.SkipZigTest; + + var tmp = tmpDir(.{}); + defer tmp.cleanup(); + const file = try tmp.dir.createFile("test_file", .{ .read = true }); + // Create a file and make it read-only + const metadata = try file.metadata(); + var permissions = metadata.permissions(); + permissions.setReadOnly(true); + try file.setPermissions(permissions); + try testing.expectError(error.AccessDenied, tmp.dir.deleteFile("test_file")); + // Now make the file not read-only + permissions.setReadOnly(false); + try file.setPermissions(permissions); + file.close(); + try tmp.dir.deleteFile("test_file"); +} From 8c79559748b9828d2ad6a5bff89db4da0283d6b2 Mon Sep 17 00:00:00 2001 From: xEgoist Date: Tue, 18 Apr 2023 19:59:33 -0500 Subject: [PATCH 5/5] windows: Handle `DELETE_PENDING` in `DeleteFile`. DELETE_PENDING can happen when the file is yet to be closed for deletion or if it never get closed. In that case, DeleteFile should assume the file deletion is succeeding (no CloseHandle is required as it's a "failure"). In case of `DELETE_PENDING` failure, the file may still exist. In which case if it's part of `deleteTree`, it will eventually fail on `error.DirNotEmpty`. --- lib/std/os/windows.zig | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/std/os/windows.zig b/lib/std/os/windows.zig index 8d2df4c7ef4d..7a2d75f3457f 100644 --- a/lib/std/os/windows.zig +++ b/lib/std/os/windows.zig @@ -934,6 +934,7 @@ pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFil .NOT_A_DIRECTORY => return error.NotDir, .SHARING_VIOLATION => return error.FileBusy, .ACCESS_DENIED => return error.AccessDenied, + .DELETE_PENDING => return, else => return unexpectedStatus(rc), } var file_dispo = FILE_DISPOSITION_INFORMATION{