From 3dd4ca73494a09006f83d361d5d5b9c6d1b22665 Mon Sep 17 00:00:00 2001 From: Anthony Date: Thu, 25 Feb 2021 21:08:15 +1300 Subject: [PATCH 1/5] Implement IsEmuHandle by keeping a lookup of EmuHandle objects Fixes a crash in Amped on loading a map after having exited one Amped requests an invalid handle 0xFDFDFDFD IsEmuHandle return a false positive and we'd crash trying to call NtClose() --- src/core/kernel/exports/EmuKrnlNt.cpp | 6 +-- src/core/kernel/support/EmuFile.cpp | 60 +++++++++++++++++++-------- src/core/kernel/support/EmuFile.h | 16 ++++++- 3 files changed, 60 insertions(+), 22 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnlNt.cpp b/src/core/kernel/exports/EmuKrnlNt.cpp index 9a04a8995..6b70378ec 100644 --- a/src/core/kernel/exports/EmuKrnlNt.cpp +++ b/src/core/kernel/exports/EmuKrnlNt.cpp @@ -142,7 +142,7 @@ XBSYSAPI EXPORTNUM(187) xbox::ntstatus_xt NTAPI xbox::NtClose NTSTATUS ret = xbox::status_success; - if (IsEmuHandle(Handle)) + if (EmuHandle::IsEmuHandle(Handle)) { // delete 'special' handles EmuHandle *iEmuHandle = HandleToEmuHandle(Handle); @@ -682,7 +682,7 @@ XBSYSAPI EXPORTNUM(197) xbox::ntstatus_xt NTAPI xbox::NtDuplicateObject NTSTATUS ret = xbox::status_success; - if (IsEmuHandle(SourceHandle)) { + if (EmuHandle::IsEmuHandle(SourceHandle)) { EmuHandle* iEmuHandle = HandleToEmuHandle(SourceHandle); ret = iEmuHandle->NtDuplicateObject(TargetHandle, Options); /* @@ -739,7 +739,7 @@ XBSYSAPI EXPORTNUM(198) xbox::ntstatus_xt NTAPI xbox::NtFlushBuffersFile LOG_FUNC_END; NTSTATUS ret = xbox::status_success; - if (IsEmuHandle(FileHandle)) + if (EmuHandle::IsEmuHandle(FileHandle)) LOG_UNIMPLEMENTED(); else ret = NtDll::NtFlushBuffersFile(FileHandle, (NtDll::IO_STATUS_BLOCK*)IoStatusBlock); diff --git a/src/core/kernel/support/EmuFile.cpp b/src/core/kernel/support/EmuFile.cpp index c5c6113e0..83b3d6f8b 100644 --- a/src/core/kernel/support/EmuFile.cpp +++ b/src/core/kernel/support/EmuFile.cpp @@ -255,9 +255,50 @@ EmuHandle::EmuHandle(EmuNtObject* ntObject) NtObject = ntObject; } +std::unordered_map EmuHandle::EmuHandleLookup = {}; +std::mutex EmuHandle::EmuHandleLookupLock = {}; + +EmuHandle* EmuHandle::CreateEmuHandle(EmuNtObject* ntObject) { + auto emuHandle = new EmuHandle(ntObject); + + // Register EmuHandle + EmuHandleLookupLock.lock(); + EmuHandleLookup.emplace(EmuHandleToHandle(emuHandle), emuHandle); + EmuHandleLookupLock.unlock(); + + return emuHandle; +} + NTSTATUS EmuHandle::NtClose() { - return NtObject->NtClose(); + auto status = NtObject->NtClose(); + + // Unregister the handle + if (status == STATUS_SUCCESS) { + EmuHandleLookupLock.lock(); + EmuHandleLookup.erase(EmuHandleToHandle(this)); + EmuHandleLookupLock.unlock(); + } + + return status; +} + +bool EmuHandle::IsEmuHandle(HANDLE Handle) +{ + auto iter = EmuHandleLookup.find(Handle); + return !(iter == EmuHandleLookup.end()); +} + +EmuHandle* HandleToEmuHandle(HANDLE Handle) +{ + return (EmuHandle*)((uint32_t)Handle & 0x7FFFFFFF); +} + +HANDLE EmuHandleToHandle(EmuHandle* emuHandle) +{ + // Set the high bit + // Avoids collisions with real Xbox handles..? + return (HANDLE)((uint32_t)emuHandle | 0x80000000); } NTSTATUS EmuHandle::NtDuplicateObject(PHANDLE TargetHandle, DWORD Options) @@ -274,7 +315,7 @@ EmuNtObject::EmuNtObject() HANDLE EmuNtObject::NewHandle() { RefCount++; - return EmuHandleToHandle(new EmuHandle(this)); + return EmuHandle::CreateEmuHandle(this); } NTSTATUS EmuNtObject::NtClose() @@ -292,21 +333,6 @@ EmuNtObject* EmuNtObject::NtDuplicateObject(DWORD Options) return this; } -bool IsEmuHandle(HANDLE Handle) -{ - return ((uint32_t)Handle > 0x80000000) && ((uint32_t)Handle < 0xFFFFFFFE); -} - -EmuHandle* HandleToEmuHandle(HANDLE Handle) -{ - return (EmuHandle*)((uint32_t)Handle & 0x7FFFFFFF); -} - -HANDLE EmuHandleToHandle(EmuHandle* emuHandle) -{ - return (HANDLE)((uint32_t)emuHandle | 0x80000000); -} - std::wstring string_to_wstring(std::string const & src) { std::wstring result = std::wstring(src.length(), L' '); diff --git a/src/core/kernel/support/EmuFile.h b/src/core/kernel/support/EmuFile.h index 8690f6970..c8be90781 100644 --- a/src/core/kernel/support/EmuFile.h +++ b/src/core/kernel/support/EmuFile.h @@ -30,6 +30,8 @@ #include #include #include +#include +#include // ****************************************************************** // * prevent name collisions @@ -160,10 +162,21 @@ NTSTATUS CxbxConvertFilePath(std::string RelativeXboxPath, OUT std::wstring &Rel class EmuHandle { public: - EmuHandle(EmuNtObject* ntObject); NTSTATUS NtClose(); NTSTATUS NtDuplicateObject(PHANDLE TargetHandle, DWORD Options); EmuNtObject* NtObject; + + static EmuHandle* CreateEmuHandle(EmuNtObject* ntObject); + static bool IsEmuHandle(HANDLE Handle); +private: + EmuHandle(EmuNtObject* ntObject); + // Keep track of every EmuHandle wrapper + // If we remember what EmuHandle objects we hand out, we can tell the difference between an EmuHandle and garbage + // We used to rely on the high bit to differentiatean EmuHandles + // But titles may attempt to operate on invalid handles with the high bit set + // Test case: Amped sets a handle value to 0xFDFDFDFD (coincidentally a VS debugger guard value) + static std::unordered_map EmuHandleLookup; + static std::mutex EmuHandleLookupLock; }; // ****************************************************************** @@ -207,7 +220,6 @@ struct XboxDevice { // ****************************************************************** // * is Handle a 'special' emulated handle? // ****************************************************************** -bool IsEmuHandle(HANDLE Handle); EmuHandle* HandleToEmuHandle(HANDLE Handle); HANDLE EmuHandleToHandle(EmuHandle* emuHandle); From 7c51e2e34b01c966699a1dfe7017bf8c42827e2c Mon Sep 17 00:00:00 2001 From: Anthony Date: Thu, 25 Feb 2021 21:38:24 +1300 Subject: [PATCH 2/5] Use lock_guard instead of explicit lock/unlock --- src/core/kernel/support/EmuFile.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/kernel/support/EmuFile.cpp b/src/core/kernel/support/EmuFile.cpp index 83b3d6f8b..0931a9961 100644 --- a/src/core/kernel/support/EmuFile.cpp +++ b/src/core/kernel/support/EmuFile.cpp @@ -262,9 +262,10 @@ EmuHandle* EmuHandle::CreateEmuHandle(EmuNtObject* ntObject) { auto emuHandle = new EmuHandle(ntObject); // Register EmuHandle - EmuHandleLookupLock.lock(); - EmuHandleLookup.emplace(EmuHandleToHandle(emuHandle), emuHandle); - EmuHandleLookupLock.unlock(); + { + const std::lock_guard scopedLock(EmuHandleLookupLock); + EmuHandleLookup.emplace(EmuHandleToHandle(emuHandle), emuHandle); + } return emuHandle; } @@ -275,9 +276,8 @@ NTSTATUS EmuHandle::NtClose() // Unregister the handle if (status == STATUS_SUCCESS) { - EmuHandleLookupLock.lock(); + const std::lock_guard scopedLock(EmuHandleLookupLock); EmuHandleLookup.erase(EmuHandleToHandle(this)); - EmuHandleLookupLock.unlock(); } return status; From f818e43fc723acb172ea513ef338cae58fbe6229 Mon Sep 17 00:00:00 2001 From: Anthony Date: Thu, 25 Feb 2021 22:02:41 +1300 Subject: [PATCH 3/5] - use read/write locking via shared_mutex - lock in IsEmuHandle --- src/core/kernel/support/EmuFile.cpp | 7 ++++--- src/core/kernel/support/EmuFile.h | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/core/kernel/support/EmuFile.cpp b/src/core/kernel/support/EmuFile.cpp index 0931a9961..0748f6c8b 100644 --- a/src/core/kernel/support/EmuFile.cpp +++ b/src/core/kernel/support/EmuFile.cpp @@ -256,14 +256,14 @@ EmuHandle::EmuHandle(EmuNtObject* ntObject) } std::unordered_map EmuHandle::EmuHandleLookup = {}; -std::mutex EmuHandle::EmuHandleLookupLock = {}; +std::shared_mutex EmuHandle::EmuHandleLookupLock = {}; EmuHandle* EmuHandle::CreateEmuHandle(EmuNtObject* ntObject) { auto emuHandle = new EmuHandle(ntObject); // Register EmuHandle { - const std::lock_guard scopedLock(EmuHandleLookupLock); + std::unique_lock scopedLock(EmuHandleLookupLock); EmuHandleLookup.emplace(EmuHandleToHandle(emuHandle), emuHandle); } @@ -276,7 +276,7 @@ NTSTATUS EmuHandle::NtClose() // Unregister the handle if (status == STATUS_SUCCESS) { - const std::lock_guard scopedLock(EmuHandleLookupLock); + std::unique_lock scopedLock(EmuHandleLookupLock); EmuHandleLookup.erase(EmuHandleToHandle(this)); } @@ -285,6 +285,7 @@ NTSTATUS EmuHandle::NtClose() bool EmuHandle::IsEmuHandle(HANDLE Handle) { + std::shared_lock scopedLock(EmuHandleLookupLock); auto iter = EmuHandleLookup.find(Handle); return !(iter == EmuHandleLookup.end()); } diff --git a/src/core/kernel/support/EmuFile.h b/src/core/kernel/support/EmuFile.h index c8be90781..9780fea0b 100644 --- a/src/core/kernel/support/EmuFile.h +++ b/src/core/kernel/support/EmuFile.h @@ -31,7 +31,7 @@ #include #include #include -#include +#include // ****************************************************************** // * prevent name collisions @@ -176,7 +176,7 @@ private: // But titles may attempt to operate on invalid handles with the high bit set // Test case: Amped sets a handle value to 0xFDFDFDFD (coincidentally a VS debugger guard value) static std::unordered_map EmuHandleLookup; - static std::mutex EmuHandleLookupLock; + static std::shared_mutex EmuHandleLookupLock; }; // ****************************************************************** From 16c449ddc79f639e475a1fecb0871a257a22ee37 Mon Sep 17 00:00:00 2001 From: Anthony Date: Thu, 25 Feb 2021 22:48:00 +1300 Subject: [PATCH 4/5] Drop Handle <-> EmuHandle conversion step --- src/core/kernel/exports/EmuKrnlNt.cpp | 6 +++--- src/core/kernel/support/EmuFile.cpp | 20 ++++---------------- src/core/kernel/support/EmuFile.h | 10 ++-------- 3 files changed, 9 insertions(+), 27 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnlNt.cpp b/src/core/kernel/exports/EmuKrnlNt.cpp index 6b70378ec..5ab4cbba1 100644 --- a/src/core/kernel/exports/EmuKrnlNt.cpp +++ b/src/core/kernel/exports/EmuKrnlNt.cpp @@ -145,7 +145,7 @@ XBSYSAPI EXPORTNUM(187) xbox::ntstatus_xt NTAPI xbox::NtClose if (EmuHandle::IsEmuHandle(Handle)) { // delete 'special' handles - EmuHandle *iEmuHandle = HandleToEmuHandle(Handle); + auto iEmuHandle = (EmuHandle*)Handle; ret = iEmuHandle->NtClose(); LOG_UNIMPLEMENTED(); // TODO : Base this on the Ob* functions @@ -683,7 +683,7 @@ XBSYSAPI EXPORTNUM(197) xbox::ntstatus_xt NTAPI xbox::NtDuplicateObject NTSTATUS ret = xbox::status_success; if (EmuHandle::IsEmuHandle(SourceHandle)) { - EmuHandle* iEmuHandle = HandleToEmuHandle(SourceHandle); + auto iEmuHandle = (EmuHandle*)SourceHandle; ret = iEmuHandle->NtDuplicateObject(TargetHandle, Options); /* PVOID Object; @@ -1377,7 +1377,7 @@ XBSYSAPI EXPORTNUM(215) xbox::ntstatus_xt NTAPI xbox::NtQuerySymbolicLinkObject // Check that we actually got an EmuHandle : ret = STATUS_INVALID_HANDLE; - EmuHandle* iEmuHandle = HandleToEmuHandle(LinkHandle); + auto iEmuHandle = (EmuHandle*)LinkHandle; // Retrieve the NtSymbolicLinkObject and populate the output arguments : ret = xbox::status_success; symbolicLinkObject = (EmuNtSymbolicLinkObject*)iEmuHandle->NtObject; diff --git a/src/core/kernel/support/EmuFile.cpp b/src/core/kernel/support/EmuFile.cpp index 0748f6c8b..4aa93c7bc 100644 --- a/src/core/kernel/support/EmuFile.cpp +++ b/src/core/kernel/support/EmuFile.cpp @@ -255,7 +255,7 @@ EmuHandle::EmuHandle(EmuNtObject* ntObject) NtObject = ntObject; } -std::unordered_map EmuHandle::EmuHandleLookup = {}; +std::unordered_set EmuHandle::EmuHandleLookup = {}; std::shared_mutex EmuHandle::EmuHandleLookupLock = {}; EmuHandle* EmuHandle::CreateEmuHandle(EmuNtObject* ntObject) { @@ -264,7 +264,7 @@ EmuHandle* EmuHandle::CreateEmuHandle(EmuNtObject* ntObject) { // Register EmuHandle { std::unique_lock scopedLock(EmuHandleLookupLock); - EmuHandleLookup.emplace(EmuHandleToHandle(emuHandle), emuHandle); + EmuHandleLookup.insert(emuHandle); } return emuHandle; @@ -277,7 +277,7 @@ NTSTATUS EmuHandle::NtClose() // Unregister the handle if (status == STATUS_SUCCESS) { std::unique_lock scopedLock(EmuHandleLookupLock); - EmuHandleLookup.erase(EmuHandleToHandle(this)); + EmuHandleLookup.erase(this); } return status; @@ -286,22 +286,10 @@ NTSTATUS EmuHandle::NtClose() bool EmuHandle::IsEmuHandle(HANDLE Handle) { std::shared_lock scopedLock(EmuHandleLookupLock); - auto iter = EmuHandleLookup.find(Handle); + auto iter = EmuHandleLookup.find((EmuHandle*) Handle); return !(iter == EmuHandleLookup.end()); } -EmuHandle* HandleToEmuHandle(HANDLE Handle) -{ - return (EmuHandle*)((uint32_t)Handle & 0x7FFFFFFF); -} - -HANDLE EmuHandleToHandle(EmuHandle* emuHandle) -{ - // Set the high bit - // Avoids collisions with real Xbox handles..? - return (HANDLE)((uint32_t)emuHandle | 0x80000000); -} - NTSTATUS EmuHandle::NtDuplicateObject(PHANDLE TargetHandle, DWORD Options) { *TargetHandle = NtObject->NtDuplicateObject(Options)->NewHandle(); diff --git a/src/core/kernel/support/EmuFile.h b/src/core/kernel/support/EmuFile.h index 9780fea0b..0c4d5a834 100644 --- a/src/core/kernel/support/EmuFile.h +++ b/src/core/kernel/support/EmuFile.h @@ -30,7 +30,7 @@ #include #include #include -#include +#include #include // ****************************************************************** @@ -175,7 +175,7 @@ private: // We used to rely on the high bit to differentiatean EmuHandles // But titles may attempt to operate on invalid handles with the high bit set // Test case: Amped sets a handle value to 0xFDFDFDFD (coincidentally a VS debugger guard value) - static std::unordered_map EmuHandleLookup; + static std::unordered_set EmuHandleLookup; static std::shared_mutex EmuHandleLookupLock; }; @@ -217,12 +217,6 @@ struct XboxDevice { HANDLE HostRootHandle; }; -// ****************************************************************** -// * is Handle a 'special' emulated handle? -// ****************************************************************** -EmuHandle* HandleToEmuHandle(HANDLE Handle); -HANDLE EmuHandleToHandle(EmuHandle* emuHandle); - CHAR* NtStatusToString(IN NTSTATUS Status); int CxbxRegisterDeviceHostPath(std::string XboxFullPath, std::string HostDevicePath, bool IsFile = false); From de20deaa8a1f0efae160781ad2c01648bf80a2f8 Mon Sep 17 00:00:00 2001 From: Anthony Date: Fri, 26 Feb 2021 20:59:06 +1300 Subject: [PATCH 5/5] Detect when NtQuerySymbolicLinkObject is called without an EmuHandle --- src/core/kernel/exports/EmuKrnlNt.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnlNt.cpp b/src/core/kernel/exports/EmuKrnlNt.cpp index 5ab4cbba1..efbebd61a 100644 --- a/src/core/kernel/exports/EmuKrnlNt.cpp +++ b/src/core/kernel/exports/EmuKrnlNt.cpp @@ -1371,11 +1371,14 @@ XBSYSAPI EXPORTNUM(215) xbox::ntstatus_xt NTAPI xbox::NtQuerySymbolicLinkObject LOG_FUNC_ARG_OUT(ReturnedLength) LOG_FUNC_END; - NTSTATUS ret = 0; + NTSTATUS ret = STATUS_INVALID_HANDLE; EmuNtSymbolicLinkObject* symbolicLinkObject = NULL; - // Check that we actually got an EmuHandle : - ret = STATUS_INVALID_HANDLE; + // We expect LinkHandle to always be an EmuHandle + if (!EmuHandle::IsEmuHandle(LinkHandle)) { + LOG_UNIMPLEMENTED(); + return ret; + } auto iEmuHandle = (EmuHandle*)LinkHandle; // Retrieve the NtSymbolicLinkObject and populate the output arguments :