From d2a81f3951c91533624561950acfcb993189c70f Mon Sep 17 00:00:00 2001 From: Matt Witherspoon <32485495+spoonincode@users.noreply.github.com> Date: Tue, 14 Jan 2025 23:17:21 -0500 Subject: [PATCH 1/2] avoid spurious access violation from other timedout threads --- include/eosio/vm/backend.hpp | 9 +-- include/eosio/vm/execution_context.hpp | 6 +- include/eosio/vm/signals.hpp | 92 +++++++++++++++----------- tests/signals_tests.cpp | 4 +- 4 files changed, 64 insertions(+), 47 deletions(-) diff --git a/include/eosio/vm/backend.hpp b/include/eosio/vm/backend.hpp index b0af3e0..9940822 100644 --- a/include/eosio/vm/backend.hpp +++ b/include/eosio/vm/backend.hpp @@ -305,20 +305,21 @@ namespace eosio { namespace vm { template inline void timed_run(Watchdog&& wd, F&& f) { - std::atomic _timed_out = false; + std::atomic& _timed_out = timed_run_is_timed_out; auto reenable_code = scope_guard{[&](){ - if (_timed_out) { + if (_timed_out.load(std::memory_order_acquire)) { mod->allocator.enable_code(Impl::is_jit); + _timed_out.store(false, std::memory_order_release); } }}; try { auto wd_guard = wd.scoped_run([this,&_timed_out]() { - _timed_out = true; + _timed_out.store(true, std::memory_order_release); mod->allocator.disable_code(); }); static_cast(f)(); } catch(wasm_memory_exception&) { - if (_timed_out) { + if (_timed_out.load(std::memory_order_acquire)) { throw timeout_exception{ "execution timed out" }; } else { throw; diff --git a/include/eosio/vm/execution_context.hpp b/include/eosio/vm/execution_context.hpp index e6ac59c..8473a58 100644 --- a/include/eosio/vm/execution_context.hpp +++ b/include/eosio/vm/execution_context.hpp @@ -358,11 +358,11 @@ namespace eosio { namespace vm { vm::invoke_with_signal_handler([&]() { result = execute(args_raw, fn, this, base_type::linear_memory(), stack); - }, &handle_signal, {_mod->allocator.get_code_span(), base_type::get_wasm_allocator()->get_span()}); + }, &handle_signal, _mod->allocator, base_type::get_wasm_allocator()); } else { vm::invoke_with_signal_handler([&]() { result = execute(args_raw, fn, this, base_type::linear_memory(), stack); - }, &handle_signal, {_mod->allocator.get_code_span(), base_type::get_wasm_allocator()->get_span()}); + }, &handle_signal, _mod->allocator, base_type::get_wasm_allocator()); } } } catch(wasm_exit_exception&) { @@ -800,7 +800,7 @@ namespace eosio { namespace vm { setup_locals(func_index); vm::invoke_with_signal_handler([&]() { execute(visitor); - }, &handle_signal, {_mod->allocator.get_code_span(), base_type::get_wasm_allocator()->get_span()}); + }, &handle_signal, _mod->allocator, base_type::get_wasm_allocator()); } if (_mod->get_function_type(func_index).return_count && !_state.exiting) { diff --git a/include/eosio/vm/signals.hpp b/include/eosio/vm/signals.hpp index b5a5312..040798e 100644 --- a/include/eosio/vm/signals.hpp +++ b/include/eosio/vm/signals.hpp @@ -18,7 +18,13 @@ namespace eosio { namespace vm { inline thread_local std::atomic signal_dest{nullptr}; __attribute__((visibility("default"))) - inline thread_local std::vector> protected_memory_ranges; + inline thread_local std::span code_memory_range; + + __attribute__((visibility("default"))) + inline thread_local std::span memory_range; + + __attribute__((visibility("default"))) + inline thread_local std::atomic timed_run_is_timed_out{false}; // Fixes a duplicate symbol build issue when building with `-fvisibility=hidden` __attribute__((visibility("default"))) @@ -27,46 +33,55 @@ namespace eosio { namespace vm { template inline struct sigaction prev_signal_handler; - inline bool in_protected_range(void* addr) { - //empty protection list means legacy catch-all behavior; useful for some of the old tests - if(protected_memory_ranges.empty()) - return true; - - for(const std::span& range : protected_memory_ranges) { - if(addr >= range.data() && addr < range.data() + range.size()) - return true; - } - return false; - } - inline void signal_handler(int sig, siginfo_t* info, void* uap) { sigjmp_buf* dest = std::atomic_load(&signal_dest); - if (dest && in_protected_range(info->si_addr)) { - siglongjmp(*dest, sig); - } else { - struct sigaction* prev_action; - switch(sig) { - case SIGSEGV: prev_action = &prev_signal_handler; break; - case SIGBUS: prev_action = &prev_signal_handler; break; - case SIGFPE: prev_action = &prev_signal_handler; break; - default: std::abort(); + if (dest) { + const void* addr = info->si_addr; + + //neither range set means legacy catch-all behavior; useful for some of the old tests + if (code_memory_range.empty() && memory_range.empty()) + siglongjmp(*dest, sig); + + //a failure in the memory range is always jumped out of + if (addr >= memory_range.data() && addr < memory_range.data() + memory_range.size()) + siglongjmp(*dest, sig); + + //a failure in the code range... + if (addr >= code_memory_range.data() && addr < code_memory_range.data() + code_memory_range.size()) { + //a SEGV in the code range when timed_run_is_timed_out=false is due to a _different_ thread's execution activating a deadline + // timer. Return and retry executing the same code again. Eventually timed_run() on the other thread will reset the page + // permissions and progress on this thread can continue + if (sig == SIGSEGV && timed_run_is_timed_out.load(std::memory_order_acquire) == false) + return; + //otherwise, jump out + siglongjmp(*dest, sig); } - if (!prev_action) std::abort(); - if (prev_action->sa_flags & SA_SIGINFO) { - // FIXME: We need to be at least as strict as the original - // flags and relax the mask as needed. - prev_action->sa_sigaction(sig, info, uap); + + //if in neither range, fall through and let chained handler an opportunity to handle + } + + struct sigaction* prev_action; + switch(sig) { + case SIGSEGV: prev_action = &prev_signal_handler; break; + case SIGBUS: prev_action = &prev_signal_handler; break; + case SIGFPE: prev_action = &prev_signal_handler; break; + default: std::abort(); + } + if (!prev_action) std::abort(); + if (prev_action->sa_flags & SA_SIGINFO) { + // FIXME: We need to be at least as strict as the original + // flags and relax the mask as needed. + prev_action->sa_sigaction(sig, info, uap); + } else { + if(prev_action->sa_handler == SIG_DFL) { + // The default for all three signals is to terminate the process. + sigaction(sig, prev_action, nullptr); + raise(sig); + } else if(prev_action->sa_handler == SIG_IGN) { + // Do nothing } else { - if(prev_action->sa_handler == SIG_DFL) { - // The default for all three signals is to terminate the process. - sigaction(sig, prev_action, nullptr); - raise(sig); - } else if(prev_action->sa_handler == SIG_IGN) { - // Do nothing - } else { - prev_action->sa_handler(sig); - } + prev_action->sa_handler(sig); } } } @@ -139,11 +154,12 @@ namespace eosio { namespace vm { // It's unlikely, but I'm not sure that it can definitely be ruled out if both // this and f are inlined and f modifies locals from the caller. template - [[gnu::noinline]] auto invoke_with_signal_handler(F&& f, E&& e, const std::vector>& protect_ranges) { + [[gnu::noinline]] auto invoke_with_signal_handler(F&& f, E&& e, growable_allocator& code_allocator, wasm_allocator* mem_allocator) { setup_signal_handler(); sigjmp_buf dest; sigjmp_buf* volatile old_signal_handler = nullptr; - protected_memory_ranges = protect_ranges; + code_memory_range = code_allocator.get_code_span(); + memory_range = mem_allocator->get_span(); int sig; if((sig = sigsetjmp(dest, 1)) == 0) { // Note: Cannot use RAII, as non-trivial destructors w/ longjmp diff --git a/tests/signals_tests.cpp b/tests/signals_tests.cpp index 6dfba49..fc810cb 100644 --- a/tests/signals_tests.cpp +++ b/tests/signals_tests.cpp @@ -15,7 +15,7 @@ TEST_CASE("Testing signals", "[invoke_with_signal_handler]") { std::raise(SIGSEGV); }, [](int sig) { throw test_exception{}; - }, {}); + }, {}, {}); } catch(test_exception&) { okay = true; } @@ -25,7 +25,7 @@ TEST_CASE("Testing signals", "[invoke_with_signal_handler]") { TEST_CASE("Testing throw", "[signal_handler_throw]") { CHECK_THROWS_AS(eosio::vm::invoke_with_signal_handler([](){ eosio::vm::throw_( "Exiting" ); - }, [](int){}, {}), eosio::vm::wasm_exit_exception); + }, [](int){}, {}, {}), eosio::vm::wasm_exit_exception); } static volatile sig_atomic_t sig_handled; From 1e84cb8739a95ee9e88ca1ae3150bb631d0254e8 Mon Sep 17 00:00:00 2001 From: Matt Witherspoon <32485495+spoonincode@users.noreply.github.com> Date: Tue, 21 Jan 2025 14:31:51 -0500 Subject: [PATCH 2/2] comment & name changes from review --- include/eosio/vm/backend.hpp | 6 +++++- include/eosio/vm/signals.hpp | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/include/eosio/vm/backend.hpp b/include/eosio/vm/backend.hpp index 9940822..677cc8d 100644 --- a/include/eosio/vm/backend.hpp +++ b/include/eosio/vm/backend.hpp @@ -305,7 +305,11 @@ namespace eosio { namespace vm { template inline void timed_run(Watchdog&& wd, F&& f) { - std::atomic& _timed_out = timed_run_is_timed_out; + //timed_run_has_timed_out -- declared in signal handling code because signal handler needs to inspect it on a SEGV too -- is a thread local + // so that upon a SEGV the signal handling code can discern if the thread that caused the SEGV has a timed_run that has timed out. This + // thread local also need to be an atomic because the thread that a Watchdog callback will be called from may not be the same as the + // executing thread. + std::atomic& _timed_out = timed_run_has_timed_out; auto reenable_code = scope_guard{[&](){ if (_timed_out.load(std::memory_order_acquire)) { mod->allocator.enable_code(Impl::is_jit); diff --git a/include/eosio/vm/signals.hpp b/include/eosio/vm/signals.hpp index 040798e..2a4109f 100644 --- a/include/eosio/vm/signals.hpp +++ b/include/eosio/vm/signals.hpp @@ -24,7 +24,7 @@ namespace eosio { namespace vm { inline thread_local std::span memory_range; __attribute__((visibility("default"))) - inline thread_local std::atomic timed_run_is_timed_out{false}; + inline thread_local std::atomic timed_run_has_timed_out{false}; // Fixes a duplicate symbol build issue when building with `-fvisibility=hidden` __attribute__((visibility("default"))) @@ -49,10 +49,10 @@ namespace eosio { namespace vm { //a failure in the code range... if (addr >= code_memory_range.data() && addr < code_memory_range.data() + code_memory_range.size()) { - //a SEGV in the code range when timed_run_is_timed_out=false is due to a _different_ thread's execution activating a deadline + //a SEGV in the code range when timed_run_has_timed_out=false is due to a _different_ thread's execution activating a deadline // timer. Return and retry executing the same code again. Eventually timed_run() on the other thread will reset the page // permissions and progress on this thread can continue - if (sig == SIGSEGV && timed_run_is_timed_out.load(std::memory_order_acquire) == false) + if (sig == SIGSEGV && timed_run_has_timed_out.load(std::memory_order_acquire) == false) return; //otherwise, jump out siglongjmp(*dest, sig);