From 5210c51be8039ed33d53f8e3b16c78e838f7d01f Mon Sep 17 00:00:00 2001 From: Phil Dennis-Jordan Date: Wed, 5 Jun 2019 16:24:32 +0200 Subject: [PATCH 1/6] Moves vnode eligibility check to start of HandleVnodeOperation This change moves the vnode eligibility check (file system type, vnode type) from ShouldHandleVnodeOpEvent to HandleVnodeOperation to avoid repeated calls. A unit test has been added to provide test coverage for the new code branch. --- ProjFS.Mac/PrjFSKext/KauthHandler.cpp | 17 ++++++++--------- ProjFS.Mac/PrjFSKext/public/PrjFSPerfCounter.h | 2 +- .../PrjFSKextTests/HandleOperationTests.mm | 16 ++++++++++++++++ .../PrjFSLib/prjfs-log/kext-perf-tracing.cpp | 2 +- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/ProjFS.Mac/PrjFSKext/KauthHandler.cpp b/ProjFS.Mac/PrjFSKext/KauthHandler.cpp index ac8353d460..ceb4bca736 100644 --- a/ProjFS.Mac/PrjFSKext/KauthHandler.cpp +++ b/ProjFS.Mac/PrjFSKext/KauthHandler.cpp @@ -248,6 +248,14 @@ KEXT_STATIC int HandleVnodeOperation( bool isDeleteAction = false; bool isDirectory = false; + { + PerfSample considerVnodeSample(&perfTracer, PrjFSPerfCounter_VnodeOp_BasicVnodeChecks); + if (!VnodeIsEligibleForEventHandling(currentVnode)) + { + goto CleanupAndReturn; + } + } + if (!ShouldHandleVnodeOpEvent( &perfTracer, context, @@ -823,15 +831,6 @@ KEXT_STATIC bool ShouldHandleVnodeOpEvent( } } - { - PerfSample considerVnodeSample(perfTracer, PrjFSPerfCounter_VnodeOp_ShouldHandle_BasicVnodeChecks); - if (!VnodeIsEligibleForEventHandling(vnode)) - { - *kauthResult = KAUTH_RESULT_DEFER; - return false; - } - } - { PerfSample readFlagsSample(perfTracer, PrjFSPerfCounter_VnodeOp_ShouldHandle_ReadFileFlags); if (!TryReadVNodeFileFlags(vnode, context, vnodeFileFlags)) diff --git a/ProjFS.Mac/PrjFSKext/public/PrjFSPerfCounter.h b/ProjFS.Mac/PrjFSKext/public/PrjFSPerfCounter.h index 0d4bda8e46..ded0712590 100644 --- a/ProjFS.Mac/PrjFSKext/public/PrjFSPerfCounter.h +++ b/ProjFS.Mac/PrjFSKext/public/PrjFSPerfCounter.h @@ -7,10 +7,10 @@ enum PrjFSPerfCounter : int32_t PrjFSPerfCounter_VnodeOp, PrjFSPerfCounter_VnodeOp_GetPath, + PrjFSPerfCounter_VnodeOp_BasicVnodeChecks, PrjFSPerfCounter_VnodeOp_ShouldHandle, PrjFSPerfCounter_VnodeOp_ShouldHandle_IsVnodeAccessCheck, PrjFSPerfCounter_VnodeOp_ShouldHandle_IgnoredVnodeAccessCheck, - PrjFSPerfCounter_VnodeOp_ShouldHandle_BasicVnodeChecks, PrjFSPerfCounter_VnodeOp_ShouldHandle_ReadFileFlags, PrjFSPerfCounter_VnodeOp_ShouldHandle_NotInAnyRoot, PrjFSPerfCounter_VnodeOp_ShouldHandle_CheckFileSystemCrawler, diff --git a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm index ca5366cf44..44ef09a24a 100644 --- a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm +++ b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm @@ -853,5 +853,21 @@ - (void) testWriteDataToForkedPath { XCTAssertTrue(MockCalls::CallCount(ProviderMessaging_TrySendRequestAndWaitForResponse) == 0); } +- (void) testIneligibleFilesystemType +{ + shared_ptr testMountNone = mount::Create("msdos", fsid_t{}, 0); + shared_ptr testVnodeNone = vnode::Create(testMountNone, "/Volumes/USBSTICK"); + XCTAssertEqual( + KAUTH_RESULT_DEFER, + HandleVnodeOperation( + nullptr, + nullptr, + KAUTH_VNODE_ADD_FILE, + reinterpret_cast(context), + reinterpret_cast(testVnodeNone.get()), + 0, + 0)); + XCTAssertFalse(MockCalls::DidCallFunction(ProviderMessaging_TrySendRequestAndWaitForResponse)); +} @end diff --git a/ProjFS.Mac/PrjFSLib/prjfs-log/kext-perf-tracing.cpp b/ProjFS.Mac/PrjFSLib/prjfs-log/kext-perf-tracing.cpp index 1e3bdfa9f8..f600e9f927 100644 --- a/ProjFS.Mac/PrjFSLib/prjfs-log/kext-perf-tracing.cpp +++ b/ProjFS.Mac/PrjFSLib/prjfs-log/kext-perf-tracing.cpp @@ -31,10 +31,10 @@ static constexpr const char* const PerfCounterNames[PrjFSPerfCounter_Count] = { [PrjFSPerfCounter_VnodeOp] = "HandleVnodeOperation", [PrjFSPerfCounter_VnodeOp_GetPath] = " |--GetPath", + [PrjFSPerfCounter_VnodeOp_BasicVnodeChecks] = " |--BasicVnodeChecks", [PrjFSPerfCounter_VnodeOp_ShouldHandle] = " |--ShouldHandleVnodeOpEvent", [PrjFSPerfCounter_VnodeOp_ShouldHandle_IsVnodeAccessCheck] = " | |--IsVnodeAccessCheck", [PrjFSPerfCounter_VnodeOp_ShouldHandle_IgnoredVnodeAccessCheck] = " | | |--IgnoredVnodeAccessCheck", - [PrjFSPerfCounter_VnodeOp_ShouldHandle_BasicVnodeChecks] = " | |--BasicVnodeChecks", [PrjFSPerfCounter_VnodeOp_ShouldHandle_ReadFileFlags] = " | |--TryReadVNodeFileFlags", [PrjFSPerfCounter_VnodeOp_ShouldHandle_NotInAnyRoot] = " | | |--NotInAnyRoot", [PrjFSPerfCounter_VnodeOp_ShouldHandle_CheckFileSystemCrawler] = " | |--IsFileSystemCrawler", From 472f4e179815e43be7cc989cd41a4ec49b31cd4f Mon Sep 17 00:00:00 2001 From: Phil Dennis-Jordan Date: Fri, 7 Jun 2019 14:17:38 +0200 Subject: [PATCH 2/6] Mac ProjFS kext: Adds enum for darwin major versions This adds an enum scoped in a namespace to be used instead of magic numbers in version checks. The reasons for choosing a classic enum in a namespace are: * The namespace gives us explicit scoping rather than polluting the root namespace. * The using a classic enum vs a modern C++ enum class means the values are implicitly convertible to integers, which is how we actually use them. --- ProjFS.Mac/PrjFSKext/public/PrjFSCommon.h | 10 ++++++++++ ProjFS.Mac/PrjFSLib/PrjFSUser.cpp | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/ProjFS.Mac/PrjFSKext/public/PrjFSCommon.h b/ProjFS.Mac/PrjFSKext/public/PrjFSCommon.h index abd70711c0..54965a41d8 100644 --- a/ProjFS.Mac/PrjFSKext/public/PrjFSCommon.h +++ b/ProjFS.Mac/PrjFSKext/public/PrjFSCommon.h @@ -44,4 +44,14 @@ enum PrjFSServiceUserClientType #define KEXT_STATIC_INLINE static inline #endif +namespace PrjFSDarwinMajorVersion +{ + enum + { + MacOS10_13_HighSierra = 17, + MacOS10_14_Mojave = 18, + MacOS10_15_Catalina = 19, + }; +} + #endif /* PrjFSCommon_h */ diff --git a/ProjFS.Mac/PrjFSLib/PrjFSUser.cpp b/ProjFS.Mac/PrjFSLib/PrjFSUser.cpp index a2ad096770..8e06de0bb3 100644 --- a/ProjFS.Mac/PrjFSLib/PrjFSUser.cpp +++ b/ProjFS.Mac/PrjFSLib/PrjFSUser.cpp @@ -335,8 +335,8 @@ static void InitDataQueueFunctions() return; } - if ((osVersion.major == 17 && osVersion.minor >= 7) // macOS 10.13.6+ - || (osVersion.major == 18 && osVersion.minor == 0)) // macOS 10.14(.0) exactly + if ((osVersion.major == PrjFSDarwinMajorVersion::MacOS10_13_HighSierra && osVersion.minor >= 7) // macOS 10.13.6+ + || (osVersion.major == PrjFSDarwinMajorVersion::MacOS10_14_Mojave && osVersion.minor == 0)) // macOS 10.14(.0) exactly { void* dataQueueLibrary = dlopen("libSharedDataQueue.dylib", RTLD_LAZY); if (nullptr == dataQueueLibrary) From 9b1688e8e04bd555d00f1a6dd4a11a76fa5a0923 Mon Sep 17 00:00:00 2001 From: Phil Dennis-Jordan Date: Fri, 7 Jun 2019 15:30:25 +0200 Subject: [PATCH 3/6] Mac ProjFS kext testing: Use modern mock vnode creation API Replaces a use of the old mock vnode creation function with the new tree and mountpoint-based one. --- ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm index 44ef09a24a..17ebfea729 100644 --- a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm +++ b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm @@ -856,7 +856,8 @@ - (void) testWriteDataToForkedPath { - (void) testIneligibleFilesystemType { shared_ptr testMountNone = mount::Create("msdos", fsid_t{}, 0); - shared_ptr testVnodeNone = vnode::Create(testMountNone, "/Volumes/USBSTICK"); + shared_ptr testVnodeNone = testMountNone->CreateVnodeTree("/Volumes/USBSTICK", VDIR); + XCTAssertEqual( KAUTH_RESULT_DEFER, HandleVnodeOperation( From 369dab5cea154ce896bd8a54cebdb5010d515e87 Mon Sep 17 00:00:00 2001 From: Phil Dennis-Jordan Date: Tue, 21 May 2019 16:38:07 +0200 Subject: [PATCH 4/6] Mac ProjFS: Only hydrate/expand when deleting due to rename This change utilises the KAUTH_FILEOP_WILL_RENAME event which was introduced with Mojave/10.14 to determine if a KAUTH_VNODE_DELETE authorisation check is due to a rename() operation. In this case, the file or directory genuinely needs hydrating, otherwise hydration can be skipped. This change also adds unit tests for the new rename operation tracking code, and adapts existing unit tests so hydration is not expected when a KAUTH_VNODE_DELETE check occurs outside of a rename() operation. Note that this change means the behaviour on macOS 10.13 and earlier versus 10.14 and later subtly diverges. --- ProjFS.Mac/PrjFS.xcodeproj/project.pbxproj | 4 + ProjFS.Mac/PrjFSKext/ArrayUtilities.hpp | 24 ++ ProjFS.Mac/PrjFSKext/KauthHandler.cpp | 221 ++++++++++- ProjFS.Mac/PrjFSKext/KauthHandlerPrivate.hpp | 8 + ProjFS.Mac/PrjFSKext/KauthHandlerTestable.hpp | 10 + ProjFS.Mac/PrjFSKext/Locks.cpp | 27 ++ ProjFS.Mac/PrjFSKext/Locks.hpp | 11 + ProjFS.Mac/PrjFSKext/Message_Kernel.cpp | 7 +- .../HandleFileOpOperationTests.mm | 21 + .../PrjFSKextTests/HandleOperationTests.mm | 367 +++++++++++++++++- .../PrjFSKextTests/KauthHandlerTests.mm | 53 +++ ProjFS.Mac/PrjFSKextTests/MockProc.cpp | 18 + ProjFS.Mac/PrjFSKextTests/MockProc.hpp | 9 + ProjFS.Mac/PrjFSKextTests/TestLocks.cpp | 22 ++ ProjFS.Mac/PrjFSKextTests/TestMemory.cpp | 15 +- ProjFS.Mac/Scripts/Build.sh | 3 +- 16 files changed, 782 insertions(+), 38 deletions(-) create mode 100644 ProjFS.Mac/PrjFSKext/ArrayUtilities.hpp create mode 100644 ProjFS.Mac/PrjFSKext/KauthHandlerPrivate.hpp diff --git a/ProjFS.Mac/PrjFS.xcodeproj/project.pbxproj b/ProjFS.Mac/PrjFS.xcodeproj/project.pbxproj index 1f67fc5a87..78492f96f8 100644 --- a/ProjFS.Mac/PrjFS.xcodeproj/project.pbxproj +++ b/ProjFS.Mac/PrjFS.xcodeproj/project.pbxproj @@ -283,6 +283,8 @@ 4A08257021E77BDD00E21AFD /* org.vfsforgit.prjfs.PrjFSKextLogDaemon.plist */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.plist.xml; path = org.vfsforgit.prjfs.PrjFSKextLogDaemon.plist; sourceTree = ""; }; 4A08257121E77BDD00E21AFD /* PrjFSKextLogDaemon.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PrjFSKextLogDaemon.cpp; sourceTree = ""; }; 4A08257221E77BDD00E21AFD /* Info.plist */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; + 4A2A699B2295AA7800ACAAAF /* ArrayUtilities.hpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = ArrayUtilities.hpp; sourceTree = ""; }; + 4A2A699D2296BACE00ACAAAF /* KauthHandlerPrivate.hpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = KauthHandlerPrivate.hpp; sourceTree = ""; }; 4A2A699F2297339A00ACAAAF /* KextAssertIntegration.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = KextAssertIntegration.m; sourceTree = ""; }; 4A2A69A12297375A00ACAAAF /* KextAssertIntegration.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = KextAssertIntegration.h; sourceTree = ""; }; 4A558DB822357AB000AFDE07 /* ProviderMessaging.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = ProviderMessaging.cpp; sourceTree = ""; }; @@ -439,9 +441,11 @@ 4391F88D21E42AA70008103C /* PrjFSKext */ = { isa = PBXGroup; children = ( + 4A2A699B2295AA7800ACAAAF /* ArrayUtilities.hpp */, 4391F88F21E42AC40008103C /* Info.plist */, 4391F89321E42AC40008103C /* KauthHandler.cpp */, 4391F8A421E42AC50008103C /* KauthHandler.hpp */, + 4A2A699D2296BACE00ACAAAF /* KauthHandlerPrivate.hpp */, F5E39C8121F11556006D65C2 /* KauthHandlerTestable.hpp */, 4391F89021E42AC40008103C /* kernel-header-wrappers */, 4391F89121E42AC40008103C /* KextLog.cpp */, diff --git a/ProjFS.Mac/PrjFSKext/ArrayUtilities.hpp b/ProjFS.Mac/PrjFSKext/ArrayUtilities.hpp new file mode 100644 index 0000000000..af873d4677 --- /dev/null +++ b/ProjFS.Mac/PrjFSKext/ArrayUtilities.hpp @@ -0,0 +1,24 @@ +#pragma once + +template + constexpr size_t Array_Size(T (&array)[size]) +{ + return size; +} + +template void Array_CopyElements(T* destination, const T* source, size_t count) +{ + for (size_t i = 0; i < count; ++i) + { + destination[i] = source[i]; + } +} + +template void Array_DefaultInit(T* array, size_t count) +{ + for (size_t i = 0; i < count; ++i) + { + array[i] = T(); + } +} + diff --git a/ProjFS.Mac/PrjFSKext/KauthHandler.cpp b/ProjFS.Mac/PrjFSKext/KauthHandler.cpp index ceb4bca736..78bf501f03 100644 --- a/ProjFS.Mac/PrjFSKext/KauthHandler.cpp +++ b/ProjFS.Mac/PrjFSKext/KauthHandler.cpp @@ -2,7 +2,10 @@ #include #include #include +#include +#include +#include "KauthHandlerPrivate.hpp" #include "public/PrjFSCommon.h" #include "public/PrjFSPerfCounter.h" #include "public/PrjFSXattrs.h" @@ -19,17 +22,30 @@ #include "ProviderMessaging.hpp" #include "VnodeCache.hpp" #include "Memory.hpp" +#include "ArrayUtilities.hpp" #ifdef KEXT_UNIT_TESTING #include "KauthHandlerTestable.hpp" #endif +template + auto clamp(const T& value, const MIN_T& min, const MAX_T& max) +{ + return value < min ? min : (value > max ? max : value); +} + enum ProviderCallbackPolicy { CallbackPolicy_AllowAny, CallbackPolicy_UserInitiatedOnly, }; +struct PendingRenameOperation +{ + vnode_t vnode; + thread_t thread; +}; + // Function prototypes KEXT_STATIC int HandleVnodeOperation( kauth_cred_t credential, @@ -101,11 +117,19 @@ KEXT_STATIC bool ShouldHandleFileOpEvent( VirtualizationRootHandle* root, int* pid); +KEXT_STATIC bool InitPendingRenames(); +KEXT_STATIC void CleanupPendingRenames(); +KEXT_STATIC void ResizePendingRenames(uint32_t newMaxPendingRenames); + // State static kauth_listener_t s_vnodeListener = nullptr; static kauth_listener_t s_fileopListener = nullptr; static atomic_int s_numActiveKauthEvents; +static SpinLock s_renameLock; +static PendingRenameOperation* s_pendingRenames = nullptr; +KEXT_STATIC uint32_t s_pendingRenameCount = 0; +KEXT_STATIC uint32_t s_maxPendingRenames = 0; // Public functions kern_return_t KauthHandler_Init() @@ -130,6 +154,11 @@ kern_return_t KauthHandler_Init() goto CleanupAndFail; } + if (!InitPendingRenames()) + { + goto CleanupAndFail; + } + s_vnodeListener = kauth_listen_scope(KAUTH_SCOPE_VNODE, HandleVnodeOperation, nullptr); if (nullptr == s_vnodeListener) { @@ -178,6 +207,8 @@ kern_return_t KauthHandler_Cleanup() WaitForListenerCompletion(); + CleanupPendingRenames(); + if (VnodeCache_Cleanup()) { result = KERN_FAILURE; @@ -216,6 +247,162 @@ KEXT_STATIC void UseMainForkIfNamedStream( } } +KEXT_STATIC bool InitPendingRenames() +{ + if (version_major >= PrjFSDarwinMajorVersion::MacOS10_14_Mojave) // Only need to track renames on Mojave and newer + { + s_renameLock = SpinLock_Alloc(); + s_maxPendingRenames = 8; // Arbitrary choice, should be maximum number of expected concurrent threads performing renames, but array will resize on demand + s_pendingRenameCount = 0; + s_pendingRenames = Memory_AllocArray(s_maxPendingRenames); + if (!SpinLock_IsValid(s_renameLock) || s_pendingRenames == nullptr) + { + return false; + } + + Array_DefaultInit(s_pendingRenames, s_maxPendingRenames); + } + + return true; +} + +KEXT_STATIC void CleanupPendingRenames() +{ + if (version_major >= PrjFSDarwinMajorVersion::MacOS10_14_Mojave) + { + if (SpinLock_IsValid(s_renameLock)) + { + SpinLock_FreeMemory(&s_renameLock); + } + + if (s_pendingRenames != nullptr) + { + assert(s_pendingRenameCount == 0); + Memory_FreeArray(s_pendingRenames, s_maxPendingRenames); + s_pendingRenames = nullptr; + s_maxPendingRenames = 0; + } + } +} + +KEXT_STATIC void ResizePendingRenames(uint32_t newMaxPendingRenames) +{ + PendingRenameOperation* newArray = Memory_AllocArray(newMaxPendingRenames); + assert(newArray != nullptr); + PendingRenameOperation* arrayToFree = nullptr; + uint32_t arrayToFreeLength = 0; + + SpinLock_Acquire(s_renameLock); + { + if (newMaxPendingRenames > s_maxPendingRenames) + { + Array_CopyElements(newArray, s_pendingRenames, s_maxPendingRenames); + Array_DefaultInit(newArray + s_maxPendingRenames, newMaxPendingRenames - s_maxPendingRenames); + + arrayToFree = s_pendingRenames; + arrayToFreeLength = s_maxPendingRenames; + + s_pendingRenames = newArray; + s_maxPendingRenames = newMaxPendingRenames; + } + else + { + arrayToFree = newArray; + arrayToFreeLength = newMaxPendingRenames; + } + } + SpinLock_Release(s_renameLock); + + if (arrayToFree != nullptr) + { + assert(arrayToFreeLength > 0); + Memory_FreeArray(arrayToFree, arrayToFreeLength); + } +} + +KEXT_STATIC void RecordPendingRenameOperation(vnode_t vnode) +{ + assertf(version_major >= PrjFSDarwinMajorVersion::MacOS10_14_Mojave, "This function should only be called from the KAUTH_FILEOP_WILL_RENAME handler, which is only supported by Darwin 18 (macOS 10.14 Mojave) and newer (version_major = %u)", version_major); + thread_t myThread = current_thread(); + + bool resizeTable; + do + { + resizeTable = false; + uint32_t resizeTableLength = 0; + + SpinLock_Acquire(s_renameLock); + { + if (s_pendingRenameCount < s_maxPendingRenames) + { + s_pendingRenames[s_pendingRenameCount].thread = myThread; + s_pendingRenames[s_pendingRenameCount].vnode = vnode; + ++s_pendingRenameCount; + } + else + { + for (uint32_t i = 0; i < s_pendingRenameCount; ++i) + { + assert(s_pendingRenames[i].thread != myThread); + } + + resizeTable = true; + + resizeTableLength = static_cast(clamp(s_maxPendingRenames * UINT64_C(2), 1u, UINT32_MAX)); + assert(resizeTableLength > s_maxPendingRenames); + } + } + SpinLock_Release(s_renameLock); + + if (resizeTable) + { + if (resizeTableLength > 16) + { + KextLog_Error("Warning: RecordPendingRenameOperation is causing pending rename array resize to %u items.", resizeTableLength); + } + ResizePendingRenames(resizeTableLength); + } + } while (resizeTable); +} + +KEXT_STATIC bool DeleteOpIsForRename(vnode_t vnode) +{ + if (version_major < PrjFSDarwinMajorVersion::MacOS10_14_Mojave) + { + // High Sierra and earlier do not support WILL_RENAME notification, so we have to assume any delete may be caused by a rename + assert(s_pendingRenameCount == 0); + assert(s_maxPendingRenames == 0); + return true; + } + + bool isRename = false; + + thread_t myThread = current_thread(); + + SpinLock_Acquire(s_renameLock); + { + for (uint32_t i = 0; i < s_pendingRenameCount; ++i) + { + if (s_pendingRenames[i].thread == myThread) + { + isRename = true; + assert(s_pendingRenames[i].vnode == vnode); + --s_pendingRenameCount; + if (i != s_pendingRenameCount) + { + s_pendingRenames[i] = s_pendingRenames[s_pendingRenameCount]; + } + + s_pendingRenames[s_pendingRenameCount] = PendingRenameOperation{}; + break; + } + } + } + SpinLock_Release(s_renameLock); + + return isRename; +} + // Private functions KEXT_STATIC int HandleVnodeOperation( kauth_cred_t credential, @@ -247,6 +434,7 @@ KEXT_STATIC int HandleVnodeOperation( char procname[MAXCOMLEN + 1] = ""; bool isDeleteAction = false; bool isDirectory = false; + bool isRename = false; { PerfSample considerVnodeSample(&perfTracer, PrjFSPerfCounter_VnodeOp_BasicVnodeChecks); @@ -256,6 +444,15 @@ KEXT_STATIC int HandleVnodeOperation( } } + isDeleteAction = ActionBitIsSet(action, KAUTH_VNODE_DELETE); + if (isDeleteAction) + { + // This removes a matching entry from the array, so must run under the same + // conditions as the original RecordPendingRenameOperation call - hence early + // in the callback. + isRename = DeleteOpIsForRename(currentVnode); + } + if (!ShouldHandleVnodeOpEvent( &perfTracer, context, @@ -270,22 +467,21 @@ KEXT_STATIC int HandleVnodeOperation( goto CleanupAndReturn; } - isDeleteAction = ActionBitIsSet(action, KAUTH_VNODE_DELETE); isDirectory = vnode_isdir(currentVnode); if (isDirectory) { - if (ActionBitIsSet( + if (isRename || + ActionBitIsSet( action, KAUTH_VNODE_LIST_DIRECTORY | KAUTH_VNODE_SEARCH | KAUTH_VNODE_READ_SECURITY | KAUTH_VNODE_READ_ATTRIBUTES | - KAUTH_VNODE_READ_EXTATTRIBUTES | - KAUTH_VNODE_DELETE)) + KAUTH_VNODE_READ_EXTATTRIBUTES)) { - // Recursively expand directory on delete to ensure child placeholders are created before rename operations - if (isDeleteAction) + // Recursively expand directory on rename as user will expect the moved directory to have the same contents as in its original location + if (isRename) { // Prevent system services from expanding directories as part of enumeration as this tends to cause deadlocks with the kauth listeners for Antivirus software if (!TryGetVirtualizationRoot(&perfTracer, context, currentVnode, pid, CallbackPolicy_UserInitiatedOnly, &root, &vnodeFsidInode, &kauthResult, kauthError)) @@ -339,7 +535,8 @@ KEXT_STATIC int HandleVnodeOperation( } else { - if (ActionBitIsSet( + if (isRename || // Hydrate before a file is moved as the user will not expect an empty file at the new location + ActionBitIsSet( action, KAUTH_VNODE_READ_ATTRIBUTES | KAUTH_VNODE_WRITE_ATTRIBUTES | @@ -348,7 +545,6 @@ KEXT_STATIC int HandleVnodeOperation( KAUTH_VNODE_READ_DATA | KAUTH_VNODE_WRITE_DATA | KAUTH_VNODE_EXECUTE | - KAUTH_VNODE_DELETE | // Hydrate on delete to ensure files are hydrated before rename operations KAUTH_VNODE_APPEND_DATA)) { if (FileFlagsBitIsSet(currentVnodeFileFlags, FileFlags_IsEmpty)) @@ -765,6 +961,15 @@ KEXT_STATIC int HandleFileOpOperation( goto CleanupAndReturn; } } + else if (KAUTH_FILEOP_WILL_RENAME == action) + { + currentVnode = reinterpret_cast(arg0); + if (VnodeIsEligibleForEventHandling(currentVnode)) + { + // Records thread/vnode as rename() operation in progress, enabling optimisation in subsequent DELETE vnode listener handler + RecordPendingRenameOperation(currentVnode); + } + } CleanupAndReturn: if (NULLVP != currentVnode && putCurrentVnode) diff --git a/ProjFS.Mac/PrjFSKext/KauthHandlerPrivate.hpp b/ProjFS.Mac/PrjFSKext/KauthHandlerPrivate.hpp new file mode 100644 index 0000000000..72a3ec158a --- /dev/null +++ b/ProjFS.Mac/PrjFSKext/KauthHandlerPrivate.hpp @@ -0,0 +1,8 @@ +#pragma once + +#include "kernel-header-wrappers/kauth.h" + +// Define missing symbol when building with 10.13 SDK/Xcode 9 +#ifndef KAUTH_FILEOP_WILL_RENAME +#define KAUTH_FILEOP_WILL_RENAME 8 +#endif diff --git a/ProjFS.Mac/PrjFSKext/KauthHandlerTestable.hpp b/ProjFS.Mac/PrjFSKext/KauthHandlerTestable.hpp index 5ca41ffa64..1cae4b6d94 100644 --- a/ProjFS.Mac/PrjFSKext/KauthHandlerTestable.hpp +++ b/ProjFS.Mac/PrjFSKext/KauthHandlerTestable.hpp @@ -5,6 +5,7 @@ #include "../PrjFSKext/PerformanceTracing.hpp" #include "../PrjFSKext/VirtualizationRoots.hpp" #include "KauthHandler.hpp" +#include "KauthHandlerPrivate.hpp" #ifndef __cplusplus #error None of the kext code is set up for being called from C or Objective-C; change the including file to C++ or Objective-C++ @@ -16,6 +17,9 @@ struct FsidInode; +extern uint32_t s_maxPendingRenames; +extern uint32_t s_pendingRenameCount; + KEXT_STATIC_INLINE bool FileFlagsBitIsSet(uint32_t fileFlags, uint32_t bit); KEXT_STATIC_INLINE bool ActionBitIsSet(kauth_action_t action, kauth_action_t mask); KEXT_STATIC_INLINE bool TryGetFileIsFlaggedAsInRoot(vnode_t vnode, vfs_context_t context, bool* flaggedInRoot); @@ -64,3 +68,9 @@ KEXT_STATIC bool ShouldHandleFileOpEvent( int* pid); KEXT_STATIC void UseMainForkIfNamedStream(vnode_t& vnode, bool& putVnodeWhenDone); KEXT_STATIC bool CurrentProcessWasSpawnedByRegularUser(); +KEXT_STATIC bool InitPendingRenames(); +KEXT_STATIC void CleanupPendingRenames(); +KEXT_STATIC void ResizePendingRenames(uint32_t newMaxPendingRenames); +KEXT_STATIC void RecordPendingRenameOperation(vnode_t vnode); +KEXT_STATIC bool DeleteOpIsForRename(vnode_t vnode); + diff --git a/ProjFS.Mac/PrjFSKext/Locks.cpp b/ProjFS.Mac/PrjFSKext/Locks.cpp index 4c15aeac09..c4662c810e 100644 --- a/ProjFS.Mac/PrjFSKext/Locks.cpp +++ b/ProjFS.Mac/PrjFSKext/Locks.cpp @@ -180,3 +180,30 @@ void RWLock_DropExclusiveToShared(RWLock& rwLock) #endif } + +SpinLock SpinLock_Alloc() +{ + return SpinLock { lck_spin_alloc_init(s_lockGroup, LCK_ATTR_NULL) }; +} + +void SpinLock_FreeMemory(SpinLock* lock) +{ + assert(lock->p != nullptr); + lck_spin_free(lock->p, s_lockGroup); + lock->p = nullptr; +} + +bool SpinLock_IsValid(SpinLock lock) +{ + return lock.p != nullptr; +} + +void SpinLock_Acquire(SpinLock lock) +{ + lck_spin_lock(lock.p); +} + +void SpinLock_Release(SpinLock lock) +{ + lck_spin_unlock(lock.p); +} diff --git a/ProjFS.Mac/PrjFSKext/Locks.hpp b/ProjFS.Mac/PrjFSKext/Locks.hpp index b4b7ca118d..11d3e63dec 100644 --- a/ProjFS.Mac/PrjFSKext/Locks.hpp +++ b/ProjFS.Mac/PrjFSKext/Locks.hpp @@ -49,6 +49,17 @@ void RWLock_ReleaseExclusive(RWLock& rwLock); void RWLock_DropExclusiveToShared(RWLock& rwLock); bool RWLock_AcquireSharedToExclusive(RWLock& rwLock); +typedef struct __lck_spin_t__ lck_spin_t; +struct SpinLock +{ + lck_spin_t* p; +}; + +SpinLock SpinLock_Alloc(); +void SpinLock_FreeMemory(SpinLock* lock); +bool SpinLock_IsValid(SpinLock lock); +void SpinLock_Acquire(SpinLock lock); +void SpinLock_Release(SpinLock lock); #endif /* Locks_h */ diff --git a/ProjFS.Mac/PrjFSKext/Message_Kernel.cpp b/ProjFS.Mac/PrjFSKext/Message_Kernel.cpp index 79f9e7fd6b..396802a881 100644 --- a/ProjFS.Mac/PrjFSKext/Message_Kernel.cpp +++ b/ProjFS.Mac/PrjFSKext/Message_Kernel.cpp @@ -1,13 +1,8 @@ #include "Message_Kernel.hpp" +#include "ArrayUtilities.hpp" #include #include -template - constexpr size_t Array_Size(T (&array)[size]) -{ - return size; -} - void Message_Init( Message* spec, MessageHeader* header, diff --git a/ProjFS.Mac/PrjFSKextTests/HandleFileOpOperationTests.mm b/ProjFS.Mac/PrjFSKextTests/HandleFileOpOperationTests.mm index bd25e27591..a4bd0105cd 100644 --- a/ProjFS.Mac/PrjFSKextTests/HandleFileOpOperationTests.mm +++ b/ProjFS.Mac/PrjFSKextTests/HandleFileOpOperationTests.mm @@ -21,6 +21,7 @@ using std::make_tuple; using std::shared_ptr; using std::vector; +using std::string; using KextMock::_; class PrjFSProviderUserClient @@ -457,4 +458,24 @@ - (void)testFileopHardlinkOtherRepoOtherProviderPID _)); } +- (void)testNoPendingRenameRecordedOnIneligibleFilesystem +{ + shared_ptr testMountNone = mount::Create("msdos", fsid_t{}, 0); + const string testMountPath = "/Volumes/USBSTICK"; + shared_ptr testMountRoot = testMountNone->CreateVnodeTree(testMountPath, VDIR); + const string filePath = testMountPath + "/file"; + shared_ptr testVnode = testMountNone->CreateVnodeTree(filePath); + const string renamedFilePath = filePath + "_renamed"; + + HandleFileOpOperation( + nullptr, // credential + nullptr, /* idata, unused */ + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(testVnode.get()), + reinterpret_cast(filePath.c_str()), + reinterpret_cast(renamedFilePath.c_str()), + 0); // unused + XCTAssertEqual(0, s_pendingRenameCount); +} + @end diff --git a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm index 17ebfea729..2c76ef098a 100644 --- a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm +++ b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm @@ -21,8 +21,12 @@ using std::make_tuple; using std::shared_ptr; using std::vector; +using std::string; using KextMock::_; +// Darwin version of running kernel +int version_major = PrjFSDarwinMajorVersion::MacOS10_14_Mojave; + class PrjFSProviderUserClient { }; @@ -35,15 +39,31 @@ static void SetPrjFSFileXattrData(const shared_ptr& vnode) vnode->xattrs.insert(make_pair(PrjFSFileXAttrName, rootXattrData)); } +static void TestForDarwinVersionRange(int versionMin, int versionMax, void(^testBlock)(void)) +{ + const int savedVersion = version_major; + for (int version = versionMin; version <= versionMax; ++version) + { + version_major = version; + testBlock(); + } + version_major = savedVersion; +} + +static void TestForAllSupportedDarwinVersions(void(^testBlock)(void)) +{ + TestForDarwinVersionRange(PrjFSDarwinMajorVersion::MacOS10_13_HighSierra, PrjFSDarwinMajorVersion::MacOS10_15_Catalina, testBlock); +} + @interface HandleVnodeOperationTests : PFSKextTestCase @end @implementation HandleVnodeOperationTests { vfs_context_t context; - const char* repoPath; - const char* filePath; - const char* dirPath; + string repoPath; + string filePath; + string dirPath; VirtualizationRootHandle dummyRepoHandle; PrjFSProviderUserClient dummyClient; pid_t dummyClientPid; @@ -75,7 +95,7 @@ - (void) setUp testDirVnode = testMount->CreateVnodeTree(dirPath, VDIR); // Register provider for the repository path (Simulate a mount) - VirtualizationRootResult result = VirtualizationRoot_RegisterProviderForPath(&dummyClient, dummyClientPid, repoPath); + VirtualizationRootResult result = VirtualizationRoot_RegisterProviderForPath(&dummyClient, dummyClientPid, repoPath.c_str()); XCTAssertEqual(result.error, 0); self->dummyRepoHandle = result.root; @@ -102,6 +122,7 @@ - (void) tearDown cacheWrapper.FreeCache(); VirtualizationRoots_Cleanup(); + CleanupPendingRenames(); vfs_context_rele(context); MockVnodes_CheckAndClear(); MockCalls::Clear(); @@ -125,8 +146,7 @@ - (void) testEmptyFileHydrates { testFileVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; SetPrjFSFileXattrData(testFileVnode); - const int actionCount = 9; - kauth_action_t actions[actionCount] = + kauth_action_t actions[] = { KAUTH_VNODE_READ_ATTRIBUTES, KAUTH_VNODE_WRITE_ATTRIBUTES, @@ -135,10 +155,10 @@ - (void) testEmptyFileHydrates { KAUTH_VNODE_READ_DATA, KAUTH_VNODE_WRITE_DATA, KAUTH_VNODE_EXECUTE, - KAUTH_VNODE_DELETE, KAUTH_VNODE_APPEND_DATA, }; - + const int actionCount = std::extent::value; + for (int i = 0; i < actionCount; i++) { XCTAssertTrue(HandleVnodeOperation( @@ -166,6 +186,53 @@ - (void) testEmptyFileHydrates { } } +- (void) testFileDeleteHydratesOnlyWhenNecessary +{ + testFileVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; + SetPrjFSFileXattrData(testFileVnode); + + TestForAllSupportedDarwinVersions(^{ + InitPendingRenames(); + XCTAssertEqual( + KAUTH_RESULT_DEFER, + HandleVnodeOperation( + nullptr, + nullptr, + KAUTH_VNODE_DELETE, + reinterpret_cast(self->context), + reinterpret_cast(self->testFileVnode.get()), + 0, + 0)); + bool didHydrate = + MockCalls::DidCallFunction( + ProviderMessaging_TrySendRequestAndWaitForResponse, + _, + MessageType_KtoU_HydrateFile, + self->testFileVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr); + + // On High Sierra, any delete causes hydration as it might be due to a rename: + if (version_major <= PrjFSDarwinMajorVersion::MacOS10_13_HighSierra) + { + XCTAssertTrue(didHydrate); + } + else + { + // On Mojave+, no hydration on non-rename delete: + XCTAssertFalse(didHydrate); + } + + MockCalls::Clear(); + CleanupPendingRenames(); + }); +} + - (void) testVnodeAccessCausesNoEvent { testFileVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; SetPrjFSFileXattrData(testFileVnode); @@ -325,8 +392,20 @@ - (void) testEventsThatShouldNotHydrate { } } -- (void) testDeleteFile { +- (void) testHydrationOnDeleteWhenRenamingFile { testFileVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; + + // Hydration on delete only occurs if deletion is caused by rename + string renamedFilePath = filePath + "_renamed"; + HandleFileOpOperation( + nullptr, // credential + nullptr, /* idata, unused */ + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(testFileVnode.get()), + reinterpret_cast(filePath.c_str()), + reinterpret_cast(renamedFilePath.c_str()), + 0); // unused + XCTAssertTrue(HandleVnodeOperation( nullptr, nullptr, @@ -364,8 +443,166 @@ - (void) testDeleteFile { XCTAssertTrue(MockCalls::CallCount(ProviderMessaging_TrySendRequestAndWaitForResponse) == 2); } -- (void) testDeleteDir { +- (void) testDeleteFileNonRenamed +{ + testFileVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; + + XCTAssertTrue(HandleVnodeOperation( + nullptr, + nullptr, + KAUTH_VNODE_DELETE, + reinterpret_cast(context), + reinterpret_cast(testFileVnode.get()), + 0, + 0) == KAUTH_RESULT_DEFER); + XCTAssertTrue(MockCalls::DidCallFunction( + ProviderMessaging_TrySendRequestAndWaitForResponse, + _, + MessageType_KtoU_NotifyFilePreDelete, + testFileVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr)); + + // Should not hydrate if delete is not caused by rename + XCTAssertFalse(MockCalls::DidCallFunction( + ProviderMessaging_TrySendRequestAndWaitForResponse, + _, + MessageType_KtoU_HydrateFile, + _, + _, + _, + _, + _, + _, + _, + nullptr)); +} + +- (void) testConcurrentRenameOperationRecording +{ + self->testFileVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; + + string otherFilePath = repoPath + "/otherFile"; + shared_ptr otherTestFileVnode = testMount->CreateVnodeTree(otherFilePath); + otherTestFileVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; + + string renamedFilePath = filePath + "_renamed"; + string renamedOtherFilePath = otherFilePath + "_renamed"; + + MockProcess_SetCurrentThreadIndex(0); + HandleFileOpOperation( + nullptr, // credential + nullptr, /* idata, unused */ + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(testFileVnode.get()), + reinterpret_cast(filePath.c_str()), + reinterpret_cast(renamedFilePath.c_str()), + 0); // unused + + MockProcess_SetCurrentThreadIndex(1); + HandleFileOpOperation( + nullptr, // credential + nullptr, /* idata, unused */ + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(otherTestFileVnode.get()), + reinterpret_cast(otherFilePath.c_str()), + reinterpret_cast(renamedOtherFilePath.c_str()), + 0); // unused + + MockProcess_SetCurrentThreadIndex(0); + XCTAssertTrue(HandleVnodeOperation( + nullptr, + nullptr, + KAUTH_VNODE_DELETE, + reinterpret_cast(context), + reinterpret_cast(testFileVnode.get()), + 0, + 0) == KAUTH_RESULT_DEFER); + + MockProcess_SetCurrentThreadIndex(1); + XCTAssertTrue(HandleVnodeOperation( + nullptr, + nullptr, + KAUTH_VNODE_DELETE, + reinterpret_cast(context), + reinterpret_cast(otherTestFileVnode.get()), + 0, + 0) == KAUTH_RESULT_DEFER); + + XCTAssertTrue(MockCalls::DidCallFunctionsInOrder( + ProviderMessaging_TrySendRequestAndWaitForResponse, + make_tuple( + _, + MessageType_KtoU_HydrateFile, + testFileVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr), + ProviderMessaging_TrySendRequestAndWaitForResponse, + make_tuple( + _, + MessageType_KtoU_NotifyFilePreDelete, + testFileVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr), + ProviderMessaging_TrySendRequestAndWaitForResponse, + make_tuple( + _, + MessageType_KtoU_HydrateFile, + otherTestFileVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr), + ProviderMessaging_TrySendRequestAndWaitForResponse, + make_tuple( + _, + MessageType_KtoU_NotifyFilePreDelete, + otherTestFileVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr)) + ); + XCTAssertTrue(MockCalls::CallCount(ProviderMessaging_TrySendRequestAndWaitForResponse) == 4); +} + + +- (void) testHydrationOnDeleteWhenRenamingDirectory +{ testDirVnode->attrValues.va_flags = FileFlags_IsInVirtualizationRoot; + + // Hydration on delete only occurs if deletion is caused by rename + string renamedDirPath = self->dirPath + "_renamed"; + HandleFileOpOperation( + nullptr, // credential + nullptr, /* idata, unused */ + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(self->testDirVnode.get()), + reinterpret_cast(self->dirPath.c_str()), + reinterpret_cast(renamedDirPath.c_str()), + 0); // unused + XCTAssertTrue(HandleVnodeOperation( nullptr, nullptr, @@ -403,6 +640,58 @@ - (void) testDeleteDir { XCTAssertTrue(MockCalls::CallCount(ProviderMessaging_TrySendRequestAndWaitForResponse) == 2); } +- (void) testDeleteDirNonRenamed +{ + testDirVnode->attrValues.va_flags = FileFlags_IsInVirtualizationRoot; + + TestForAllSupportedDarwinVersions(^{ + XCTAssertTrue(HandleVnodeOperation( + nullptr, + nullptr, + KAUTH_VNODE_DELETE, + reinterpret_cast(self->context), + reinterpret_cast(self->testDirVnode.get()), + 0, + 0) == KAUTH_RESULT_DEFER); + XCTAssertTrue(MockCalls::DidCallFunction( + ProviderMessaging_TrySendRequestAndWaitForResponse, + _, + MessageType_KtoU_NotifyDirectoryPreDelete, + self->testDirVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr)); + + // Should not enumerate if delete is not caused by rename, except on High Sierra + bool didEnumerate = MockCalls::DidCallFunction( + ProviderMessaging_TrySendRequestAndWaitForResponse, + _, + MessageType_KtoU_RecursivelyEnumerateDirectory, + self->testDirVnode.get(), + _, + _, + _, + _, + _, + _, + nullptr); + if (version_major <= PrjFSDarwinMajorVersion::MacOS10_13_HighSierra) + { + XCTAssertTrue(didEnumerate); + } + else + { + XCTAssertFalse(didEnumerate); + } + + MockCalls::Clear(); + }); +} + - (void) testEmptyDirectoryEnumerates { testDirVnode->attrValues.va_flags = FileFlags_IsEmpty | FileFlags_IsInVirtualizationRoot; const int actionCount = 5; @@ -694,9 +983,20 @@ - (void) testDeleteDirectoryWithDisappearingVirtualizationRoot { } // When the first call to getVirtualizationRoot fails, ensure no more calls are made -- (void) testDeleteDirectoryWhenFirstRequestFails { +- (void) testRenameDirectoryWhenFirstRequestFails { ProviderMessageMock_SetDefaultRequestResult(false); testDirVnode->attrValues.va_flags = FileFlags_IsInVirtualizationRoot; + + string renamedDirPath = self->dirPath + "_renamed"; + HandleFileOpOperation( + nullptr, // credential + nullptr, /* idata, unused */ + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(self->testDirVnode.get()), + reinterpret_cast(self->dirPath.c_str()), + reinterpret_cast(renamedDirPath.c_str()), + 0); // unused + XCTAssertTrue(HandleVnodeOperation( nullptr, nullptr, @@ -709,8 +1009,20 @@ - (void) testDeleteDirectoryWhenFirstRequestFails { XCTAssertTrue(MockCalls::CallCount(ProviderMessaging_TrySendRequestAndWaitForResponse) == 1); } -- (void) testDeleteDirectoryWhenSecondRequestFails { +- (void) testDeleteDirectoryForRenameWhenSecondRequestFails { ProviderMessageMock_SetSecondRequestResult(false); + + // Hydration on delete only occurs if deletion is caused by rename + string renamedDirPath = self->dirPath + "_renamed"; + HandleFileOpOperation( + nullptr, // credential + nullptr, /* idata, unused */ + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(self->testDirVnode.get()), + reinterpret_cast(self->dirPath.c_str()), + reinterpret_cast(renamedDirPath.c_str()), + 0); // unused + testDirVnode->attrValues.va_flags = FileFlags_IsInVirtualizationRoot; XCTAssertTrue(HandleVnodeOperation( nullptr, @@ -724,17 +1036,30 @@ - (void) testDeleteDirectoryWhenSecondRequestFails { XCTAssertTrue(MockCalls::CallCount(ProviderMessaging_TrySendRequestAndWaitForResponse) == 2); } -- (void) testDeleteDirectoryWithNoVirtualizationRoot { +- (void) testRenameDirectoryWithNoVirtualizationRoot { [self removeAllVirtualizationRoots]; testDirVnode->attrValues.va_flags = FileFlags_IsInVirtualizationRoot; - XCTAssertTrue(HandleVnodeOperation( - nullptr, - nullptr, - KAUTH_VNODE_DELETE, - reinterpret_cast(context), - reinterpret_cast(testDirVnode.get()), - 0, - 0) == KAUTH_RESULT_DEFER); + + string renamedDirPath = self->dirPath + "_renamed"; + HandleFileOpOperation( + nullptr, // credential + nullptr, // idata, unused + KAUTH_FILEOP_WILL_RENAME, + reinterpret_cast(self->testDirVnode.get()), + reinterpret_cast(self->dirPath.c_str()), + reinterpret_cast(renamedDirPath.c_str()), + 0); // unused + + XCTAssertEqual( + KAUTH_RESULT_DEFER, + HandleVnodeOperation( + nullptr, + nullptr, + KAUTH_VNODE_DELETE, + reinterpret_cast(context), + reinterpret_cast(testDirVnode.get()), + 0, + 0)); XCTAssertFalse(MockCalls::DidCallFunction(ProviderMessaging_TrySendRequestAndWaitForResponse)); } diff --git a/ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm b/ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm index 2f744825be..edcd996dc5 100644 --- a/ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm +++ b/ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm @@ -11,6 +11,7 @@ #include "VnodeCacheEntriesWrapper.hpp" using std::shared_ptr; +using std::string; @interface KauthHandlerTests : PFSKextTestCase @end @@ -37,6 +38,7 @@ - (void) tearDown { self->cacheWrapper.FreeCache(); MockVnodes_CheckAndClear(); VirtualizationRoots_Cleanup(); + MockCalls::Clear(); [super tearDown]; } @@ -344,4 +346,55 @@ - (void)testUseMainForkIfNamedStream { vnode_put(testVnode); } +- (void)testResizePendingRenamesRace +{ + InitPendingRenames(); + XCTAssertLessThan(s_maxPendingRenames, 16); + ResizePendingRenames(16); + ResizePendingRenames(16); + XCTAssertEqual(s_maxPendingRenames, 16); + CleanupPendingRenames(); +} + +- (void)testResizePendingRenamesLogsLargeArrayError +{ + shared_ptr testMount = mount::Create(); + string filePath = "/Users/test/code/Repo/file"; + shared_ptr testFile = testMount->CreateVnodeTree(filePath); + + InitPendingRenames(); + ResizePendingRenames(16); + s_pendingRenameCount = 16; // pretend we're full + XCTAssertFalse(MockCalls::DidCallFunction(KextMessageLogged, KEXTLOG_ERROR)); + RecordPendingRenameOperation(testFile.get()); + XCTAssertTrue(MockCalls::DidCallFunction(KextMessageLogged, KEXTLOG_ERROR)); + XCTAssertEqual(s_pendingRenameCount, 17); + XCTAssertGreaterThan(s_maxPendingRenames, 16); + XCTAssertTrue(DeleteOpIsForRename(testFile.get())); + XCTAssertEqual(s_pendingRenameCount, 16); + s_pendingRenameCount = 0; + CleanupPendingRenames(); +} + +- (void)testPendingRenamesOutOfOrderInsertAndRemoval +{ + shared_ptr testMount = mount::Create(); + string file1Path = "/Users/test/code/Repo/file1"; + string file2Path = "/Users/test/code/Repo/file2"; + shared_ptr testFile1 = testMount->CreateVnodeTree(file1Path); + shared_ptr testFile2 = testMount->CreateVnodeTree(file2Path); + + InitPendingRenames(); + RecordPendingRenameOperation(testFile1.get()); + MockProcess_SetCurrentThreadIndex(1); + RecordPendingRenameOperation(testFile2.get()); + MockProcess_SetCurrentThreadIndex(0); + XCTAssertTrue(DeleteOpIsForRename(testFile1.get())); + MockProcess_SetCurrentThreadIndex(1); + XCTAssertTrue(DeleteOpIsForRename(testFile2.get())); + MockProcess_SetCurrentThreadIndex(0); + XCTAssertEqual(s_pendingRenameCount, 0); + CleanupPendingRenames(); +} + @end diff --git a/ProjFS.Mac/PrjFSKextTests/MockProc.cpp b/ProjFS.Mac/PrjFSKextTests/MockProc.cpp index 2252cc3d35..7d96fbea0c 100644 --- a/ProjFS.Mac/PrjFSKextTests/MockProc.cpp +++ b/ProjFS.Mac/PrjFSKextTests/MockProc.cpp @@ -2,6 +2,10 @@ #include #include +struct thread +{ +}; + using std::make_pair; using std::map; using std::string; @@ -10,12 +14,15 @@ static map s_credentialMap; static map s_contextMap; static map s_processMap; static int s_selfPid; +static uint16_t s_currentThreadIndex = 0; +static thread s_threadPool[MockProcess_ThreadPoolSize] = {}; void MockProcess_Reset() { s_processMap.clear(); s_credentialMap.clear(); s_contextMap.clear(); + MockProcess_SetCurrentThreadIndex(0); } void MockProcess_SetSelfPid(int selfPid) @@ -157,3 +164,14 @@ void MockProcess_AddProcess(int pid, uintptr_t credentialId, int ppid, string na proc process = proc {pid, credentialId, ppid, name}; s_processMap.insert(make_pair(pid, process)); } + +kernel_thread_t current_thread() +{ + return &s_threadPool[s_currentThreadIndex]; +} + +void MockProcess_SetCurrentThreadIndex(uint16_t threadIndex) +{ + assert(threadIndex < MockProcess_ThreadPoolSize); + s_currentThreadIndex = threadIndex; +} diff --git a/ProjFS.Mac/PrjFSKextTests/MockProc.hpp b/ProjFS.Mac/PrjFSKextTests/MockProc.hpp index 8bb645a06a..a015e116bb 100644 --- a/ProjFS.Mac/PrjFSKextTests/MockProc.hpp +++ b/ProjFS.Mac/PrjFSKextTests/MockProc.hpp @@ -1,6 +1,10 @@ #include "../PrjFSKext/kernel-header-wrappers/vnode.h" #include +// In the kernel/kext this type is "thread_t" but there already exists a +// conflicting Mach type in user space hence the name change. +typedef struct thread* kernel_thread_t; + // Kernel functions that are being mocked extern "C" { @@ -16,6 +20,7 @@ extern "C" proc_t proc_find(int pid); int proc_rele(proc_t p); int proc_selfpid(void); + kernel_thread_t current_thread(void); } struct proc { @@ -30,3 +35,7 @@ void MockProcess_AddCredential(uintptr_t credentialId, uid_t UID); void MockProcess_AddContext(vfs_context_t context, int pid); void MockProcess_AddProcess(int pid, uintptr_t credentialId, int ppid, std::string procName); void MockProcess_Reset(); + +// Tests will only need to simulate a small number of threads, so we provide an existing, indexed pool. +constexpr uint16_t MockProcess_ThreadPoolSize = 2; +void MockProcess_SetCurrentThreadIndex(uint16_t threadIndex); diff --git a/ProjFS.Mac/PrjFSKextTests/TestLocks.cpp b/ProjFS.Mac/PrjFSKextTests/TestLocks.cpp index f2b6a44c65..d560cd4fa5 100644 --- a/ProjFS.Mac/PrjFSKextTests/TestLocks.cpp +++ b/ProjFS.Mac/PrjFSKextTests/TestLocks.cpp @@ -39,3 +39,25 @@ void RWLock_FreeMemory(RWLock* rwLock) delete rwLock->p; rwLock->p = nullptr; } + +void SpinLock_Acquire(SpinLock lock) +{ +} + +void SpinLock_Release(SpinLock lock) +{ +} + +SpinLock SpinLock_Alloc() +{ + return SpinLock{}; +} + +void SpinLock_FreeMemory(SpinLock* lock) +{ +} + +bool SpinLock_IsValid(SpinLock lock) +{ + return true; +} diff --git a/ProjFS.Mac/PrjFSKextTests/TestMemory.cpp b/ProjFS.Mac/PrjFSKextTests/TestMemory.cpp index ec57877acc..23bc5dbf90 100644 --- a/ProjFS.Mac/PrjFSKextTests/TestMemory.cpp +++ b/ProjFS.Mac/PrjFSKextTests/TestMemory.cpp @@ -1,11 +1,22 @@ #include "../PrjFSKext/Memory.hpp" +#include + +struct TrackedKextMemoryAllocation +{ + uint32_t size; + uint8_t data[]; +}; void Memory_Free(void* memory, uint32_t sizeBytes) { - free(memory); + TrackedKextMemoryAllocation* memoryObject = reinterpret_cast(static_cast(memory) - offsetof(TrackedKextMemoryAllocation, data)); + assert(memoryObject->size == sizeBytes); + free(memoryObject); } void* Memory_Alloc(uint32_t sizeBytes) { - return malloc(sizeBytes); + TrackedKextMemoryAllocation* memoryObject = static_cast(malloc(sizeBytes + sizeof(TrackedKextMemoryAllocation))); + memoryObject->size = sizeBytes; + return memoryObject->data; } diff --git a/ProjFS.Mac/Scripts/Build.sh b/ProjFS.Mac/Scripts/Build.sh index 46a3a1a300..a85697e82a 100755 --- a/ProjFS.Mac/Scripts/Build.sh +++ b/ProjFS.Mac/Scripts/Build.sh @@ -59,7 +59,8 @@ while read line; do # PrjFSKext exclusions [[ $line != *"AllArrayElementsInitialized"* ]] && #Function is used for compile time checks only [[ $line != *"KauthHandler_Init"* ]] && - [[ $line != *"KauthHandler_Cleanup"* ]] && + [[ $line != *"KauthHandler_Cleanup"* ]] && + [[ $line != *"InitPendingRenames"* ]] && [[ $line != *"HandleFileOpOperation"* ]] && #SHOULD ADD COVERAGE [[ $line != *"TryGetVirtualizationRoot"* ]] && #SHOULD ADD COVERAGE [[ $line != *"WaitForListenerCompletion"* ]] && From 172c70fc45017a2108af40aaae662c995f7f1cea Mon Sep 17 00:00:00 2001 From: Jessica Schumaker Date: Mon, 10 Jun 2019 09:59:03 -0400 Subject: [PATCH 5/6] Add PreDeleteFromRename Message Type --- ProjFS.Mac/PrjFSKext/KauthHandler.cpp | 2 +- ProjFS.Mac/PrjFSKext/ProviderMessaging.cpp | 1 + ProjFS.Mac/PrjFSKext/public/Message.h | 1 + ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm | 6 +++--- ProjFS.Mac/PrjFSLib.Mac.Managed/NotificationType.cs | 1 + .../PrjFSLib.Mac.Managed/VirtualizationInstance.cs | 1 + ProjFS.Mac/PrjFSLib/PrjFSLib.cpp | 13 +++++++++++-- ProjFS.Mac/PrjFSLib/PrjFSLib.h | 1 + 8 files changed, 20 insertions(+), 6 deletions(-) diff --git a/ProjFS.Mac/PrjFSKext/KauthHandler.cpp b/ProjFS.Mac/PrjFSKext/KauthHandler.cpp index 78bf501f03..43ff4e149e 100644 --- a/ProjFS.Mac/PrjFSKext/KauthHandler.cpp +++ b/ProjFS.Mac/PrjFSKext/KauthHandler.cpp @@ -623,7 +623,7 @@ KEXT_STATIC int HandleVnodeOperation( root, isDirectory ? MessageType_KtoU_NotifyDirectoryPreDelete : - MessageType_KtoU_NotifyFilePreDelete, + isRename ? MessageType_KtoU_NotifyFilePreDeleteFromRename : MessageType_KtoU_NotifyFilePreDelete, currentVnode, vnodeFsidInode, nullptr, // path not needed, use fsid/inode diff --git a/ProjFS.Mac/PrjFSKext/ProviderMessaging.cpp b/ProjFS.Mac/PrjFSKext/ProviderMessaging.cpp index cd3605e4af..da037761b8 100644 --- a/ProjFS.Mac/PrjFSKext/ProviderMessaging.cpp +++ b/ProjFS.Mac/PrjFSKext/ProviderMessaging.cpp @@ -92,6 +92,7 @@ void ProviderMessaging_HandleKernelMessageResponse(VirtualizationRootHandle prov case MessageType_KtoU_HydrateFile: case MessageType_KtoU_NotifyFileModified: case MessageType_KtoU_NotifyFilePreDelete: + case MessageType_KtoU_NotifyFilePreDeleteFromRename: case MessageType_KtoU_NotifyDirectoryPreDelete: case MessageType_KtoU_NotifyFileCreated: case MessageType_KtoU_NotifyFileRenamed: diff --git a/ProjFS.Mac/PrjFSKext/public/Message.h b/ProjFS.Mac/PrjFSKext/public/Message.h index 1083ad6d6a..2af36931ef 100644 --- a/ProjFS.Mac/PrjFSKext/public/Message.h +++ b/ProjFS.Mac/PrjFSKext/public/Message.h @@ -16,6 +16,7 @@ typedef enum MessageType_KtoU_NotifyFileModified, MessageType_KtoU_NotifyFilePreDelete, + MessageType_KtoU_NotifyFilePreDeleteFromRename, MessageType_KtoU_NotifyDirectoryPreDelete, MessageType_KtoU_NotifyFileCreated, MessageType_KtoU_NotifyFileRenamed, diff --git a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm index 2c76ef098a..d94128af9d 100644 --- a/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm +++ b/ProjFS.Mac/PrjFSKextTests/HandleOperationTests.mm @@ -430,7 +430,7 @@ - (void) testHydrationOnDeleteWhenRenamingFile { ProviderMessaging_TrySendRequestAndWaitForResponse, make_tuple( _, - MessageType_KtoU_NotifyFilePreDelete, + MessageType_KtoU_NotifyFilePreDeleteFromRename, testFileVnode.get(), _, _, @@ -550,7 +550,7 @@ - (void) testConcurrentRenameOperationRecording ProviderMessaging_TrySendRequestAndWaitForResponse, make_tuple( _, - MessageType_KtoU_NotifyFilePreDelete, + MessageType_KtoU_NotifyFilePreDeleteFromRename, testFileVnode.get(), _, _, @@ -574,7 +574,7 @@ - (void) testConcurrentRenameOperationRecording ProviderMessaging_TrySendRequestAndWaitForResponse, make_tuple( _, - MessageType_KtoU_NotifyFilePreDelete, + MessageType_KtoU_NotifyFilePreDeleteFromRename, otherTestFileVnode.get(), _, _, diff --git a/ProjFS.Mac/PrjFSLib.Mac.Managed/NotificationType.cs b/ProjFS.Mac/PrjFSLib.Mac.Managed/NotificationType.cs index 08b780d728..cd3a1a5e4a 100644 --- a/ProjFS.Mac/PrjFSLib.Mac.Managed/NotificationType.cs +++ b/ProjFS.Mac/PrjFSLib.Mac.Managed/NotificationType.cs @@ -10,6 +10,7 @@ public enum NotificationType None = 0x00000001, NewFileCreated = 0x00000004, PreDelete = 0x00000010, + PreDeleteFromRename = 0x00000011, FileRenamed = 0x00000080, HardLinkCreated = 0x00000100, PreConvertToFull = 0x00001000, diff --git a/ProjFS.Mac/PrjFSLib.Mac.Managed/VirtualizationInstance.cs b/ProjFS.Mac/PrjFSLib.Mac.Managed/VirtualizationInstance.cs index 8dc5b1aaa6..eb78b7ce75 100644 --- a/ProjFS.Mac/PrjFSLib.Mac.Managed/VirtualizationInstance.cs +++ b/ProjFS.Mac/PrjFSLib.Mac.Managed/VirtualizationInstance.cs @@ -191,6 +191,7 @@ public virtual Result StopVirtualizationInstance() switch (notificationType) { case NotificationType.PreDelete: + case NotificationType.PreDeleteFromRename: return this.OnPreDelete(relativePath, isDirectory); case NotificationType.FileModified: diff --git a/ProjFS.Mac/PrjFSLib/PrjFSLib.cpp b/ProjFS.Mac/PrjFSLib/PrjFSLib.cpp index c82fce7c56..786d706653 100644 --- a/ProjFS.Mac/PrjFSLib/PrjFSLib.cpp +++ b/ProjFS.Mac/PrjFSLib/PrjFSLib.cpp @@ -753,6 +753,7 @@ static void HandleKernelRequest(void* messageMemory, uint32_t messageSize) case MessageType_KtoU_NotifyFileModified: case MessageType_KtoU_NotifyFilePreDelete: + case MessageType_KtoU_NotifyFilePreDeleteFromRename: case MessageType_KtoU_NotifyDirectoryPreDelete: case MessageType_KtoU_NotifyFilePreConvertToFull: { @@ -1122,10 +1123,13 @@ static PrjFS_Result HandleFileNotification( notificationType, nullptr /* destinationRelativePath */); - // We have to convert to full for PreDelete until we have WILL_RENAME support. The PreDelete could be for a rename. + // Remove placeholder xattrs for renames (PreDeleteFromRename) because: + // - Renames are treated as "Delete old path" + "create new path", and new files should not be marked as placeholders + // - The target of the rename might be outside of the root, and we should not allow placeholder files to + // live outside of the virtualization root (as the kext won't know what provider they belong to) if (result == PrjFS_Result_Success && placeholderFile && - (PrjFS_NotificationType_PreConvertToFull == notificationType || PrjFS_NotificationType_PreDelete == notificationType)) + (PrjFS_NotificationType_PreConvertToFull == notificationType || PrjFS_NotificationType_PreDeleteFromRename == notificationType)) { errno_t result = RemoveXAttrWithoutFollowingLinks(absolutePath, PrjFSFileXAttrName); if (0 != result) @@ -1317,6 +1321,9 @@ static inline PrjFS_NotificationType KUMessageTypeToNotificationType(MessageType case MessageType_KtoU_NotifyDirectoryPreDelete: return PrjFS_NotificationType_PreDelete; + case MessageType_KtoU_NotifyFilePreDeleteFromRename: + return PrjFS_NotificationType_PreDeleteFromRename; + case MessageType_KtoU_NotifyFilePreConvertToFull: return PrjFS_NotificationType_PreConvertToFull; @@ -1445,6 +1452,8 @@ static const char* NotificationTypeToString(PrjFS_NotificationType notificationT return STRINGIFY(PrjFS_NotificationType_PreDelete); case PrjFS_NotificationType_FileRenamed: return STRINGIFY(PrjFS_NotificationType_FileRenamed); + case PrjFS_NotificationType_PreDeleteFromRename: + return STRINGIFY(PrjFS_NotificationType_PreDeleteFromRename); case PrjFS_NotificationType_HardLinkCreated: return STRINGIFY(PrjFS_NotificationType_HardLinkCreated); case PrjFS_NotificationType_PreConvertToFull: diff --git a/ProjFS.Mac/PrjFSLib/PrjFSLib.h b/ProjFS.Mac/PrjFSLib/PrjFSLib.h index f6173b9782..21a312c2c7 100644 --- a/ProjFS.Mac/PrjFSLib/PrjFSLib.h +++ b/ProjFS.Mac/PrjFSLib/PrjFSLib.h @@ -47,6 +47,7 @@ typedef enum PrjFS_NotificationType_None = 0x00000001, PrjFS_NotificationType_NewFileCreated = 0x00000004, PrjFS_NotificationType_PreDelete = 0x00000010, + PrjFS_NotificationType_PreDeleteFromRename = 0x00000011, PrjFS_NotificationType_FileRenamed = 0x00000080, PrjFS_NotificationType_HardLinkCreated = 0x00000100, PrjFS_NotificationType_PreConvertToFull = 0x00001000, From 63d18aa2ba415f4adcd0f2eeec477b03edce7ee0 Mon Sep 17 00:00:00 2001 From: Jessica Schumaker Date: Wed, 12 Jun 2019 12:54:08 -0400 Subject: [PATCH 6/6] Update FileMoved Test to use an unhydrated file --- .../EnlistmentPerFixture/GitFilesTests.cs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitFilesTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitFilesTests.cs index 7020b61c42..bb72cab8e1 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitFilesTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitFilesTests.cs @@ -376,19 +376,23 @@ public void FileMovedFromOutsideRepoToInside() [TestCase, Order(17)] public void FileMovedFromInsideRepoToOutside() { - string fileName = "InsideRepoToOutside.txt"; - string fileInsideRepo = this.Enlistment.GetVirtualPathTo(fileName); - this.fileSystem.WriteAllText(fileInsideRepo, "Contents for the new file"); - fileInsideRepo.ShouldBeAFile(this.fileSystem); - this.Enlistment.WaitForBackgroundOperations(); - GVFSHelpers.ModifiedPathsShouldContain(this.Enlistment, this.fileSystem, fileName); + string fileInsideRepoEntry = "GitCommandsTests/RenameFileTests/1/#test"; + string fileNameFullPath = Path.Combine("GitCommandsTests", "RenameFileTests", "1", "#test"); + string fileInsideRepo = this.Enlistment.GetVirtualPathTo(fileNameFullPath); + GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.fileSystem, fileInsideRepoEntry); + + string fileNameOutsideRepo = "FileNameOutSideRepo"; + string fileMovedOutsideRepo = Path.Combine(this.Enlistment.EnlistmentRoot, fileNameOutsideRepo); + GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.fileSystem, fileNameOutsideRepo); - string fileMovedOutsideRepo = Path.Combine(this.Enlistment.EnlistmentRoot, fileName); this.fileSystem.MoveFile(fileInsideRepo, fileMovedOutsideRepo); + fileInsideRepo.ShouldNotExistOnDisk(this.fileSystem); fileMovedOutsideRepo.ShouldBeAFile(this.fileSystem); + this.fileSystem.ReadAllText(fileMovedOutsideRepo).ShouldContain("test"); this.Enlistment.WaitForBackgroundOperations(); - GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.fileSystem, fileName); + GVFSHelpers.ModifiedPathsShouldContain(this.Enlistment, this.fileSystem, fileInsideRepoEntry); + GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.fileSystem, fileNameOutsideRepo); } [TestCase, Order(18)]