From 47d04ae8073af0fdffe4de3fbcfbd26753d8c0bc Mon Sep 17 00:00:00 2001 From: Ryan Houdek Date: Fri, 19 Nov 2021 12:05:30 -0800 Subject: [PATCH] FEXCore: Fixes ERROR_AND_DIE ERROR_AND_DIE was using __builtin_trap which would send our application either a SIGILL or SIGTRAP depending on architecture. This would then be captured by our faulting system and passed over to the guest application. If the guest application happened to have a signal handler installed for these then it would pick up this fault and potentially continue unsafely. Now we can remove this usage of __builtin_trap and switch over to our own handler. Our frontend will check to see if the fault came from our handler and uninstall the host signal handlers in this case. Which is what we want for "ERROR_AND_DIE" --- External/FEXCore/Source/CMakeLists.txt | 1 + .../FEXCore/Source/Utils/ForcedAssert.cpp | 14 +++++++++++++ .../include/FEXCore/Utils/CompilerDefs.h | 9 +++++++-- Source/Tests/FEXLoader.cpp | 20 +++++++++++++++++++ .../Tests/LinuxSyscalls/SignalDelegator.cpp | 6 ++++++ Source/Tests/LinuxSyscalls/SignalDelegator.h | 1 + 6 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 External/FEXCore/Source/Utils/ForcedAssert.cpp diff --git a/External/FEXCore/Source/CMakeLists.txt b/External/FEXCore/Source/CMakeLists.txt index 45c33d790..f7b2fc3a3 100644 --- a/External/FEXCore/Source/CMakeLists.txt +++ b/External/FEXCore/Source/CMakeLists.txt @@ -142,6 +142,7 @@ set (SRCS Utils/Allocator.cpp Utils/Allocator/64BitAllocator.cpp Utils/FileLoading.cpp + Utils/ForcedAssert.cpp Utils/LogManager.cpp Utils/Telemetry.cpp Utils/Threads.cpp diff --git a/External/FEXCore/Source/Utils/ForcedAssert.cpp b/External/FEXCore/Source/Utils/ForcedAssert.cpp new file mode 100644 index 000000000..4d1917949 --- /dev/null +++ b/External/FEXCore/Source/Utils/ForcedAssert.cpp @@ -0,0 +1,14 @@ +namespace FEXCore::Assert { + // This function can not be inlined + [[noreturn]] + __attribute__((noinline, naked)) + void ForcedAssert() { +#ifdef _M_X86_64 + asm volatile("ud2"); +#else + asm volatile("hlt #1"); +#endif + } +} + + diff --git a/External/FEXCore/include/FEXCore/Utils/CompilerDefs.h b/External/FEXCore/include/FEXCore/Utils/CompilerDefs.h index 4e52dddab..37b533fdd 100644 --- a/External/FEXCore/include/FEXCore/Utils/CompilerDefs.h +++ b/External/FEXCore/include/FEXCore/Utils/CompilerDefs.h @@ -1,5 +1,4 @@ #pragma once - // Contains general abstractions related to compilers used to build FEX. // Specifies the minimum alignment for a variable or structure field, measured in bytes. @@ -21,9 +20,15 @@ #define FEX_PACKED __attribute__((packed)) // Causes execution to exit abnormally. -#define FEX_TRAP_EXECUTION __builtin_trap() +#define FEX_TRAP_EXECUTION FEXCore::Assert::ForcedAssert() // Dictates to the compiler that the path this is on should not be reachable // from normal execution control flow. If normal execution does reach this, // then program behavior is undefined. #define FEX_UNREACHABLE __builtin_unreachable() + +namespace FEXCore::Assert { + // This function can not be inlined + [[noreturn]] + FEX_DEFAULT_VISIBILITY void ForcedAssert(); +} diff --git a/Source/Tests/FEXLoader.cpp b/Source/Tests/FEXLoader.cpp index e1702a878..5d62abc7f 100644 --- a/Source/Tests/FEXLoader.cpp +++ b/Source/Tests/FEXLoader.cpp @@ -49,6 +49,7 @@ $end_info$ #include #include +#include namespace { static bool SilentLog; @@ -534,6 +535,25 @@ int main(int argc, char **argv, char **const envp) { FEXCore::Context::InitializeContext(CTX); auto SignalDelegation = std::make_unique(); + + SignalDelegation->RegisterFrontendHostSignalHandler(SIGILL, [&SignalDelegation](FEXCore::Core::InternalThreadState *Thread, int Signal, void *info, void *ucontext) -> bool { + ucontext_t* _context = (ucontext_t*)ucontext; + auto &mcontext = _context->uc_mcontext; + uint64_t PC{}; +#ifdef _M_ARM_64 + PC = mcontext.pc; +#else + PC = mcontext.gregs[REG_RIP]; +#endif + if (PC == reinterpret_cast(&FEXCore::Assert::ForcedAssert)) { + // This is a host side assert. Don't deliver this to the guest + // We want to actually break here + SignalDelegation->UninstallHostHandler(Signal); + return true; + } + return false; + }, true); + auto SyscallHandler = Loader.Is64BitMode() ? FEX::HLE::x64::CreateHandler(CTX, SignalDelegation.get()) : FEX::HLE::x32::CreateHandler(CTX, SignalDelegation.get(), std::move(Allocator)); diff --git a/Source/Tests/LinuxSyscalls/SignalDelegator.cpp b/Source/Tests/LinuxSyscalls/SignalDelegator.cpp index 4ff64ef65..4356d931d 100644 --- a/Source/Tests/LinuxSyscalls/SignalDelegator.cpp +++ b/Source/Tests/LinuxSyscalls/SignalDelegator.cpp @@ -241,6 +241,12 @@ namespace FEX::HLE { return true; } + void SignalDelegator::UninstallHostHandler(int Signal) { + SignalHandler &SignalHandler = HostHandlers[Signal]; + + ::syscall(SYS_rt_sigaction, Signal, &SignalHandler.OldAction, nullptr, 8); + } + SignalDelegator::SignalDelegator() { // Register this delegate LOGMAN_THROW_A(!GlobalDelegator, "Can't register global delegator multiple times!"); diff --git a/Source/Tests/LinuxSyscalls/SignalDelegator.h b/Source/Tests/LinuxSyscalls/SignalDelegator.h index 0edf7f769..2430e9403 100644 --- a/Source/Tests/LinuxSyscalls/SignalDelegator.h +++ b/Source/Tests/LinuxSyscalls/SignalDelegator.h @@ -55,6 +55,7 @@ namespace FEX::HLE { uint64_t GuestSignalFD(int fd, const uint64_t *set, size_t sigsetsize , int flags); /** @} */ + void UninstallHostHandler(int Signal); protected: // Called from the thunk handler to handle the signal void HandleGuestSignal(FEXCore::Core::InternalThreadState *Thread, int Signal, void *Info, void *UContext) override;