Notepad.exe (memory mapped files) causes ReadFile after Cleanup on same context

[Dwedit]

Environment

• Windows version: 10 (Version 10.0.19042.1165)
• Processor architecture: x64
• Dokany version: 1.5.0.3173
• Library type (Dokany/FUSE): Dokany

Check List

• I checked my issue doesn't exist yet
• My issue is valid with mirror default sample and not specific to my user-mode driver implementation
• I can always reproduce the issue with the provided description below.
• I have updated Dokany to the latest version and have reboot my computer after.
• I tested one of the last snapshot from appveyor CI

Description

Notepad.exe is making this sequence of Win32 API calls:

CreateFileW ( "C:\MOUNTPOINT\FILENAME.txt", GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL )	0x00000000000003dc
CreateFileMappingW ( 0x00000000000003dc, NULL, PAGE_READONLY, 0, 12, NULL )	0x0000000000000424
MapViewOfFile ( 0x0000000000000424, FILE_MAP_READ, 0, 0, 12 )	0x000002590f580000
CloseHandle ( 0x0000000000000424 )	TRUE
CloseHandle ( 0x00000000000003dc )	TRUE   <- file handle closed, triggers Cleanup
UnmapViewOfFile ( 0x000002590f580000 )	TRUE

This sequence causes the Dokan Filesystem to see ReadFile get called on the same context after Cleanup has been called.

Cleanup was called as soon as the application called CloseHandle on the file handle, even though the file handle had been passed into a FileMapping object, then that FileMapping was passed into a View Of File.

Cleanup should not have happened until the File Mapping and View Of File were both closed.

Note that it may possibly be necessary to disable antivirus to reproduce this

Logs

No logs included yet, I don't think they're necessary for this issue.

[Liryna]

Hi @Dwedit ,

This is expected. See last line of Dokan operations section.
https://dokan-dev.github.io/dokany-doc/html/

[Dwedit]

I completely missed that part of the documentation, because I was only reading the comments for the DOKAN_OPERATIONS struct. I didn't realize there was other documentation as well.

Here is the line from the documentation in question:

Note: when applications make use of memory mapped files, WriteFile or ReadFile functions may be invoked after Cleanup in order to complete the I/O operations. The file system application should also properly work in this case.

The comments for Cleanup, ReadFile, and WriteFile do not warn that ReadFile or WriteFile may be called after Cleanup for memory mapped files.

Another note from the documentation describes the lifetime of the Context:

File system applications can freely use this variable to store values that are constant in a file access session (the period from CreateFile to CloseFile),

But a comment in CreateFile directly contradicts this:

To avoid memory leak, Context needs to be released in DOKAN_OPERATIONS.Cleanup

This suggests that the lifetime of the Context ends in Cleanup, and not CloseFile.

I think it would also help if the documentation made it clear that Cleanup is a blocking operation, and is intended for finishing up writes, deleting files, and closing file handles, while CloseFile is non-blocking, and is intended for cleaning up internal objects and memory.

[Liryna]

I agree the documentation could be improved regarding this.
Do you mind proposing a pull request that includes your observations? This would be very appreciated!

Just, what do you mean by Cleanup is a blocking operation?

[Dwedit]

I meant that the Application calls CloseHandle, then the application waits for Cleanup in the Dokan filesystem to finish before CloseHandle can return. Cleanup is said to be "blocking" CloseHandle from finishing, so it is a blocking operation.

Meanwhile, the application does not wait CloseFile to finish, so it is non-blocking.

[Liryna]

@Dwedit Oh yes that way indeed. Thanks for sharing your thoughts.

[the-Arioch]

This suggests that the lifetime of the Context ends in Cleanup, and not CloseFile.

Which makes me wonder...
What would Windows (and then Dokan) do if...

CloseHandle ( 0x00000000000003dc )	TRUE   <- file handle closed, triggers Cleanup
  >>>  insert memory write into the mapped area here, which should trigger file write operation  <<<
