Possible race conditions in mirror.c sample. #32
Comments
|
Windows Kernel controls dokan driver. Dokan driver only forward the kernel call to user land. |
|
@Liryna It seems that I misunderstood the purpose of the #include <stdio.h>
#include <windows.h>
char *file1="n:\\test";
char *file2="n:\\test2";
DWORD WINAPI ThreadProc(void*)
{
while(1)
{
MoveFile(file1, file2);
}
return 0;
}
int main(int nargs,char *args[])
{
DWORD tid;
HANDLE thr=CreateThread(NULL, 0, ThreadProc, 0, 0, &tid);
while(1)
{
HANDLE hFile=CreateFile(file1, GENERIC_WRITE|GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE, 0, CREATE_NEW, FILE_FLAG_SEQUENTIAL_SCAN, 0);
if(hFile==INVALID_HANDLE_VALUE)
continue;
DWORD tmp;
WriteFile(hFile, "1", 1, &tmp, 0);
HANDLE hMap=CreateFileMapping(hFile, 0, PAGE_READONLY, 0, 0, 0);
char *buf=(char*)MapViewOfFile(hMap, FILE_MAP_READ, 0, 0, 0);
CloseHandle(hMap);
CloseHandle(hFile);
printf("%c\n", buf[0]);
UnmapViewOfFile(buf);
DeleteFile(file1);
DeleteFile(file2);
}
return 0;
}before compiling |
|
Is it your software that is crashing or mirrorfs ? |
|
The crash is in my test program, it is crashing on the attempt to read from In short, when |
|
Thank for the logs ! But what do you expect to happen ? Your file is open with FILE_SHARE_DELETE. I do not see your point sorry :( |
|
But without dokan windows working just fine after rename. My point is that file should be really closed only after the program is done with it, including access through memory map (in MirrorCloseFile instead of MirrorCleanup maybe?) closing it and then reopening is not reliable. |
|
OK I see, |
|
I have been starting |
|
I guess this could come from IRP race in kernel driver. |
|
I am not very familiar with the kernel programming, but I have made a simpler test case demonstrating this problem, without multiple threads accessing the same file: #include <stdio.h>
#include <windows.h>
char *file1="n:\\test";
int main(int nargs,char *args[])
{
HANDLE hFile=CreateFile(file1, GENERIC_WRITE|GENERIC_READ, FILE_SHARE_READ, 0, CREATE_ALWAYS, FILE_FLAG_SEQUENTIAL_SCAN, 0);
DWORD tmp;
WriteFile(hFile, "1", 1, &tmp, 0);
HANDLE hMap=CreateFileMapping(hFile, 0, PAGE_READWRITE, 0, 0, 0);
char *buf=(char*)MapViewOfFile(hMap, FILE_MAP_WRITE, 0, 0, 0);
CloseHandle(hFile);
CloseHandle(hMap);
HANDLE hFile2=CreateFile(file1, GENERIC_WRITE|GENERIC_READ, FILE_SHARE_READ, 0, CREATE_ALWAYS, FILE_FLAG_SEQUENTIAL_SCAN, 0);
printf("%x %x\n", hFile2, GetLastError());
UnmapViewOfFile(buf);
}On the regular fs second So it seems to be not a good idea to close file handle in the |
|
I agree, it is a race condition on mirror sample (not on driver neither on the library). |
|
@Maxhy doesn't s seems to help, MirrorCloseFile is printing |
|
So far in my application I am closing the file handle (one internal to my application, not a real file system one) on CloseFile instead of Cleanup. This seems to work fine (although I am not using the file system very aggressively), but this is against what the documentation says, and what some messages on the mailing list said. According to the docs: "If the user application calls CloseHandle and subsequently opens the same file, the CloseFile function of the file system application may not be invoked before the CreateFile API is called. This may cause a sharing violation error." |
|
@suy I am doing the same as you. More informations about cleanup/close difference: From what I have understand: Last part of IRP_MJ_CLOSE: |
|
Is there any update on this? Right now there seems to be no way to properly handle memory mapped files in dokan, MirrorCleanup is called too soon to close the handle in it, but MirrorCloseFile is called too late... |
|
My application seems to work fine with what I said: close the handle in CloseFile instead. I don't know if in some conditions I can get a sharing violation error with it, though (that's what the original documentation said), but I don't know how to provoke that. |
|
I have tried moving all the code in mirror sample from MirrorCleanup to MirrorCloseFile and then executing this test program: #include <stdio.h>
#include <windows.h>
char *file1="n:\\test";
int main(int nargs,char *args[])
{
HANDLE hFile=CreateFile(file1, GENERIC_WRITE|GENERIC_READ, FILE_SHARE_READ, 0, CREATE_ALWAYS, FILE_FLAG_SEQUENTIAL_SCAN, 0);
printf("%x %x\n", hFile, GetLastError());
if(hFile==INVALID_HANDLE_VALUE)
return 0;
DWORD tmp;
WriteFile(hFile, "1", 1, &tmp, 0);
HANDLE hMap=CreateFileMapping(hFile, 0, PAGE_READWRITE, 0, 0, 0);
char *buf=(char*)MapViewOfFile(hMap, FILE_MAP_WRITE, 0, 0, 0);
CloseHandle(hFile);
CloseHandle(hMap);
printf("%c\n", buf[0]);
UnmapViewOfFile(buf);
}On the first run all seems to be okay, but on the second |
|
Sorry for the delay @storzinc . Here is the debug output after two run: MirroFS The Dokan seems to be never called to Close the first handle by Windows Kernel for an unknown reason. |
|
@storzinc I solved the issue by moving UnmapViewOfFile before CloseHandle. #include <stdio.h>
#include <windows.h>
char *file1 = "m:\\test.txt";
int main(int nargs, char *args[])
{
HANDLE hFile = CreateFile(L"m:\\test2.txt", GENERIC_WRITE | GENERIC_READ, FILE_SHARE_READ, 0, CREATE_ALWAYS, FILE_FLAG_SEQUENTIAL_SCAN, 0);
printf("%x %x\n", hFile, GetLastError());
if (hFile == INVALID_HANDLE_VALUE)
return 0;
DWORD tmp;
WriteFile(hFile, "1", 1, &tmp, 0);
HANDLE hMap = CreateFileMapping(hFile, 0, PAGE_READWRITE, 0, 0, 0);
char *buf = (char*)MapViewOfFile(hMap, FILE_MAP_WRITE, 0, 0, 0);
printf("%c\n", buf[0]);
UnmapViewOfFile(buf);
CloseHandle(hFile);
CloseHandle(hMap);
}I successfully get a call from close after the cleanup at each launch of this test code. Observation about your example: It is right that close in not called after cleanup but this only happening for the first test, every other run successfully get a call from close after the cleanup. So even if the Microsoft documentation say that it can be called in any order, it seems that there is a different behaviour in the windows kernel when UnmapViewOfFile is called after CloseHandle. Does this seems alright for you ? |
|
In windows it is perfectly legal to close all of the file handles and work only with the memory map, many programs are relying on this, I have moved UnmapViewOfFile to the end just to demonstrate the problem with dokan. And either way, even if the program did something wrong, there should be no access violations after the process is terminated. |
|
This part of the observation is not related to dokan but every file system driver. If you have any informations/fix to provide, let me know and I will look more about it. For me this issue is related to Windows and it should forward to them. |
|
I have tried to figure out how memory mapping is working on the regular fs, without dokan, and I got interesting result with Process Monitor (https://technet.microsoft.com/en-us/library/bb896645.aspx) With this test program: #include <stdio.h>
#include <windows.h>
int main(int nargs,char *args[])
{
HANDLE hFile=CreateFile("test.txt", GENERIC_WRITE|GENERIC_READ, FILE_SHARE_READ, 0, CREATE_ALWAYS, FILE_FLAG_SEQUENTIAL_SCAN, 0);
DWORD tmp;
WriteFile(hFile, "1", 1, &tmp, 0);
HANDLE hMap=CreateFileMapping(hFile, 0, PAGE_READWRITE, 0, 0, 0);
char *buf=(char*)MapViewOfFile(hMap, FILE_MAP_WRITE, 0, 0, 0);
CloseHandle(hFile);
CloseHandle(hMap);
printf("%c\n", buf[0]);
HANDLE hFile2=CreateFile("test.txt", GENERIC_WRITE|GENERIC_READ, FILE_SHARE_READ, 0, CREATE_ALWAYS, FILE_FLAG_SEQUENTIAL_SCAN, 0);
printf("%x %x\n", hFile2, GetLastError());
UnmapViewOfFile(buf);
hFile2=CreateFile("test.txt", GENERIC_WRITE|GENERIC_READ, FILE_SHARE_READ, 0, CREATE_ALWAYS, FILE_FLAG_SEQUENTIAL_SCAN, 0);
printf("%x %x\n", hFile2, GetLastError());
}I get this log with Process Monitor:
Here IRP_MJ_CLEANUP was sent before the second IRP_MJ_CREATE, and IRP_MJ_CLOSE was sent after the third, but still, system somehow knew that the second IRP_MJ_CREATE should be rejected with Maybe there is another API to work with memory mapping that the system is using? |
|
Dokany driver actually never raises the SHARING VIOLATION because the driver does actually not implement the ReferenceCount and OpendHandleCount Lines 258 to 259 in 837633a What is the expected timeframe for the implementation of the missing functionality from your point of view? |
|
Ah you are right 😄 ! I just do not get the difference of OpenCount (ReferenceCount ?) & OpenFileCount(OpendHandleCount ?) in their example for now. |
|
ReferenceCount is the actual FileCount in the dokan code. You can reference a file for various operations (not only open) and this prevents the fcb from being freed. OpenFileCount is meant for how many times is the file opened. If you try to open the file > 0 times, then the check should be performed. In the example above provided by microsoft fastfat the count is used for the volume and not for the file it self. But the code would look the similar way. |
|
The problem I actually see is that CreateFileMapping does not send an IRP request to the filesystem. So the file system has no idea, that the file is opened a second time. On NTFS filesystem this works properly whyever 😟 In the actual version of dokan I think when you do a CreateFile of the same file two times (without to close the handle), even then you will not get SHARED_ACCESS_VIOLATION. This is something which can be fixed. @Liryna |
|
@marinkobabic I think CreateFileMapping is using the file context of CreateFile since the handle is given as argument. The issue come from CreateFileMapping is not cleaning well the context of the file in some case, so it stay open for the file system. You are right about the second statement. |
|
@Liryna |
|
@marinkobabic I do not see new commit in your fork 😢 |
|
@Liryna Should now be available |
|
@marinkobabic Do you have a direct link ? |
|
@Liryna |
|
@marinkobabic Oh sorry Output of the ConsoleApplication3 (it is the last example of @storzinc) DokanFS Logs http://pastebin.com/np96j8rb |
|
The first handle is closed as you can see above CloseHandle(hFile); |
|
Yes it is right but if I run it on NTFS: The STATUS_SHARING_VIOLATION happen in the second createfile. |
|
You are right, this is because I have no idea how to manage the CreateFileMapping. On NTFS event if you close the hand to the file and hMap and you did not UnmapViewOfFile the handle somehow remains. This is the reason why you get on NTFS the exception on the second file and dokan returns the exception on the third file. Dokan gets only notified when you do CreateFile and CloseHandle. |
|
I was thinking ReferenceCount should do the job. |
|
What happens if you use FAT as the underlying file system? On Wed, Aug 19, 2015 at 5:44 PM, Marinko notifications@github.com wrote:
Kapp Arnaud - Xaqq |
|
@Liryna |
|
Seems to be an IRP for minifilter. But there must be one for file system driver. |
|
From what it worth, fat32 have the same behaviour. |
|
@marinkobabic Since your changes "implement shareaccess" does not affect this issue, have you seen some case where it work ? |
|
@Liryna SectionObjectPointers have the information about Memory Manager and if the file is memory mapped. But we need to implement it in a clean way. I would suggest to keep the change in the development branch, except somebody requests the change. Just put it on your todo list. |
|
Thank you for the clear description ! Added to the TODO |
|
I have created a ticket with the main issue we face here(#152) |
I am trying to make my own fs using mirror.c sample as a basis, but it seems to me that there will be race conditions if multiple threads will be accessing it concurrently.
For example in the
MirrorCleanupthere is such code:But wouldn't it be possible that after
CloseHandleanother thread will delete this file, and create new file in it's place?Similar problem seems to be in
MirrorDeleteDirectoryfunction, that function is checking that the directory being deleted is empty, but what if new file will be created after this check but before directory really will be deleted?Also in multiple functions there is code similar to this:
But in what cases can you get invalid handle sent to your function? And is it really okay to just reopen file here?
Maybe I am missing something?
The text was updated successfully, but these errors were encountered: