windows: use NtSetInformationFile in DeleteFile.

[ghost]

closes#15315
closes#5537

Using FILE_DELETE_ON_CLOSE can silently succeed without reporting any error on non-empty directory. One way to return an error is to add NtSetInformationFile which will cause DeleteFile to report DIRECTORY_NOT_EMPTY.

[squeek502]

Should also enable this test case:

zig/lib/std/fs/test.zig

Lines 490 to 494 in 397649f

	// 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) {
	trytesting.expectError(error.DirNotEmpty, tmp_dir.dir.deleteDir("test_dir"));
	}

[ghost]

Should also enable this test case:

zig/lib/std/fs/test.zig

Lines 490 to 494 in 397649f

	// 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) {
	trytesting.expectError(error.DirNotEmpty, tmp_dir.dir.deleteDir("test_dir"));
	}

Will do, thanks for pointing that.

[squeek502]

Because this NtCreateFile call does not use FILE_DELETE_ON_CLOSE anymore, it seems likely that this error is no longer possible here but likely is possible from the NtSetInformationFile call.

More context:

• Add CANNOT_DELETE as a possible error in os.windows.DeleteFile #10495
• Windows: Files marked as read-only fail to get deleted #13084

It'd probably be good to add a test case that intentionally tries to delete a read-only file so that we know this case continues to be handled in the same way. Maybe something like (totally untested):

test"deleting a read-only file on windows" {
if (builtin.os.tag!=.windows) returnerror.SkipZigTest;
vartmp=tmpDir(.{});
defertmp.cleanup();
// Create a file and make it read-only
{
constfile=trytmp.dir.createFile("test_file", .{ .read=true });
deferfile.close();
constmetadata=tryfile.metadata();
varpermissions=metadata.permissions();
permissions.setReadOnly(true);
tryfile.setPermissions(permissions);
}
trytesting.expectError(error.AccessDenied, tmp.dir.deleteFile("test_file"));
// Now make the file not read-only
{
constfile=trytmp.dir.openFile("test_file", .{});
deferfile.close();
constmetadata=tryfile.metadata();
varpermissions=metadata.permissions();
permissions.setReadOnly(false);
tryfile.setPermissions(permissions);
}
trytmp.dir.deleteFile("test_file");
}

[ghost]

You are right. It should be moved to the set function return.
Also, while testing this, I found out that NtCreateFile can also result in AccessDenied, when the owner is different. So we can replace CANNOT_DELETE with ACCESS_DENIED and add CANNOT_DELETE to the second rc.

[ghost]

Note on the test example:
I think you may have found another bug with that (if it doesn't already exist) 😄 . The test will fail on Access Denied for a different reason. The file is not open with FILE_WRITE_ATTRIBUTES as the only OpenMode are read_only (GENERIC_READ), write_only (GENERIC_WRITE), and read_write (both).

and since GENERIC_READ only have FILE_READ_ATTRIBUTES, it cannot change its attributes anymore after the read only is set. Sort of related to the second issue you linked, but specific to setPermissions rather than deleteFile

I am looking to see if there are more refined flags or functions that allows that, but from a quick search, i couldn't find one.

[squeek502]

Looks like this is the relevant issue: #12377 (about Dir but seems like it might apply to File.OpenFlags too).

[ghost]

Yep, that looks like the same behavior.
I am adding a modified test with no file closing/reopen. If it doesn't seem necessary to test right now due to this, I can remove it.

[squeek502]

Can also add "closes #5537" to OP

[andrewrk]

Thank you for the implementation @xEgoist and thank you for the review @squeek502. I'm happy to merge this when @squeek502 gives the thumbs up.

[squeek502]

Looks good to me 👍