UnmapViewOfFile ( 0x000002590f580000 )	TRUE

[Dwedit]

Windows is properly doing its accounting. Dokan's CloseFile is called after all three pieces of the file mapping have been released.

It's just that here, Dokan's Cleanup is happening "early", at the first call to close the file handle, rather than when all three pieces have been closed.

Currently, a call to Dokan ReadFile or WriteFile must re-open any file handles it needs before it can do any reads or writes. (The Mirror filesystem does this.)

However, this can cause a race condition. The application will release the file mapping, and there will not be any blocking calls made to Dokan Cleanup at that time. Instead, there is a non-blocking call made to Dokan CloseFile. If the application wants to re-open the file immediately after the file mapping is released, it will likely happen before Dokan CloseFile has finished. This can cause a sharing violation when it tries to reopen a file which hasn't finished being closed yet.

What should ideally happen:

• No "early" calls to Dokan Cleanup
• The final call to release a file mapping (CloseHandle on the File Mapping, UnmapViewOfFile on the ViewOfFile, and CloseHandle on the file handle) should trigger the call to Dokan Cleanup, and it should block until Cleanup has finished.
  This would eliminate the need for the Dokan filesystem to reopen any files, which would eliminate the race condition.

If there is a technical reason why a blocking Cleanup can't be called when all three pieces of the file mapping have been released, there is a possible ugly workaround which could be implemented within the usermode library:

• This would catch cases where ReadFile or WriteFile have been called after Cleanup. ("Reanimated" files)
• If it would call any Dokan operation that isn't CloseFile on that file handle (especially ReadFile or WriteFile):
  • These operations should be synchronous (use a critical section here)
  • Trigger CloseFile to finish destroying the context
  • Call ZwCreateFile to create a new context (will always need to open an existing file, too bad if the original file was 'delete on close')
  • Associate the new context with the old file handle
• When CloseFile happens on a reanimated handle:
  • We're trying to simulate a synchronous call to Cleanup if there was a technical reason that it couldn't be done at the time of destroying the File Mapping, so instead:
  • Want to block every Dokan operation that uses that Filename until CloseFile has finished.
  • Possible implementation:
    • List of "Closing-in-progress" filenames, Critical Section for each filename (to wait for CloseFile to finish), Critical Section for the entire list (for adding and removing names)
    • Add the Filename to the list, wait for Cleanup and CloseFile to finish, remove the filename from the list.
    • Every Dokan operation will need to check the "closing-in-progress" filename list to see if it needs to block.

While this could eliminate the need to reanimate files, this would add a filename list check (list is normally empty, checking list size can be lock-free), and possible critical section lock to every single Dokan operation.

[the-Arioch]

This would catch cases where ReadFile or WriteFile have been called after Cleanup. ("Reanimated" files)

How exactly a dokanlet would do it?

Basically this says that every dokanlet has to:

1. implement canonicalization of every filename to make a stable unique ID.
2. implement a global and fast and multi-threaded list of open files.

It is not a simplest task. When you say "Windows is properly doing its accounting." you mean Microsoft already managed to "do it right" and iron out unobvious bugs. Now every dokanlet is expected to do it too (while the whole premise of Dokany is to do difficult things once and let "regular" programmers "have it easy" doing dokanlets?

Okay, let's assume dokanlets would do.

Then it would make dokanlets totalyl independent of Dokan core on the "accounting" part.
If every dokanlet would make those canonicalization and accounting on their own behalf - it would be relatively easy to add ARC (auto-ref-counting) on top of it, ignore Dokan's call to Cleanup and do dokanlet's own clean-up on refcounter going zero.

So, basically that is the road for dokanlets to ignore Dokany's service and reimplementing them on their own.
It might be a working "ugly hack" when there is no other choice and the "fix is required yesterday", but other than that it does not seem a good road to take.

As an option, some eager dokanlet might implement it, but with the goal to iron it out and then "uplift" the implementation into Dokany core, replacing current implementation. Basically making such a dokanlet an "alpha of Dokany" :-)