Deleting a file twice with a file handle open, on Windows 7

[Yutaka Hirano]

Hi,

I encountered a platform-dependent problem with base::DeleteFile.

Here is a change containing two test cases.

TEST_F(FileUtilTest, DeleteTwice) {
  const FilePath path =
      temp_dir_.GetPath().Append(FILE_PATH_LITERAL("foo.txt"));
  {
    const uint32_t flags = File::FLAG_CREATE | File::FLAG_READ |
                           File::FLAG_WRITE | File::FLAG_CAN_DELETE_ON_CLOSE |
                           File::FLAG_WIN_SHARE_DELETE;
    File file(path, flags);
  }

  EXPECT_TRUE(base::PathExists(path));
  EXPECT_TRUE(base::DeleteFile(path));
  EXPECT_FALSE(base::PathExists(path));
  EXPECT_TRUE(base::DeleteFile(path));
}

TEST_F(FileUtilTest, DeleteTwiceWithHandle) {
  const FilePath path =
      temp_dir_.GetPath().Append(FILE_PATH_LITERAL("foo.txt"));
  const uint32_t flags = File::FLAG_CREATE | File::FLAG_READ |
                         File::FLAG_WRITE | File::FLAG_CAN_DELETE_ON_CLOSE |
                         File::FLAG_WIN_SHARE_DELETE;
  File file(path, flags);

  EXPECT_TRUE(base::PathExists(path));
  EXPECT_TRUE(base::DeleteFile(path));
  EXPECT_FALSE(base::PathExists(path));
  EXPECT_TRUE(base::DeleteFile(path));
}

I expect both test cases to pass on every platform. In reality, DeleteTwiceWithHandle fails only on Windows 7 (win7-rel results, win10_chromium_x64_rel_ng results).

 1. Is this an expected behavior?

 2. Assuming it's not expected, how can we fix this?

Thanks,

[Wez]

Historically it was the case that you could not delete a file under Windows if one or more processes had that file "open", unlike POSIX platforms in which it is fine to "unlink" a file from a directory even while it is still in-use.

IIRC there were some differences in how this worked for different filesystems, but I don't know offhand why this should differ between Win7 and Win10 - I imagine we're running both on NTFS rather than FAT, for example. :P

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABihn6EnnYsPUTMU_K1PRwsqKMC0tH_fC8q5-8JKSiJ1hu3HAQ%40mail.gmail.com.

[Greg Thompson]

Hi. Welcome to Windows.

There are (at least) two things going here that are confusing you (and likely most other people who would look at this):

1. base::PathExists(path) doesn't do what you might think it does. I tried to fix this once; see https://crrev.com/c/2306029. It has a nice comment in base/files/file_util_win.cc explaining what's happening. Briefly, it returns `false` when the attributes of the named path can't be retrieved. It just so happens that there are many reasons why you may not be able to fetch attributes, only some of which are that the path doesn't exist.

2. On Windows, ::DeleteFile only marks a file for deletion -- it doesn't necessarily delete it. The file will be deleted once all handles are closed and the filesystem drivers have finished with it. Attempts to access the file by name will fail with ERROR_ACCESS_DENIED (as you're seeing). It's entirely possible that modern versions of Windows use POSIX semantics[1] by default, which could explain why the test passes on a more recent version of the OS.

As for "how can we fix this", what is the "this" you'd like to fix?

If you're curious, read https://crsrc.org/c/chrome/installer/mini_installer/delete_with_retry.cc. It is an implementation of "delete the file/dir at `path`" that blocks until it succeeds. It has copious documentation explaining what it does and why.

[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntddk/ns-ntddk-_file_disposition_information_ex

[Yutaka Hirano]

Thank you!

> As for "how can we fix this", what is the "this" you'd like to fix?

I expect file deletion to be an idempotent operation, so if the first base::DeleteFile call succeeded (and there are no competing/conflicting operations), I expect the second call to succeed.

Is it possible to distinguish the "marked for deletion" state somehow?

[Greg Thompson]

How would you know that you're not racing against another process (e.g., you delete, other process creates and locks, now you can't delete again)?

There might be a way to infer that the entry is marked for deletion, but I'm not sure what that buys you.

If we take a step back, what is the problem you're trying to solve that brought you to discovering these quirks of Windows?

In other news, I've thought of implementing an asynchronous API for "delete this thing and run my callback when it's so completely gone that I can reuse its name." But I haven't.

Back in 2015 or 2016 I wrote an extremely complicated thing that tried to do this synchronously (by renaming the thing before deleting it), but there were so many edge cases that I feared making an already odd thing even more odd. I probably have an abandoned CR for that somewhere...

[Yutaka Hirano]

Thank you very much, and sorry for the belated response.

This came from https://crrev.com/c/3583411 - I replaced disk_cache::DeleteCacheFile calls

with base::DeleteFile calls. These have subtle behavior differences on older Windows (as described above),

and that caused failures on DiskCacheBackendTest.FileSharing.

I checked the existing call sites, and I think the behavior different actually doesn't matter for us, so I'm going

to accept the behavior and fix the test: https://chromium-review.googlesource.com/c/chromium/src/+/3620035

Again, thank you very much for your help!

[Greg Thompson]

if i'm reading the old comment within DeleteCacheFile correctly, it sounds like you are certain that the cache has an open handle to the file at the moment you want to delete it. if this is the case, then perhaps there's an even better way to delete the file.

since windows deletes files when the last handle is closed, you can effectively delete and undelete a file as much as you want while you have an open handle to it. once you mark a file for deletion via an open handle, no other process can open it. when the last handle to a file marked for deletion is closed, the file is deleted. knowing this, you can use base::File::FLAG_CAN_DELETE_ON_CLOSE when you open a file and then use base::File::DeleteOnClose(true) to cause the file to be deleted when you close your handle. base/files/file.h has some commentary about this on the base::File::DeleteOnClose() method.

another thing to consider is what behavior you want in case of abnormal process termination while a file is open in the cache. should the file stay there or be deleted? if the latter, you can use base::File::FLAG_CAN_DELETE_ON_CLOSE when you open the file, call DeleteOnClose(true) immediately thereafter, and then do nothing other than close the handle if you wish to delete the file after use. when you reach the point in processing where you want the file to persist, call DeleteOnClose(false). now the file will stay there after being closed but will not if the process terminates before reaching that point.

let me know if this isn't clear. it's a bit non-intuitive.