From 1ae378e7a253ec845d8ee8d52b5505c9aa177369 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Sat, 22 Jul 2023 23:27:11 -0700 Subject: [PATCH 1/2] windows.DeleteFile: Use FileDispositionInformationEx if possible, but fallback if not Using FileDispositionInformationEx (and therefore flags like FILE_DISPOSITION_POSIX_SEMANTICS and FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE) is only supported on NTFS, so the comptime Windows version range check is not enough to determine whether or not the NtSetInformationFile will succeed. This commit makes DeleteFile always try using FileDispositionInformationEx first, but if INVALID_PARAMETER is received (which is the status that occurs when the filesystem doesn't support FileDispositionInformationEx), then it will fallback and try calling NtSetInformationFile with FileDispositionInformation. This keeps NTFS as fast as it was before, since it will do at most 1 NtSetInformationFile call, but on non-NTFS filesystems (e.g. FAT32), DeleteFile may need to do 2 NtSetInformationFile calls. Closes #16497 --- lib/std/os/windows.zig | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/std/os/windows.zig b/lib/std/os/windows.zig index 9f8aa326a9..02adf1a92f 100644 --- a/lib/std/os/windows.zig +++ b/lib/std/os/windows.zig @@ -940,8 +940,13 @@ pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFil } defer CloseHandle(tmp_handle); + // FileDispositionInformationEx (and therefore FILE_DISPOSITION_POSIX_SEMANTICS and FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE) + // are only supported on NTFS filesystems, so the version check on its own is only a partial solution. To support non-NTFS filesystems + // like FAT32, we need to fallback to FileDispositionInformation if the usage of FileDispositionInformationEx gives + // us INVALID_PARAMETER. + var need_fallback = true; if (comptime builtin.target.os.version_range.windows.min.isAtLeast(.win10_rs1)) { - // Deletion with posix semantics. + // Deletion with posix semantics if the filesystem supports it. var info = FILE_DISPOSITION_INFORMATION_EX{ .Flags = FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS | @@ -955,7 +960,14 @@ pub fn DeleteFile(sub_path_w: []const u16, options: DeleteFileOptions) DeleteFil @sizeOf(FILE_DISPOSITION_INFORMATION_EX), .FileDispositionInformationEx, ); - } else { + switch (rc) { + // INVALID_PARAMETER here means that the filesystem does not support FileDispositionInformationEx + .INVALID_PARAMETER => {}, + // For all other statuses, fall down to the switch below to handle them. + else => need_fallback = false, + } + } + if (need_fallback) { // Deletion with file pending semantics, which requires waiting or moving // files to get them removed (from here). var file_dispo = FILE_DISPOSITION_INFORMATION{ From 21ecb1ba0fabe471476df9ca328c8aa14767af24 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Sun, 23 Jul 2023 00:21:07 -0700 Subject: [PATCH 2/2] Consolidate 'delete a read-only file on windows' test cases These two tests can't be disambiguated at comptime, since the filesystem that the test is running on also matters for whether or not POSIX_SEMANTICS / IGNORE_READONLY_ATTRIBUTE can actually be used (since they are only supported on NTFS). --- lib/std/fs/test.zig | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index 4c51e512b6..ac4163fe91 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -1416,34 +1416,13 @@ test "File.PermissionsUnix" { try testing.expect(!permissions_unix.unixHas(.other, .execute)); } -test "delete a read-only file on windows with file pending semantics" { - if (builtin.os.tag != .windows or builtin.target.os.version_range.windows.min.isAtLeast(.win10_rs1)) +test "delete a read-only file on windows" { + if (builtin.os.tag != .windows) return error.SkipZigTest; - var tmp = tmpDir(.{}); + var tmp = testing.tmpDir(.{}); defer tmp.cleanup(); - { - const file = try tmp.dir.createFile("test_file", .{ .read = true }); - defer file.close(); - // 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); - } - try tmp.dir.deleteFile("test_file"); -} -test "delete a read-only file on windows with posix semantis" { - if (builtin.os.tag != .windows or !builtin.target.os.version_range.windows.min.isAtLeast(.win10_rs1)) - return error.SkipZigTest; - - var tmp = tmpDir(.{}); - defer tmp.cleanup(); const file = try tmp.dir.createFile("test_file", .{ .read = true }); defer file.close(); // Create a file and make it read-only @@ -1451,7 +1430,21 @@ test "delete a read-only file on windows with posix semantis" { var permissions = metadata.permissions(); permissions.setReadOnly(true); try file.setPermissions(permissions); - try tmp.dir.deleteFile("test_file"); // file is unmapped and deleted once last handle closed + + // If the OS and filesystem support it, POSIX_SEMANTICS and IGNORE_READONLY_ATTRIBUTE + // is used meaning that the deletion of a read-only file will succeed. + // Otherwise, this delete will fail and the read-only flag must be unset before it's + // able to be deleted. + const delete_result = tmp.dir.deleteFile("test_file"); + if (delete_result) { + try testing.expectError(error.FileNotFound, tmp.dir.deleteFile("test_file")); + } else |err| { + try testing.expectEqual(@as(anyerror, error.AccessDenied), err); + // Now make the file not read-only + permissions.setReadOnly(false); + try file.setPermissions(permissions); + try tmp.dir.deleteFile("test_file"); + } } test "delete a setAsCwd directory on Windows" {