{"action":"created","issue":{"url":"https://api.github.com/repos/dokan-dev/dokany/issues/269","repository_url":"https://api.github.com/repos/dokan-dev/dokany","labels_url":"https://api.github.com/repos/dokan-dev/dokany/issues/269/labels{/name}","comments_url":"https://api.github.com/repos/dokan-dev/dokany/issues/269/comments","events_url":"https://api.github.com/repos/dokan-dev/dokany/issues/269/events","html_url":"https://github.com/dokan-dev/dokany/issues/269","id":162033473,"number":269,"title":"cygwin bash cannot delete directories on mirror drive","user":{"login":"bailey27","id":17813578,"avatar_url":"https://avatars.githubusercontent.com/u/17813578?v=3","gravatar_id":"","url":"https://api.github.com/users/bailey27","html_url":"https://github.com/bailey27","followers_url":"https://api.github.com/users/bailey27/followers","following_url":"https://api.github.com/users/bailey27/following{/other_user}","gists_url":"https://api.github.com/users/bailey27/gists{/gist_id}","starred_url":"https://api.github.com/users/bailey27/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bailey27/subscriptions","organizations_url":"https://api.github.com/users/bailey27/orgs","repos_url":"https://api.github.com/users/bailey27/repos","events_url":"https://api.github.com/users/bailey27/events{/privacy}","received_events_url":"https://api.github.com/users/bailey27/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2016-06-23T22:01:09Z","updated_at":"2016-06-25T12:35:50Z","closed_at":null,"body":"\r\n# Environment\r\n * Windows version: Windows 10\r\n * Processor architecture: x64\r\n * Dokany version: 1.0.0-RC3\r\n * Library type (Dokany/FUSE): Dokany \r\n \r\n# Check List\r\n  - [x ] I checked my issue [doesn't exist yet](https://github.com/dokan-dev/dokany/issues?utf8=%E2%9C%93&q=is%3Aissue)\r\n  - [x ] My issue is valid with mirror default sample and not specific to my user-mode driver implementation\r\n  - [x ] I can always reproduce the issue with the provided description below.\r\n  - [x ] I have updated Dokany to the latest version (https://github.com/dokan-dev/dokany/releases) and have reboot my computer after.\r\n\r\n# Description\r\n\r\nIf you use cygwin bash on a mirror drive and try to mkdir a directory and then try to rmdir it, you get \"Permission denied\".\r\n\r\n```\r\nM:\\>mkdir foo\r\n\r\nM:\\>rmdir foo\r\n\r\nM:\\>bash\r\n\r\nBailey@haswell /cygdrive/m\r\n$ mkdir foo\r\n\r\nBailey@haswell /cygdrive/m\r\n$ rmdir foo\r\nrmdir: failed to remove 'foo': Permission denied\r\n```\r\n\r\nThe problem seems to be that cygwin is using the native NT APIs.\r\n\r\nThe 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).\r\n\r\nmirror.c has three problems with this.  \r\n\r\n1. It thinks a directory should not be opened with DELETE access.\r\n\r\n2. If MirrorDeleteFile() is called on a directory, then it returns an error.  It should check the fiile\r\nattributes, and if it's a directory, then call MirrorDeleteDirectory().\r\n\r\n3. It uses only DokanFileInfo->IsDirectory to tell if the thing that should be deleted on close is a directory or a file.\r\n\r\nIt 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.\r\n\r\nSo 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.  \r\n\r\nI'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.\r\n\r\nIt 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.  \r\n\r\nHere are the changes I think should be made to mirror.c to make it work with cygwin rmdir and ntdelete.cpp.\r\n\r\n```\r\ndiff --git a/samples/dokan_mirror/mirror.c b/samples/dokan_mirror/mirror.c\r\nindex 013d481..d5ad5bc 100644\r\n--- a/samples/dokan_mirror/mirror.c\r\n+++ b/samples/dokan_mirror/mirror.c\r\n@@ -248,8 +248,7 @@ MirrorCreateFile(LPCWSTR FileName, PDOKAN_IO_SECURITY_CONTEXT SecurityContext,\r\n   fileAttr = GetFileAttributes(filePath);\r\n \r\n   if (fileAttr != INVALID_FILE_ATTRIBUTES &&\r\n-      (fileAttr & FILE_ATTRIBUTE_DIRECTORY &&\r\n-       DesiredAccess != DELETE)) { // Directory cannot be open for DELETE\r\n+      (fileAttr & FILE_ATTRIBUTE_DIRECTORY)) { \r\n     fileAttributesAndFlags |= FILE_FLAG_BACKUP_SEMANTICS;\r\n     // AccessMode = 0;\r\n   }\r\n@@ -411,7 +410,8 @@ static void DOKAN_CALLBACK MirrorCleanup(LPCWSTR FileName,\r\n     // Should already be deleted by CloseHandle\r\n     // if open with FILE_FLAG_DELETE_ON_CLOSE\r\n     DbgPrint(L\"\\tDeleteOnClose\\n\");\r\n-    if (DokanFileInfo->IsDirectory) {\r\n+\tDWORD dwAttrib = GetFileAttributes(filePath);\r\n+    if (DokanFileInfo->IsDirectory || (dwAttrib != INVALID_FILE_ATTRIBUTES && (dwAttrib & FILE_ATTRIBUTE_DIRECTORY))) {\r\n       DbgPrint(L\"  DeleteDirectory \");\r\n       if (!RemoveDirectory(filePath)) {\r\n         DbgPrint(L\"error code = %d\\n\\n\", GetLastError());\r\n@@ -700,6 +700,8 @@ MirrorFindFiles(LPCWSTR FileName,\r\n   return STATUS_SUCCESS;\r\n }\r\n \r\n+\r\n+\r\n static NTSTATUS DOKAN_CALLBACK\r\n MirrorDeleteFile(LPCWSTR FileName, PDOKAN_FILE_INFO DokanFileInfo) {\r\n   UNREFERENCED_PARAMETER(DokanFileInfo);\r\n@@ -709,12 +711,8 @@ MirrorDeleteFile(LPCWSTR FileName, PDOKAN_FILE_INFO DokanFileInfo) {\r\n \r\n   GetFilePath(filePath, MAX_PATH, FileName);\r\n   DbgPrint(L\"DeleteFile %s\\n\", filePath);\r\n-\r\n-  DWORD dwAttrib = GetFileAttributes(filePath);\r\n-\r\n-  if (dwAttrib != INVALID_FILE_ATTRIBUTES &&\r\n-      (dwAttrib & FILE_ATTRIBUTE_DIRECTORY))\r\n-    return STATUS_ACCESS_DENIED;\r\n+  \r\n+  DbgPrint(L\"\\tDeleteOnClose\\n\");\r\n \r\n   return STATUS_SUCCESS;\r\n }\r\n@@ -770,6 +768,7 @@ MirrorDeleteDirectory(LPCWSTR FileName, PDOKAN_FILE_INFO DokanFileInfo) {\r\n   return STATUS_SUCCESS;\r\n }\r\n \r\n+\r\n static NTSTATUS DOKAN_CALLBACK\r\n MirrorMoveFile(LPCWSTR FileName, // existing file name\r\n                LPCWSTR NewFileName, BOOL ReplaceIfExisting,\r\n ```\r\n\r\n# Logs\r\nHere is ntdelete.cpp in a zip file.\r\n\r\n[ntdelete.zip](https://github.com/dokan-dev/dokany/files/330965/ntdelete.zip)\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/dokan-dev/dokany/issues/comments/228538078","html_url":"https://github.com/dokan-dev/dokany/issues/269#issuecomment-228538078","issue_url":"https://api.github.com/repos/dokan-dev/dokany/issues/269","id":228538078,"user":{"login":"bailey27","id":17813578,"avatar_url":"https://avatars.githubusercontent.com/u/17813578?v=3","gravatar_id":"","url":"https://api.github.com/users/bailey27","html_url":"https://github.com/bailey27","followers_url":"https://api.github.com/users/bailey27/followers","following_url":"https://api.github.com/users/bailey27/following{/other_user}","gists_url":"https://api.github.com/users/bailey27/gists{/gist_id}","starred_url":"https://api.github.com/users/bailey27/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bailey27/subscriptions","organizations_url":"https://api.github.com/users/bailey27/orgs","repos_url":"https://api.github.com/users/bailey27/repos","events_url":"https://api.github.com/users/bailey27/events{/privacy}","received_events_url":"https://api.github.com/users/bailey27/received_events","type":"User","site_admin":false},"created_at":"2016-06-25T12:35:50Z","updated_at":"2016-06-25T12:35:50Z","body":"If I add  CreateOptions |= FILE_DIRECTORY_FILE to your change, then it makes cygwin work all by itself.\r\n\r\n```\r\nif (fileAttr != INVALID_FILE_ATTRIBUTES &&\r\n      (fileAttr & FILE_ATTRIBUTE_DIRECTORY)) { \r\n\tDokanFileInfo->IsDirectory = TRUE;\r\n       CreateOptions |= FILE_DIRECTORY_FILE;\r\n  }\r\n```\r\n\r\nIf 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.\r\n\r\nWhat happens then is that mirror gets into this code\r\n```\r\nif ((CreateOptions & FILE_DIRECTORY_FILE) == FILE_DIRECTORY_FILE) {\r\n    // It is a create directory request\r\n.\r\n\r\n```\r\n\r\nAnd it opens the directory like this\r\n\r\n```\r\nhandle = CreateFile(filePath, 0, FILE_SHARE_READ | FILE_SHARE_WRITE,\r\n                          &securityAttrib, OPEN_EXISTING,\r\n                          FILE_FLAG_BACKUP_SEMANTICS, NULL);\r\n```\r\n\r\nSo it opens it with desiredAccess 0 and read and write sharing.\r\n\r\nAnd 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).\r\n\r\nBut, this causes a third winfstest failure.\r\n\r\n```\r\nC:\\src\\msvc\\winfstest-master\\TestSuite>run-winfstest.bat  m:\\\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\00.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\01.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\02.t\r\nnot ok 8 - expect \"DeleteFile m:\\\\e5ca4fd7\" ERROR_ACCESS_DENIED - got 0\r\nnot ok 9 - expect \"RemoveDirectory m:\\\\e5ca4fd7\" 0 - got ERROR_FILE_NOT_FOUND\r\nnot ok 2/10\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\03.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\04.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\05.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\06.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\07.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\08.t  ok\r\nC:\\src\\msvc\\winfstest-master\\TestSuite\\t\\base\\09.t\r\nnot ok 17 - expect \"CreateFile m:\\\\0302478a GENERIC_READ 0 0 OPEN_EXISTING FILE_FLAG_BACKUP_SEMANTICS 0\" ERROR_SHARING_VIOLATION - got 0\r\nnot ok 1/19\r\n\r\ntotal ................................. ok 168/171 - not ok 3/171\r\n```\r\n\r\nIt fails because it opens the directory for read with no sharing and then tries to open it for read again and unexpectedly succeeds.\r\n\r\nI 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.\r\n\r\nBut, I'm wondering if Dokany ever got FILE_OPEN_FOR_BACKUP_INTENT in an Irp before?\r\n\r\nIf it was getting that, then why was this code needed (from the real current mirror.c)?\r\n\r\n```\r\nif (fileAttr != INVALID_FILE_ATTRIBUTES &&\r\n      (fileAttr & FILE_ATTRIBUTE_DIRECTORY &&\r\n       DesiredAccess != DELETE)) { // Directory cannot be open for DELETE\r\n    fileAttributesAndFlags |= FILE_FLAG_BACKUP_SEMANTICS;\r\n    // AccessMode = 0;\r\n  }\r\n```\r\n\r\nI mean the part about or-ing in FILE_FLAG_BACKUP_SEMANTICS.\r\n\r\nI 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().\r\n\r\n\r\n\r\n\r\n\r\n"}}
{"actor":{"display_login":"bailey27"}}