From 829384e488139ded2e3f9d8cf54b17c9af5158da Mon Sep 17 00:00:00 2001 From: Ryan Houdek Date: Sun, 5 Nov 2023 04:49:02 -0800 Subject: [PATCH] JITArm64: Fixes double munmap issue that was causing crashes While tracking issues in #3162, I had encountered a random crash that I started hunting. It was very quickly apparent that this crash was unrelated to that PR. I just happened to be running a unittest that was creating and tearing down a bunch of threads that exacerbated the problem. See as follows with the strace output: ``` [pid 269497] munmap(0x7fffde1ff000, 16777216) = 0 [pid 269497] munmap(0x7fffde1ff000, 16777216 [pid 268982] mmap(NULL, 16777216, PROT_READ|PROT_WRITE|PROT_EXEC, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fffde1ff000 [pid 269497] <... munmap resumed>) = 0 ``` One thread is freeing some memory with munmap, another one then does a mmap and gets the same address back. Nothing too crazy at initial glance, but taking a closelier look, we can see that there are two strange oddities: 1) We are double unmapping the same address range through munmap 2) The second munmap is interrupted and returns AFTER the mmap. This has the unfortunate side-effect that the mmap that just returned the same address has actually just been unmapped! This was resulting in spurious crashes around thread creation that was SUPER hard to nail down. The problem comes down to how code buffer objects are managed, in particular how the Arm64Emitter and Dispatcher handled its buffers. Arm64Emitter is inherited by two classes; Dispatcher, and Arm64JITCore. On class destruction the emitter would free its internal tracking buffer. Additionally on destruction, the Arm64JITCore would walk through all of its CodeBuffers and free them. The problem ends up being that in the Arm64JITCore, it would free its code buffers which also ended up being the current active buffer bound to the Arm64Emitter. Thus causing the Arm64Emitter to come back around and try to free the same buffer again. This is a double-free problem! and was only visible on thread exiting! Can't track double frees with mmap and munmap with current tooling! This problem typically didn't occur because of how fast the destruction usually takes and jemalloc inbetween also typically means the problem doesn't occur. Initially thinking this was a threaded pool allocator bug because typically the new allocation would end up in there once a new thread was spinning up. Now we change behaviour, Arm64Emitter doesn't do any buffer management itself, instead just passing an initial buffer on to its internal buffer tracking if given one up front. This leaves the Dispatcher and the Arm64JITCore to do their buffer management and ensuring there is no double free. The day is saved! --- .../Interface/Core/ArchHelpers/Arm64Emitter.cpp | 11 ++--------- .../Source/Interface/Core/ArchHelpers/Arm64Emitter.h | 3 +-- .../Source/Interface/Core/Dispatcher/Dispatcher.cpp | 9 ++++++++- FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.h | 2 +- FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp | 2 +- 5 files changed, 13 insertions(+), 14 deletions(-) diff --git a/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.cpp b/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.cpp index ade10e9af..f243c4951 100644 --- a/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.cpp +++ b/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.cpp @@ -335,8 +335,8 @@ namespace x32 { } // We want vixl to not allocate a default buffer. Jit and dispatcher will manually create one. -Arm64Emitter::Arm64Emitter(FEXCore::Context::ContextImpl *ctx, size_t size) - : Emitter(size ? (uint8_t*)FEXCore::Allocator::VirtualAlloc(size, true) : nullptr, size) +Arm64Emitter::Arm64Emitter(FEXCore::Context::ContextImpl *ctx, void* EmissionPtr, size_t size) + : Emitter(static_cast(EmissionPtr), size) , EmitterCTX {ctx} #ifdef VIXL_SIMULATOR , Simulator {&SimDecoder} @@ -379,13 +379,6 @@ Arm64Emitter::Arm64Emitter(FEXCore::Context::ContextImpl *ctx, size_t size) } } -Arm64Emitter::~Arm64Emitter() { - auto BufferSize = GetBufferSize(); - if (BufferSize) { - FEXCore::Allocator::VirtualFree(GetBufferBase(), BufferSize); - } -} - void Arm64Emitter::LoadConstant(ARMEmitter::Size s, ARMEmitter::Register Reg, uint64_t Constant, bool NOPPad) { bool Is64Bit = s == ARMEmitter::Size::i64Bit; int Segments = Is64Bit ? 4 : 2; diff --git a/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.h b/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.h index 13a3701c7..92e99be12 100644 --- a/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.h +++ b/FEXCore/Source/Interface/Core/ArchHelpers/Arm64Emitter.h @@ -57,8 +57,7 @@ constexpr FEXCore::ARMEmitter::PRegister PRED_TMP_32B = FEXCore::ARMEmitter::PRe // be used by both Arm64 JIT and ARM64 Dispatcher class Arm64Emitter : public FEXCore::ARMEmitter::Emitter { protected: - Arm64Emitter(FEXCore::Context::ContextImpl *ctx, size_t size); - ~Arm64Emitter(); + Arm64Emitter(FEXCore::Context::ContextImpl *ctx, void* EmissionPtr = nullptr, size_t size = 0); FEXCore::Context::ContextImpl *EmitterCTX; vixl::aarch64::CPU CPU; diff --git a/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp b/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp index 0c296de84..2d490931a 100644 --- a/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp +++ b/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp @@ -56,12 +56,19 @@ uint64_t Dispatcher::GetCompileBlockPtr() { constexpr size_t MAX_DISPATCHER_CODE_SIZE = 4096 * 2; Dispatcher::Dispatcher(FEXCore::Context::ContextImpl *ctx, const DispatcherConfig &config) - : Arm64Emitter(ctx, MAX_DISPATCHER_CODE_SIZE) + : Arm64Emitter(ctx, FEXCore::Allocator::VirtualAlloc(MAX_DISPATCHER_CODE_SIZE, true), MAX_DISPATCHER_CODE_SIZE) , CTX {ctx} , config {config} { EmitDispatcher(); } +Dispatcher::~Dispatcher() { + auto BufferSize = GetBufferSize(); + if (BufferSize) { + FEXCore::Allocator::VirtualFree(GetBufferBase(), BufferSize); + } +} + void Dispatcher::EmitDispatcher() { #ifdef VIXL_DISASSEMBLER const auto DisasmBegin = GetCursorAddress(); diff --git a/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.h b/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.h index c7056ec57..36b3f0318 100644 --- a/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.h +++ b/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.h @@ -43,7 +43,7 @@ public: static fextl::unique_ptr Create(FEXCore::Context::ContextImpl *CTX, const DispatcherConfig &Config); Dispatcher(FEXCore::Context::ContextImpl *ctx, const DispatcherConfig &Config); - ~Dispatcher() = default; + ~Dispatcher(); /** * @name Dispatch Helper functions diff --git a/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp b/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp index 7df56dff2..fe3454b75 100644 --- a/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp +++ b/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp @@ -530,7 +530,7 @@ void Arm64JITCore::Op_NoOp(IR::IROp_Header const *IROp, IR::NodeID Node) { Arm64JITCore::Arm64JITCore(FEXCore::Context::ContextImpl *ctx, FEXCore::Core::InternalThreadState *Thread) : CPUBackend(Thread, INITIAL_CODE_SIZE, MAX_CODE_SIZE) - , Arm64Emitter(ctx, 0) + , Arm64Emitter(ctx) , HostSupportsSVE128{ctx->HostFeatures.SupportsSVE} , HostSupportsSVE256{ctx->HostFeatures.SupportsAVX} , HostSupportsRPRES{ctx->HostFeatures.SupportsRPRES}