Merge pull request #16499 from squeek502/posix-semantics-2

windows.DeleteFile: Use FileDispositionInformationEx if possible, but fallback if not
ziglang · Jul 23, 2023 · b35874a · b35874a
2 parents 1bf16b1 + 21ecb1b
commit b35874a
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 27 deletions.
diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig
@@ -1416,42 +1416,35 @@ 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
     const metadata = try file.metadata();
     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" {

diff --git 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{