Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[spring 1.0] avoid spurious access violation on thread due to other timed out thread #30

Merged
merged 2 commits into from
Jan 21, 2025
Merged

[spring 1.0] avoid spurious access violation on thread due to other timed out thread #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2a81f3
avoid spurious access violation from other timedout threads
spoonincode Jan 15, 2025
1e84cb8
comment & name changes from review
spoonincode Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions include/eosio/vm/backend.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,20 +305,25 @@ namespace eosio { namespace vm {

template<typename Watchdog, typename F>
inline void timed_run(Watchdog&& wd, F&& f) {
std::atomic<bool> _timed_out = false;
//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<bool>& _timed_out = timed_run_has_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&&>(f)();
} catch(wasm_memory_exception&) {
if (_timed_out) {
if (_timed_out.load(std::memory_order_acquire)) {
throw timeout_exception{ "execution timed out" };
} else {
throw;
Expand Down
6 changes: 3 additions & 3 deletions include/eosio/vm/execution_context.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,11 +358,11 @@ namespace eosio { namespace vm {

vm::invoke_with_signal_handler([&]() {
result = execute<sizeof...(Args)>(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<sizeof...(Args)>(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&) {
Expand Down Expand Up @@ -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) {
Expand Down
92 changes: 54 additions & 38 deletions include/eosio/vm/signals.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@ namespace eosio { namespace vm {
inline thread_local std::atomic<sigjmp_buf*> signal_dest{nullptr};

__attribute__((visibility("default")))
inline thread_local std::vector<std::span<std::byte>> protected_memory_ranges;
inline thread_local std::span<std::byte> code_memory_range;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just name code_memory_range as code_range?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I think it mirrors the naming of existing get_*_span() calls. s/_span$/_memory_range/ (i.e. keeping the prefix in tact)


__attribute__((visibility("default")))
inline thread_local std::span<std::byte> memory_range;

__attribute__((visibility("default")))
inline thread_local std::atomic<bool> timed_run_has_timed_out{false};

// Fixes a duplicate symbol build issue when building with `-fvisibility=hidden`
__attribute__((visibility("default")))
Expand All @@ -27,46 +33,55 @@ namespace eosio { namespace vm {
template<int Sig>
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<std::byte>& 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<SIGSEGV>; break;
case SIGBUS: prev_action = &prev_signal_handler<SIGBUS>; break;
case SIGFPE: prev_action = &prev_signal_handler<SIGFPE>; 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_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_has_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<SIGSEGV>; break;
case SIGBUS: prev_action = &prev_signal_handler<SIGBUS>; break;
case SIGFPE: prev_action = &prev_signal_handler<SIGFPE>; 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);
}
}
}
Expand Down Expand Up @@ -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<typename F, typename E>
[[gnu::noinline]] auto invoke_with_signal_handler(F&& f, E&& e, const std::vector<std::span<std::byte>>& 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
Expand Down
4 changes: 2 additions & 2 deletions tests/signals_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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_<eosio::vm::wasm_exit_exception>( "Exiting" );
}, [](int){}, {}), eosio::vm::wasm_exit_exception);
}, [](int){}, {}, {}), eosio::vm::wasm_exit_exception);
}

static volatile sig_atomic_t sig_handled;
Expand Down