cygwin bash cannot delete directories on mirror drive

[bailey27]

Environment

• Windows version: Windows 10
• Processor architecture: x64
• Dokany version: 1.0.0-RC3
• Library type (Dokany/FUSE): Dokany

Check List

• [x ] I checked my issue doesn't exist yet
• [x ] My issue is valid with mirror default sample and not specific to my user-mode driver implementation
• [x ] I can always reproduce the issue with the provided description below.
• [x ] I have updated Dokany to the latest version (https://github.com/dokan-dev/dokany/releases) and have reboot my computer after.

Description

If you use cygwin bash on a mirror drive and try to mkdir a directory and then try to rmdir it, you get "Permission denied".

M:\>mkdir foo

M:\>rmdir foo

M:\>bash

Bailey@haswell /cygdrive/m
$ mkdir foo

Bailey@haswell /cygdrive/m
$ rmdir foo
rmdir: failed to remove 'foo': Permission denied

The problem seems to be that cygwin is using the native NT APIs.

The way it deletes a directory is by opening it with NtOpenFile() with access DELETE, then calling NtSetInformationFile() to set the disposition to delete on close (DELETE_ON_CLOSE).

mirror.c has three problems with this.

1. It thinks a directory should not be opened with DELETE access.
2. If MirrorDeleteFile() is called on a directory, then it returns an error. It should check the fiile
   attributes, and if it's a directory, then call MirrorDeleteDirectory().
3. It uses only DokanFileInfo->IsDirectory to tell if the thing that should be deleted on close is a directory or a file.

It looks like MirroDeleteFile() can't call MirroDeleteDirectory() if the thing is a directory, because then, because DELETE_ON_CLOSE has bee set, the FindFirstFile() will fail.

So I'm not sure if there's any way to make it behave exactly like the native fs. In the native fs, calling NtSetInfomationFile() to set the delete on close will fail if the target is an non-empty directory.

I've attached a program "ntdelete.cpp" that deletes a file or directory like cygwin does (or at least very much like cygwin). This program can delete a directory on a native fs but not on a mirror.

It looks to me like Windows doesn't really have any problem with DELETE being specified when opening a directory. And it looks like, for whatever reason, DokanFileInfo->IsDirectory isn't necessary true even if the file is a directory.

Here are the changes I think should be made to mirror.c to make it work with cygwin rmdir and ntdelete.cpp.

diff --git a/samples/dokan_mirror/mirror.c b/samples/dokan_mirror/mirror.c
index 013d481..d5ad5bc 100644
--- a/samples/dokan_mirror/mirror.c
+++ b/samples/dokan_mirror/mirror.c
@@ -248,8 +248,7 @@ MirrorCreateFile(LPCWSTR FileName, PDOKAN_IO_SECURITY_CONTEXT SecurityContext,
   fileAttr = GetFileAttributes(filePath);

   if (fileAttr != INVALID_FILE_ATTRIBUTES &&
-      (fileAttr & FILE_ATTRIBUTE_DIRECTORY &&
-       DesiredAccess != DELETE)) { // Directory cannot be open for DELETE
+      (fileAttr & FILE_ATTRIBUTE_DIRECTORY)) { 
     fileAttributesAndFlags |= FILE_FLAG_BACKUP_SEMANTICS;
     // AccessMode = 0;
   }
@@ -411,7 +410,8 @@ static void DOKAN_CALLBACK MirrorCleanup(LPCWSTR FileName,
     // Should already be deleted by CloseHandle
     // if open with FILE_FLAG_DELETE_ON_CLOSE
     DbgPrint(L"\tDeleteOnClose\n");
-    if (DokanFileInfo->IsDirectory) {
+   DWORD dwAttrib = GetFileAttributes(filePath);
+    if (DokanFileInfo->IsDirectory || (dwAttrib != INVALID_FILE_ATTRIBUTES && (dwAttrib & FILE_ATTRIBUTE_DIRECTORY))) {
       DbgPrint(L"  DeleteDirectory ");
       if (!RemoveDirectory(filePath)) {
         DbgPrint(L"error code = %d\n\n", GetLastError());
@@ -700,6 +700,8 @@ MirrorFindFiles(LPCWSTR FileName,
   return STATUS_SUCCESS;
 }

+
+
 static NTSTATUS DOKAN_CALLBACK
 MirrorDeleteFile(LPCWSTR FileName, PDOKAN_FILE_INFO DokanFileInfo) {
   UNREFERENCED_PARAMETER(DokanFileInfo);
@@ -709,12 +711,8 @@ MirrorDeleteFile(LPCWSTR FileName, PDOKAN_FILE_INFO DokanFileInfo) {

   GetFilePath(filePath, MAX_PATH, FileName);
   DbgPrint(L"DeleteFile %s\n", filePath);
-
-  DWORD dwAttrib = GetFileAttributes(filePath);
-
-  if (dwAttrib != INVALID_FILE_ATTRIBUTES &&
-      (dwAttrib & FILE_ATTRIBUTE_DIRECTORY))
-    return STATUS_ACCESS_DENIED;
+  
+  DbgPrint(L"\tDeleteOnClose\n");

   return STATUS_SUCCESS;
 }
@@ -770,6 +768,7 @@ MirrorDeleteDirectory(LPCWSTR FileName, PDOKAN_FILE_INFO DokanFileInfo) {
   return STATUS_SUCCESS;
 }

+
 static NTSTATUS DOKAN_CALLBACK
 MirrorMoveFile(LPCWSTR FileName, // existing file name
                LPCWSTR NewFileName, BOOL ReplaceIfExisting,

Logs

Here is ntdelete.cpp in a zip file.

ntdelete.zip

[bailey27]

I found a problem. The ntdelete example fails silently if the directory isn't empty. And also cygwing rmdir says "Permission denied" instead of "directory not empty".

So, maybe MirrorCreateFile() schould check to see if the Directory is being opened for DELETE, and if so then call MirrorDeleteDirectory() to make sure it's empty.

If it does this before the directory is opened with DELETE, then FindFirstFile() won't fail.

That would look like this

@@ -248,8 +248,12 @@ MirrorCreateFile(LPCWSTR FileName, PDOKAN_IO_SECURITY_CONTEXT SecurityContext,
   fileAttr = GetFileAttributes(filePath);

   if (fileAttr != INVALID_FILE_ATTRIBUTES &&
-      (fileAttr & FILE_ATTRIBUTE_DIRECTORY &&
-       DesiredAccess != DELETE)) { // Directory cannot be open for DELETE
+      (fileAttr & FILE_ATTRIBUTE_DIRECTORY)) { 
+   if (DesiredAccess == DELETE) {
+        status = MirrorDeleteDirectory(FileName, DokanFileInfo);
+       if (status != STATUS_SUCCESS)
+         return status;
+   }
     fileAttributesAndFlags |= FILE_FLAG_BACKUP_SEMANTICS;
     // AccessMode = 0;
   }

I tried that. And now ntdelete.cpp, cmd rmdir and cygwin rmdir all seem to be working and getting "directory not empty" if the directory isn't.

However, I noticed winfstest is failing two tests

C:\src\msvc\winfstest-master\TestSuite\t\base\02.t
not ok 8 - expect "DeleteFile m:\\570dda43" ERROR_ACCESS_DENIED - got 0
not ok 9 - expect "RemoveDirectory m:\\570dda43" 0 - got ERROR_FILE_NOT_FOUND
not ok 2/10

So it looks like the Windows API does prevent DeleteFile() from being called on directories (returns lasterr = 5, access denied).

But if mirror doesn't allow this, then cygwin rmdir won't work.

Is this checking DELETE in the current mirror.c maybe a thing that was done to pass winfstest?

Maybe there is some way to detect if we're getting there from Windows API DeleteFile() and return the error in that case?

[Rondom]

The same behaviour (no failure when trying to delete non-empty directories) applies to the FUSE mirror

[bailey27]

From looking at the cygwin source, I can see that it seems always use FILE_OPEN_FOR_BACKUP_INTENT when opening a directory in order to delete it.

But we don't get it in the CreateOptions in MirrorOpenFile() when the directory is opened for DELETE.

From looking at the dokan sys and dll code, I don't see where dokan is making that flag disappear.

It could be that my updated-today cygwin is different from what's in their current git source.

If we got that flag in mirror, then I think it would know that it was OK to open the directory DELETE.

[Liryna]

Hi @bailey27 ,

DesiredAccess != DELETE)) { // Directory cannot be open for DELETE

This line is made in prupose (probably can be improved) to fix DeleteFile on Directory that success without it DeleteFile('C:\TMP');.

About flag FILE_OPEN_FOR_BACKUP_INTENT, I think a simple CreateFile tests on the mirror should make it work but as far as I know, the flag should be correctly transmit to the mirror.

If you find nothing until there, I will make a test myself on cygwin this weekend :)

[bailey27]

ntdelete.cpp passes FILE_OPEN_FOR_BACKUP_INTENT to NtOpenFile() but it never makes it to dokany. And the same with cygwin when it does rmdir which ends up in unlink_nt() in cygwin1.dll. I stepped through unlink_nt() in gdb and saw it.

I built the driver with some more debug prints and put a DbgBreakPoint() if the file is opened with DELETE. I saw there in the kernel debugger that there wasn't any FILE_OPEN_FOR_BACKUP_INTENT.

It really looks to me like some driver upstream of dokan1.sys or maybe the OS itself is stripping out FILE_OPEN_FOR_BACKUP_INTENT from the IRP.

I tried it with signed dokan1.sys, signed dokan1.dll, and my fs's .exe self-signed (with cert installed) and running it as administrator, and still no FILE_OPEN_FOR_BACKUP_INTENT makes it through.

[bailey27]

Maybe you could ask them to fix cygwin?

This seems to work

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 0ffb8c1..0d9453e 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -704,6 +704,8 @@ unlink_nt (path_conv &pc)
          pc.get_nt_native_path (), pc.isdir ());
   ACCESS_MASK access = DELETE;
   ULONG flags = FILE_OPEN_FOR_BACKUP_INTENT;
+  if (pc.isdir ())
+     flags |= FILE_DIRECTORY_FILE;
   /* Add the reparse point flag to native symlinks, otherwise we remove the
      target, not the symlink. */
   if (pc.is_rep_symlink ())

[Liryna]

😮 So just by adding the flag as you did in your patch

Bailey@haswell /cygdrive/m
$ mkdir foo

Bailey@haswell /cygdrive/m
$ rmdir foo

does work ?

Humm could you try to add here https://github.com/dokan-dev/dokany/blob/master/samples/dokan_mirror/mirror.c#L257

  if (fileAttr != INVALID_FILE_ATTRIBUTES &&
      (fileAttr & FILE_ATTRIBUTE_DIRECTORY))) { 
       DokanFileInfo->IsDirectory = true;
}

and see if it helps ?

[bailey27]

Yes, if I add the flag in cygwin, then it works.

I tried your code in an original mirror.c and also in my fs (which I haven't changed about this yet) and it doesn't help.

It looks like the problem is that without the FILE_FLAG_BACKUP_SEMANTICS, then the CreateFile() still fails to open the directory.

I think your code would make it so that when the directory does get deleted (assuming most of my changes are put in), then the MirrorDeleteDirectory() will be called automatically and the MirrorCleanup() will delete the directory like it should. So I think it would eliminate the need for a lot of the changes that I suggested.

But I think, assuming it is true that somebody's clearing the FILE_OPEN_FOR_BACKUP_INTENT bit, then I think there's no way to allow cygwin as it is to delete directories and also prevent somebody from doing DeleteFile("c:\empty_dir").

I think this FILE_OPEN_FOR_BACKUP_INTENT / FILE_FLAG_BACKUP_SEMANTICS flag is like some way to be able to open the file or directory for delete without caring about whether it's a file or a directory. But I think maybe MS didn't intend for it to be used like that. Or maybe they don't anymore.

It sounds to me like FILE_OPEN_FOR_BACKUP_INTENT has something to do with security, and maybe that's whey the OS or some driver is preventing dokany from getting it.

I tried a registry hack to prevent any Windows Defender stuff from loading in my VM. I could see that after that then WdFilter.sys wasn't loaded in the filter stack. But still I got no FILE_OPEN_FOR_BACKUP_INTENT passed through to dokany. So I think it isn't the defender filter driver that's clearing the bit.

[Liryna]

@Rondom I have added an issue for the behaviour that you describe: #270

C Mirror does have the check here:

dokany/samples/dokan_mirror/mirror.c

Line 757 in bbbdbcb

return STATUS_DIRECTORY_NOT_EMPTY;

[bailey27]

If I add CreateOptions |= FILE_DIRECTORY_FILE to your change, then it makes cygwin work all by itself.

if (fileAttr != INVALID_FILE_ATTRIBUTES &&
      (fileAttr & FILE_ATTRIBUTE_DIRECTORY)) { 
    DokanFileInfo->IsDirectory = TRUE;
       CreateOptions |= FILE_DIRECTORY_FILE;
  }

If that code is there, then what happens in mirror is I think exactly like what happens if cygwin uses FILE_DIRECTORY_FILE like how I changed it in cygwin.

What happens then is that mirror gets into this code

if ((CreateOptions & FILE_DIRECTORY_FILE) == FILE_DIRECTORY_FILE) {
    // It is a create directory request
.

And it opens the directory like this

handle = CreateFile(filePath, 0, FILE_SHARE_READ | FILE_SHARE_WRITE,
                          &securityAttrib, OPEN_EXISTING,
                          FILE_FLAG_BACKUP_SEMANTICS, NULL);

So it opens it with desiredAccess 0 and read and write sharing.

And because DokanFileInfo->IsDirectory is true, then when cygwin calls NtSetInformationFile() to delete the directory and then MirroDeleteDirectory() is called, then FindFirstFile() doesn't fail because there is no sharing violation because the directory was opened differently than specified (without DELETE and with sharing).

But, this causes a third winfstest failure.

C:\src\msvc\winfstest-master\TestSuite>run-winfstest.bat  m:\
C:\src\msvc\winfstest-master\TestSuite\t\base\00.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\01.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\02.t
not ok 8 - expect "DeleteFile m:\\e5ca4fd7" ERROR_ACCESS_DENIED - got 0
not ok 9 - expect "RemoveDirectory m:\\e5ca4fd7" 0 - got ERROR_FILE_NOT_FOUND
not ok 2/10
C:\src\msvc\winfstest-master\TestSuite\t\base\03.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\04.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\05.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\06.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\07.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\08.t  ok
C:\src\msvc\winfstest-master\TestSuite\t\base\09.t
not ok 17 - expect "CreateFile m:\\0302478a GENERIC_READ 0 0 OPEN_EXISTING FILE_FLAG_BACKUP_SEMANTICS 0" ERROR_SHARING_VIOLATION - got 0
not ok 1/19

total ................................. ok 168/171 - not ok 3/171

It fails because it opens the directory for read with no sharing and then tries to open it for read again and unexpectedly succeeds.

I take back everything I said about FILE_FLAG_BACKUP_SEMANTICS maybe not really being intended by MS for opening directories like files. I saw somewhere that they say it is for that.

But, I'm wondering if Dokany ever got FILE_OPEN_FOR_BACKUP_INTENT in an Irp before?

If it was getting that, then why was this code needed (from the real current mirror.c)?

if (fileAttr != INVALID_FILE_ATTRIBUTES &&
      (fileAttr & FILE_ATTRIBUTE_DIRECTORY &&
       DesiredAccess != DELETE)) { // Directory cannot be open for DELETE
    fileAttributesAndFlags |= FILE_FLAG_BACKUP_SEMANTICS;
    // AccessMode = 0;
  }

I mean the part about or-ing in FILE_FLAG_BACKUP_SEMANTICS.

I think if somebody tried to open a directory as a file on purpose with CreateFile(), they would specify FILE_FLAG_BACKUP_SEMANTICS, and if that was getting to Dokany via FILE_OPEN_FOR_BACKUP_INTENT in the driver, then it would work without having to or in the flag in MirrorCreateFile().

[Liryna]

Hi @bailey27 ,

I have currently a fix in local for your issue but I have the issue with DeleteFile that success on directory :(

Current patch: http://pastebin.com/pyAsVeRp

[Liryna]

EDIT: FILE_FLAG_BACKUP_SEMANTICS has to be added otherwise explorer would get an acces denied for every open folde request.

I also dont know if dokan sys does never get FILE_OPEN_FOR_BACKUP_INTENT

I agree normally if a software want to open a folder, he should use FILE_FLAG_BACKUP_SEMANTICS but explorer only request:

###Create 0112
CreateFile : C:\Users\
  AccountName: liryna, DomainName: DESKTOP-T9B1IFA
        ShareMode = 0x7
        FILE_SHARE_READ
        FILE_SHARE_WRITE
        FILE_SHARE_DELETE
        AccessMode = 0x20000
        READ_CONTROL
        STANDARD_RIGHTS_READ
        STANDARD_RIGHTS_WRITE
        STANDARD_RIGHTS_EXECUTE
        FlagsAndAttributes = 0x0
        OPEN_EXISTING
        error code = 3


CreateFile status = c000003a - lastError = 3 

[Liryna]

Current fix on my side: Liryna@dd175d2
If all test pass and it work the cygwin isssue on your side, I will merge it.

[Liryna]

All test pass for me with Liryna@2ecbb93

[bailey27]

I tried it, and cygwin rmdir didn't work

Bailey@haswell /cygdrive/m
$ mkdir foo

Bailey@haswell /cygdrive/m
$ rmdir foo
rmdir: failed to remove 'foo': Device or resource busy

I got in the debug output

DeleteDirectory c:\tmp\mirror\foo
PathIsDirectoryEmpty failed 32
        DispatchSetInformation result =  c0000043

I think PathIsDirectoryEmpty() has the same sharing problem as FindFirstFile().

I tried this

diff --git a/samples/dokan_mirror/mirror.c b/samples/dokan_mirror/mirror.c
index 91680ad..25b95ed 100644
--- a/samples/dokan_mirror/mirror.c
+++ b/samples/dokan_mirror/mirror.c
@@ -252,6 +252,9 @@ MirrorCreateFile(LPCWSTR FileName, PDOKAN_IO_SECURITY_CONTEXT SecurityContext,
       (fileAttr & FILE_ATTRIBUTE_DIRECTORY) &&
       !(CreateOptions & FILE_NON_DIRECTORY_FILE)) {
     DokanFileInfo->IsDirectory = TRUE;
+   if (DesiredAccess == DELETE) {
+       ShareAccess |= FILE_SHARE_READ;
+   }
   }

   DbgPrint(L"\tFlagsAndAttributes = 0x%x\n", fileAttributesAndFlags);

And now cygwin rmdir works and all the winfstest tests still pass.

Do you think it's OK to put the FILE_SHARE_READ there if they open the directory for DELETE?

[Liryna]

😮 I was sure that PathIsDirectoryEmpty didn't had this issue, that why I replaced it 😢
Adding FILE_SHARE_READ will fix the issue but will probably somewhere create an issue 😢

Would be great to find a solution to see if directory is empty without having this sharing violation.

[Liryna]

During Delete we have FILE_READ_ATTRIBUTES access, it would be great to use the CreateFile Handle to get information some how about the directory being empty or not but I find nothing in windows api like that...
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365258(v=vs.85).aspx

[bailey27]

For my fs, I need to treat a directory as empty if it has only a gocryptfs.diriv file in it.

It wouldn't help me to know only if the directory is really empty or not.

[Liryna]

Ok, I pushed the patch with you fix so e49cf5b

Thanks again @bailey27 for your time and your report !

[bailey27]

Thanks for fixing it.

[Corillian]

I'm reviewing this code for #210 and the current solution, while it seems to resolve the issue with cygwin, does not seem to be the proper solution. Based on the documentation for FILE_OPEN_FOR_BACKUP_INTENT wouldn't it be possible that a FILE_OPEN_FOR_BACKUP_INTENT flag isn't passed to the driver because it is reflected in the security attributes/descriptor that are provided? The documentation mentions that this flag only affects access rights and that it's not used by device and intermediate drivers.

[marinkobabic]

Why not log this information in the create.c to see if the driver get's this information?

if (irpSp->Parameters.Create.Options & FILE_OPEN_FOR_BACKUP_INTENT) {
DDbgPrint("FILE_OPEN_FOR_BACKUP_INTENT\n");
}

[Liryna]

FILE_OPEN_FOR_BACKUP_INTENT isn't passed to the driver.

There is only SL_OPEN_TARGET_DIRECTORY flag set to request that the file's parent directory should be opened.

It mean that dokan does have to handle himself SL_OPEN_TARGET_DIRECTORY by opening the parent directory with the flag FILE_OPEN_FOR_BACKUP_INTENT.

@Corillian , how do you think this should be handled ?

Fastfat code:
https://github.com/Microsoft/Windows-driver-samples/blob/6e1115d6af3c22b693eccc472ebb0d53a1905cd9/filesys/fastfat/create.c#L602

[bailey27]

@marinkobabic ,

I put that print in the driver. I deleted a directory with cygwin and my own ntdelete.cpp, which I know uses this flag to open the file, and I didn't see the print.

@Corillian,
I think you are right. I think that explains why the driver never sees the flag.

I think it might be possible to deduce from the security attributes/descriptor that this flag was used for the create. But I think finding it out would be hard. And also, it might not be definitive. Those settings, however unusual, might be there in a case in which the caller didn't actually specify FILE_OPEN_FOR_BACKUP_INTENT.

What is it that you don't like about the current solution?

I think some of the complexity is there to prevent DeleteFile("m:\empty_dir") from working. If that works, then one of the winfs tests fails.

[bailey27]

This is really weird.

I ran filespy.

The first time I tried it, I saw FILE_OPEN_FOR_BACKUP_INTENT in the IRP_MJ_CREATE when cygwin opened the the dir on C: to delete it.

I tried it on my mirror drive, and the flag was not there.

I saved a screenshot of this, so I know it like that.

Then I shut down my VM and added a new disk to it so I could try it with fat32.

Now I don't see the flag on c: (ntfs), f: (fat32) or m: (mirror).

Also, I found out that when you do a CreateFile(), if you don't specify FILE_FLAG_BACKUP_SEMANTICS, then win32 automatically adds FILE_NON_DIRECTORY_FILE.

If you use NtOpenFile() to open a dir for delete, then you don't even need FILE_OPEN_FOR_BACKUP_INTENT to open it, and you can still delete it.

[Corillian]

If FILE_OPEN_FOR_BACKUP_INTENT is represented in the security flags and the security descriptor then all mirror should have to do is pass those flags and the descriptor to CreateFile(), you wouldn't need to supply FILE_OPEN_FOR_BACKUP_INTENT to it.

Too bad MS hasn't open sourced their NTFS driver =(