From 00267ba07550fc8d57aa2e184d265bc42ddbce26 Mon Sep 17 00:00:00 2001 From: isanae <14251494+isanae@users.noreply.github.com> Date: Tue, 18 Jun 2019 19:34:13 -0400 Subject: [PATCH 1/3] close file handle opened by hook_NtQueryDirectoryFile[Ex]() my _guess_ is that if NtQueryDirectoryFile() is not called until STATUS_NO_MORE_FILES, the handle is leaked because NtClose() doesn't take care of it --- src/usvfs_dll/hooks/ntdll.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/usvfs_dll/hooks/ntdll.cpp b/src/usvfs_dll/hooks/ntdll.cpp index b7badb91..419ec610 100644 --- a/src/usvfs_dll/hooks/ntdll.cpp +++ b/src/usvfs_dll/hooks/ntdll.cpp @@ -1299,6 +1299,10 @@ NTSTATUS WINAPI usvfs::hook_NtClose(HANDLE Handle) // std::lock_guard lock(activeSearches.queryMutex); auto iter = activeSearches.info.find(Handle); if (iter != activeSearches.info.end()) { + if (iter->second.currentSearchHandle != INVALID_HANDLE_VALUE) { + ::CloseHandle(iter->second.currentSearchHandle); + } + activeSearches.info.erase(iter); log = true; } From dadaf3c252aabbd438e860b077150f56dd702409 Mon Sep 17 00:00:00 2001 From: isanae <14251494+isanae@users.noreply.github.com> Date: Wed, 19 Jun 2019 22:40:07 -0400 Subject: [PATCH 2/3] fixed removing nodes when they're not found this actually happens when moving files across volumes --- src/shared/directory_tree.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/shared/directory_tree.h b/src/shared/directory_tree.h index 0e57037c..99f9b354 100644 --- a/src/shared/directory_tree.h +++ b/src/shared/directory_tree.h @@ -449,7 +449,9 @@ class DirectoryTree if (auto par = parent()) { spdlog::get("usvfs")->info("remove from tree {}", m_Name.c_str()); auto self = par->m_Nodes.find(m_Name.c_str()); - par->erase(self); + if (self != par->m_Nodes.end()) { + par->erase(self); + } } } From 3b2a0049c0226c09ee0ad606041d0702622f36a5 Mon Sep 17 00:00:00 2001 From: Al Date: Sat, 22 Jun 2019 13:07:01 +0200 Subject: [PATCH 3/3] * Added log line in case removeFromTree is asked to remove non existing nodes. * Added various TODOs and comments regarding MoveFile functions and missing context.isActive() checks. --- src/shared/directory_tree.h | 5 +++++ src/usvfs_dll/hooks/kernel32.cpp | 11 ++++++++++- src/usvfs_dll/hooks/ntdll.cpp | 2 ++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/shared/directory_tree.h b/src/shared/directory_tree.h index 99f9b354..94d5a7c6 100644 --- a/src/shared/directory_tree.h +++ b/src/shared/directory_tree.h @@ -452,6 +452,11 @@ class DirectoryTree if (self != par->m_Nodes.end()) { par->erase(self); } + else { + //trying to remove a node that des not exist, most likely because it was already removed in a lower level call. + //this is known to happen when MoveFile has the MOVEFILE_COPY_ALLOWED flag and moving a mapped file. + spdlog::get("usvfs")->warn("Failed to remove inexisting node from tree: {}", m_Name.c_str()); + } } } diff --git a/src/usvfs_dll/hooks/kernel32.cpp b/src/usvfs_dll/hooks/kernel32.cpp index 66112308..1c00edca 100644 --- a/src/usvfs_dll/hooks/kernel32.cpp +++ b/src/usvfs_dll/hooks/kernel32.cpp @@ -139,6 +139,7 @@ HMODULE WINAPI usvfs::hook_LoadLibraryExW(LPCWSTR lpFileName, HANDLE hFile, HMODULE res = nullptr; HOOK_START_GROUP(MutExHookGroup::LOAD_LIBRARY) + // Why is the usual if (!callContext.active()... check missing? RerouteW reroute = RerouteW::create(READ_CONTEXT(), callContext, lpFileName); PRE_REALCALL @@ -488,6 +489,7 @@ DWORD WINAPI usvfs::hook_SetFileAttributesW( DWORD res = 0UL; HOOK_START_GROUP(MutExHookGroup::FILE_ATTRIBUTES) + // Why is the usual if (!callContext.active()... check missing? RerouteW reroute = RerouteW::create(READ_CONTEXT(), callContext, lpFileName); PRE_REALCALL @@ -508,6 +510,7 @@ BOOL WINAPI usvfs::hook_DeleteFileW(LPCWSTR lpFileName) BOOL res = FALSE; HOOK_START_GROUP(MutExHookGroup::DELETE_FILE) + // Why is the usual if (!callContext.active()... check missing? RerouteW reroute = RerouteW::create(READ_CONTEXT(), callContext, lpFileName); @@ -800,6 +803,9 @@ BOOL WINAPI usvfs::hook_MoveFileWithProgressA(LPCSTR lpExistingFileName, LPCSTR BOOL WINAPI usvfs::hook_MoveFileWithProgressW(LPCWSTR lpExistingFileName, LPCWSTR lpNewFileName, LPPROGRESS_ROUTINE lpProgressRoutine, LPVOID lpData, DWORD dwFlags) { + + //TODO: Remove all redundant hooks to moveFile alternatives. + //it would appear that all other moveFile functions end up calling this one with no additional code. BOOL res = FALSE; HOOK_START_GROUP(MutExHookGroup::SHELL_FILEOP) @@ -861,7 +867,9 @@ BOOL WINAPI usvfs::hook_MoveFileWithProgressW(LPCWSTR lpExistingFileName, LPCWST writeReroute.updateResult(callContext, res); if (res) { - readReroute.removeMapping(READ_CONTEXT(), isDirectory); // Updating the rerouteCreate to check deleted file entries should make this okay + //TODO: this call causes the node to be removed twice in case of MOVEFILE_COPY_ALLOWED as the deleteFile hook lower level already takes care of it, + //but deleteFile can't be disabled since we are relying on it in case of MOVEFILE_REPLACE_EXISTING for the destination file. + readReroute.removeMapping(READ_CONTEXT(), isDirectory); // Updating the rerouteCreate to check deleted file entries should make this okay (not related to comments above) if (writeReroute.newReroute()) { if (isDirectory) @@ -1094,6 +1102,7 @@ DLLEXPORT BOOL WINAPI usvfs::hook_RemoveDirectoryW( BOOL res = FALSE; HOOK_START_GROUP(MutExHookGroup::DELETE_FILE) + // Why is the usual if (!callContext.active()... check missing? RerouteW reroute = RerouteW::create(READ_CONTEXT(), callContext, lpPathName); diff --git a/src/usvfs_dll/hooks/ntdll.cpp b/src/usvfs_dll/hooks/ntdll.cpp index 419ec610..4e3618a5 100644 --- a/src/usvfs_dll/hooks/ntdll.cpp +++ b/src/usvfs_dll/hooks/ntdll.cpp @@ -1075,6 +1075,7 @@ NTSTATUS ntdll_mess_NtOpenFile(PHANDLE FileHandle, NTSTATUS res = STATUS_NO_SUCH_FILE; HOOK_START_GROUP(MutExHookGroup::OPEN_FILE) + // Why is the usual if (!callContext.active()... check missing? bool storePath = false; if (((OpenOptions & FILE_DIRECTORY_FILE) != 0UL) @@ -1344,6 +1345,7 @@ NTSTATUS WINAPI usvfs::hook_NtQueryAttributesFile( NTSTATUS res = STATUS_SUCCESS; HOOK_START_GROUP(MutExHookGroup::FILE_ATTRIBUTES) + // Why is the usual if (!callContext.active()... check missing? UnicodeString inPath = CreateUnicodeString(ObjectAttributes);