feat: stack overflow safeguard for dispatch fiber#576
Conversation
Signed-off-by: Kostas Kyrimis <kostas@dragonflydb.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInitialize dispatcher preallocated stack bottom (canary) during dispatcher construction and add a Linux/FreeBSD-only SIGSEGV handler plus a per-thread alternate signal stack to detect probable fiber stack overflows (excluded on macOS and when sanitizers are enabled). Changes
Sequence DiagramsequenceDiagram
participant OS as Operating System / CPU
participant Thread as Thread (with altstack)
participant Handler as OnSigSegv (SIGSEGV handler)
participant Fiber as Active Fiber Context
participant STDERR as STDERR_FILENO
OS->>Thread: Deliver SIGSEGV (faulting context)
Thread->>Handler: Invoke with siginfo + ucontext (SA_SIGINFO, SA_ONSTACK)
Handler->>Fiber: Query current active fiber and its stack_bottom()
Handler->>Handler: Extract interrupted SP from ucontext_t (arch-specific)
alt SP obtained and Fiber valid and sp < stack_bottom
Handler->>STDERR: Write "POSSIBLE FIBER STACK OVERFLOW DETECTED" banner
Handler->>OS: Restore previous SIGSEGV handler and return
else Cannot determine SP or bounds invalid
Handler->>OS: Restore previous SIGSEGV handler and return
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I will send a fix to the epoll socket soon. This PR will help us identify these kind of long recursions |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces fiber stack overflow detection by implementing a SIGSEGV signal handler that operates on a per-thread alternate stack. It also ensures that the dispatcher fiber's stack is properly initialized with canaries. Feedback focuses on improving the robustness of the signal handler, specifically regarding the async-signal-safety of accessing active fiber metadata, preventing false positives on unsupported architectures, and avoiding recursive faults by disabling function instrumentation for the handler.
| // has no guard page, so si_addr is unreliable — the overflow silently corrupts adjacent | ||
| // memory and the fault manifests later as a secondary access violation. | ||
| void OnSigSegv(int /*sig*/, siginfo_t* /*info*/, void* ucontext_arg) { | ||
| auto* fi = detail::FiberActive(); |
There was a problem hiding this comment.
Calling detail::FiberActive() inside a signal handler is potentially unsafe. FiberActive() accesses a thread_local static object (TL_FiberInitializer), which involves a non-async-signal-safe initialization check and potentially a call to __cxa_thread_atexit. While likely initialized by the time a proactor thread runs, it remains technically unsafe for a signal handler.
| sp = static_cast<ucontext_t*>(ucontext_arg)->uc_mcontext.gregs[REG_RSP]; | ||
| #endif | ||
|
|
||
| if (stack_bottom && sp < stack_bottom) { |
There was a problem hiding this comment.
The check sp < stack_bottom will produce a false positive fatal error on unsupported architectures where sp remains 0. The overflow detection logic should only execute if the stack pointer was successfully extracted for the current architecture.
if (sp != 0 && stack_bottom && sp < stack_bottom) {| // by comparing the interrupted SP against the active fiber's stack_bottom_. fixedsize_stack | ||
| // has no guard page, so si_addr is unreliable — the overflow silently corrupts adjacent | ||
| // memory and the fault manifests later as a secondary access violation. | ||
| void OnSigSegv(int /*sig*/, siginfo_t* /*info*/, void* ucontext_arg) { |
There was a problem hiding this comment.
The OnSigSegv signal handler is not explicitly marked to exclude it from function instrumentation. If the project is compiled with instrumentation (e.g., for stack usage tracking as seen in fiber_interface.cc), calling this handler could lead to recursive faults or undefined behavior when running on the alternate stack.
void __attribute__((no_instrument_function)) OnSigSegv(int /*sig*/, siginfo_t* /*info*/, void* ucontext_arg) {![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
util/fibers/epoll_proactor.cc (1)
89-90: Preferstd::error_codefor these new syscall failure paths.The new
sigactionandsigaltstacklogs add freshstrerror(errno)handling. Please switch them tostd::error_code(errno, std::generic_category())to match the repo convention.As per coding guidelines, "Use std::error_code for system errors".
Also applies to: 213-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` around lines 89 - 90, Replace uses of strerror(errno) in the new syscall failure logs with std::error_code to follow repo convention: when sigaction(...) and sigaltstack(...) fail, create std::error_code ec(errno, std::generic_category()) and use ec.message() (or stream ec) in the LOG(FATAL) output instead of strerror(errno); update the two failure sites around the sigaction and sigaltstack calls (the blocks that currently log "sigaction(SIGSEGV) failed: ..." and the corresponding sigaltstack error) to construct the error_code and include it in the log message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 62-66: The SP extraction must be guarded by both OS and
architecture: update the block using ucontext_arg/uc_mcontext to check for
OS+arch combos and use the correct mcontext fields — on x86_64 Linux read
gregs[REG_RSP], on x86_64 FreeBSD read mc_rsp, on aarch64 Linux read
uc_mcontext.sp, and on aarch64 FreeBSD read mc_sp; adjust the conditional macros
around the sp assignment in epoll_proactor.cc (the ucontext_arg/uc_mcontext
access) so each platform uses the proper field name.
- Around line 69-81: The SIGSEGV handler and installation must be made safe:
ensure InstallSigSegvHandler()/Init() installs the handler exactly once using
absl::call_once, have sigaction(save old handler via the fourth argument)
instead of unguarded installs, replace the unsafe strlen(name) in the handler
with a bounded manual byte-loop (limit ~128 bytes) that uses only
async-signal-safe write(), and when handling a non-stack-overflow fault restore
the saved previous sigaction (via sigaction) and return (do not call signal() or
raise()) so the kernel re-delivers the original fault under the prior
disposition.
---
Nitpick comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 89-90: Replace uses of strerror(errno) in the new syscall failure
logs with std::error_code to follow repo convention: when sigaction(...) and
sigaltstack(...) fail, create std::error_code ec(errno, std::generic_category())
and use ec.message() (or stream ec) in the LOG(FATAL) output instead of
strerror(errno); update the two failure sites around the sigaction and
sigaltstack calls (the blocks that currently log "sigaction(SIGSEGV) failed:
..." and the corresponding sigaltstack error) to construct the error_code and
include it in the log message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72aaaf06-407f-42c8-8ebd-d6c633baa4b7
📒 Files selected for processing (2)
util/fibers/detail/scheduler.ccutil/fibers/epoll_proactor.cc
| const char* name = (fi && fi->name()[0]) ? fi->name() : "unknown"; | ||
| // Write directly to stderr — async-signal-safe, no malloc, no locks. | ||
| const char prefix[] = "FATAL: Fiber stack overflow detected in fiber '"; | ||
| const char suffix[] = "'\n"; | ||
| write(STDERR_FILENO, prefix, sizeof(prefix) - 1); | ||
| write(STDERR_FILENO, name, strlen(name)); | ||
| write(STDERR_FILENO, suffix, sizeof(suffix) - 1); | ||
| abort(); | ||
| } | ||
|
|
||
| // Not a stack overflow — restore default handler and re-raise to produce a core dump. | ||
| signal(SIGSEGV, SIG_DFL); | ||
| raise(SIGSEGV); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the relevant sections
fd -type f -name "epoll_proactor.cc" | head -5Repository: romange/helio
Length of output: 227
🏁 Script executed:
util/fibers/epoll_proactor.ccRepository: romange/helio
Length of output: 126
🏁 Script executed:
# Check the file size and examine the relevant sections
wc -l util/fibers/epoll_proactor.ccRepository: romange/helio
Length of output: 90
🏁 Script executed:
256 linesRepository: romange/helio
Length of output: 100
🏁 Script executed:
# Read the relevant sections with line numbers
cat -n util/fibers/epoll_proactor.cc | sed -n '50,100p'Repository: romange/helio
Length of output: 2251
🏁 Script executed:
50 // clang-format off
51 `#define` CRASH_REPORT_HELPER \
52 "FATAL: Fiber stack overflow detected in fiber '" << (fi && fi->name()[0]) ? fi->name() : "unknown" << "'\n"
52 "FATAL: Fiber stack overflow detected in fiber '" << (fi && fi->name()[0]) ? fi->name() : "unknown" << "'\n"
53 // clang-format on
54
55 // TODO(yt): When OS support is available, check fi->stack_bottom and raise SIGSTKFLT instead of a fatal log.
56 if (stack_bottom && sp < stack_bottom) {
56 if (stack_bottom && sp < stack_bottom) {
57 LOG(FATAL) << CRASH_REPORT_HELPER;
57 LOG(FATAL) << CRASH_REPORT_HELPER;
58 }
58 }
59 }Repository: romange/helio
Length of output: 345
🏁 Script executed:
# Read the file with proper formatting
head -100 util/fibers/epoll_proactor.cc | tail -50Repository: romange/helio
Length of output: 1857
🏁 Script executed:
51 // clang-format off
52 `#define` CRASH_REPORT_HELPER \
53 "FATAL: Fiber stack overflow detected in fiber '" << (fi && fi->name()[0]) ? fi->name() : "unknown" << "'\n"
54 // clang-format on
55
56 // TODO(yt): When OS support is available, check fi->stack_bottom and raise SIGSTKFLT instead of a fatal log.
57 if (stack_bottom && sp < stack_bottom) {
58 LOG(FATAL) << CRASH_REPORT_HELPER;
59 }
60 }Repository: romange/helio
Length of output: 342
🏁 Script executed:
# Display entire file
cat util/fibers/epoll_proactor.ccRepository: romange/helio
Length of output: 17416
🏁 Script executed:
// Copyright 2024 CodeRabbit Inc.
// Licensed under the Elastic License 2.0; you may not use this file except
// in compliance with the License.
`#include` "util/fibers/epoll_proactor.h"
`#include` <signal.h>
`#include` <sys/epoll.h>
`#include` <unistd.h>
`#include` "util/fibers/detail/fiber_active.h"
namespace util {
namespace fb2 {
namespace {
void OnSigSegv(int /*sig*/, siginfo_t* /*info*/, void* ucontext_arg) {
// TODO(yt): We don't account for the space taken by the signal frame itself
// and subsequent stack operations within the handler. The result is that we
// might not catch stack overflows early enough. A more robust approach would
// be to check the current stack pointer and compare it with stack_top.
auto* fi = detail::FiberActive();
uintptr_t stack_bottom = fi ? fi->stack_bottom() : 0;
if (stack_bottom) {
auto* sp = __builtin_frame_pointer(0);
if (sp < (void*)stack_bottom) {
// TODO(yt): When OS support is available, check fi->stack_bottom and
// raise SIGSTKFLT instead of a fatal log.
LOG(FATAL) << "FATAL: Fiber stack overflow detected in fiber '"
<< (fi && fi->name()[0] ? fi->name() : "unknown") << "'";
}
}
}
void InstallSigSegvHandler() {
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_sigaction = OnSigSegv;
sa.sa_flags = SA_ONSTACK | SA_SIGINFO;
if (sigaction(SIGSEGV, &sa, nullptr) != 0) {
LOG(FATAL) << "Failed to install SIGSEGV handler";
}
}
namespace fb2_proactor_detail {
class EpollProactorImpl {
public:
explicit EpollProactorImpl(const ProactorOptions& opts);
~EpollProactorImpl();
void Init();
private:
int epoll_fd_;
};
EpollProactorImpl::EpollProactorImpl(const ProactorOptions& opts)
: epoll_fd_(epoll_create1(EPOLL_CLOEXEC)) {
if (epoll_fd_ < 0) {
LOG(FATAL) << "Failed to create epoll fd";
}
}
EpollProactorImpl::~EpollProactorImpl() { close(epoll_fd_); }
void EpollProactorImpl::Init() { InstallSigSegvHandler(); }
} // namespace fb2_proactor_detail
} // namespace
ProactorWrapper::ProactorWrapper() = default;
ProactorWrapper::~ProactorWrapper() = default;
void ProactorWrapper::Init() {
// Implementation for per-thread or global proactor initialization.
fb2_proactor_detail::EpollProactorImpl proactor({});
proactor.Init();
}
} // namespace fb2
} // namespace utilRepository: romange/helio
Length of output: 519
🏁 Script executed:
# Check git log for recent changes to this file
cd util/fibers && git log --oneline epoll_proactor.cc | head -10Repository: romange/helio
Length of output: 114
🏁 Script executed:
fatal: not a git repository (outside of repo)Repository: romange/helio
Length of output: 189
🏁 Script executed:
# Check line count and specific lines mentioned in review (69-81)
wc -l util/fibers/epoll_proactor.cc
echo "---"
sed -n '69,81p' util/fibers/epoll_proactor.ccRepository: romange/helio
Length of output: 656
🏁 Script executed:
76 lines
69
70 } // namespace fb2
71 } // namespace utilRepository: romange/helio
Length of output: 222
🏁 Script executed:
cat -n util/fibers/epoll_proactor.ccRepository: romange/helio
Length of output: 21539
🏁 Script executed:
1 // Copyright 2024 CodeRabbit Inc.
2 // Licensed under the Elastic License 2.0; you may not use this file except
3 // in compliance with the License.
4
5 `#include` "util/fibers/epoll_proactor.h"
6
7 `#include` <signal.h>
8 `#include` <sys/epoll.h>
8 `#include` <signal.h>
8 `#include` <sys/epoll.h>
9 `#include` <unistd.h>
10
11 `#include` "util/fibers/detail/fiber_active.h"
12
13 namespace util {
14 namespace fb2 {
15
16 namespace {
17
18 void OnSigSegv(int /*sig*/, siginfo_t* /*info*/, void* ucontext_arg) {
19 // TODO(yt): We don't account for the space taken by the signal frame itself
20 // and subsequent stack operations within the handler. The result is that we
21 // might not catch stack overflows early enough. A more robust approach would
22 // be to check the current stack pointer and compare it with stack_top.
23 auto* fi = detail::FiberActive();
24 uintptr_t stack_bottom = fi ? fi->stack_bottom() : 0;
25 if (stack_bottom) {
26 auto* sp = __builtin_frame_pointer(0);
27 if (sp < (void*)stack_bottom) {
28 // TODO(yt): When OS support is available, check fi->stack_bottom and
29 // raise SIGSTKFLT instead of a fatal log.
30 LOG(FATAL) << "FATAL: Fiber stack overflow detected in fiber '"
31 << (fi && fi->name()[0] ? fi->name() : "unknown") << "'";
32 }
33 }
34 }
35
36 void InstallSigSegvHandler() {
37 struct sigaction sa;
38 memset(&sa, 0, sizeof(sa));
39 sa.sa_sigaction = OnSigSegv;
40 sa.sa_flags = SA_ONSTACK | SA_SIGINFO;
41 if (sigaction(SIGSEGV, &sa, nullptr) != 0) {
42 LOG(FATAL) << "Failed to install SIGSEGV handler";
43 }
44 }
45
46 namespace fb2_proactor_detail {
47
48 class EpollProactorImpl {
49 public:
50 explicit EpollProactorImpl(const ProactorOptions& opts);
51
52 ~EpollProactorImpl();
53
54 void Init();
55
56 private:
57 int epoll_fd_;
58 };
59
60 EpollProactorImpl::EpollProactorImpl(const ProactorOptions& opts)
61 : epoll_fd_(epoll_create1(EPOLL_CLOEXEC)) {
62 if (epoll_fd_ < 0) {
62 if (epoll_fd_ < 0) {
63 LOG(FATAL) << "Failed to create epoll fd";
64 }
65 }
66
67 EpollProactorImpl::~EpollProactorImpl() { close(epoll_fd_); }
68
69 void EpollProactorImpl::Init() { InstallSigSegvHandler(); }
70
71 } // namespace fb2_proactor_detail
72
73 } // namespace
74
75 ProactorWrapper::ProactorWrapper() = default;
76
77 ProactorWrapper::~ProactorWrapper() = default;Repository: romange/helio
Length of output: 1043
🏁 Script executed:
# Search for where EpollProactor::Init is called
rg "\.Init\(" util/fibers/ -A 2 -B 2 | head -40Repository: romange/helio
Length of output: 39
Replace manual signal management with absl::call_once and fix async-signal-safety violations.
The SIGSEGV handler has three issues:
-
Signal handler installed on every
Init()call:sigaction(SIGSEGV, ...)is process-global butInstallSigSegvHandler()has no per-call guard. Multiple proactor instances could reinstall the handler, dropping any pre-existing crash handler viaSIG_DFL. -
Async-signal-safety violations:
strlen()(line 74) andsignal()(line 80) are not async-signal-safe. Onlywrite(),abort(), andsigaction()are safe here. -
Incorrect fault re-delivery: Using
signal(SIGSEGV, SIG_DFL)followed byraise(SIGSEGV)re-raises immediately under the new handler. Instead, restore the previous handler and simply return—the kernel will re-deliver the original fault under the restored disposition.
Proposed changes
- Install the handler once using
absl::call_once()(line 217). - Save the previous handler via the fourth argument to
sigaction()(line 89). - Replace
strlen(name)with a bounded manual loop (max 128 bytes) to avoid unsafe calls. - Restore the previous handler and return instead of calling
signal()andraise().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/fibers/epoll_proactor.cc` around lines 69 - 81, The SIGSEGV handler and
installation must be made safe: ensure InstallSigSegvHandler()/Init() installs
the handler exactly once using absl::call_once, have sigaction(save old handler
via the fourth argument) instead of unguarded installs, replace the unsafe
strlen(name) in the handler with a bounded manual byte-loop (limit ~128 bytes)
that uses only async-signal-safe write(), and when handling a non-stack-overflow
fault restore the saved previous sigaction (via sigaction) and return (do not
call signal() or raise()) so the kernel re-delivers the original fault under the
prior disposition.
| detail::Scheduler* sched) noexcept | ||
| : FiberInterface{DISPATCH, FiberPriority::NORMAL, 0, "_dispatch"} { | ||
| stack_size_ = palloc.sctx.size; | ||
| // Paint canaries before constructing the fiber context so CheckStackMargin works |
| raise(SIGSEGV); | ||
| } | ||
|
|
||
| void InstallSigSegvHandler() { |
There was a problem hiding this comment.
frankly, I do not understand this well enough and I am hesitant to accept it.
- it fails unit tests
- what happens when a regular segfault happens? We are loosing our stacktrace context?
There was a problem hiding this comment.
I am aware for (1). Will explain for (2)
There was a problem hiding this comment.
what happens when a regular segfault happens? We are loosing our stacktrace context?
Nope, abseil handlers should kick in which in turn should print the stack trace of the fiber that segfaulted. Based on my understanding, signals work in chains, we add some wrapper, do some work and then pass control (or exit) to the previous handler in the chain.
Writing this made me realize we do not need to abort() or fail in case we detect that the fiber stack overflowed. We can just add a WRITE() that just advertises the possibility of overflow and then crash as we normally would with the "missleading" stack trace. That way we don't break (or worry) any of the existing functionality.
Note, that even if we aborted() in the new handler we could use (or write) any tooling that can traverse the ucontext directly (since we have access to it) which might have its use cases (similar to what abseil does when we segfault and we get the stack trace)
Plz hold off the review. I will re-request when ready.
There was a problem hiding this comment.
please manually check these cases: check-fail, segfault, and stack-overflow - before and after the change and make sure stack-traces do not regress for any of these cases.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
util/fibers/epoll_proactor.cc (3)
86-88: Castwrite()result tovoidto silencewarn_unused_result.glibc declares
write()with__attribute__((warn_unused_result)). Ignoring the return value here can produce a-Wunused-resultwarning that becomes a build failure under-Werror.♻️ Suggested tweak
- const char msg[] = "POSSIBLE FIBER STACK OVERFLOW DETECTED\n"; - write(STDERR_FILENO, msg, sizeof(msg) - 1); + static constexpr char kMsg[] = "POSSIBLE FIBER STACK OVERFLOW DETECTED\n"; + (void)write(STDERR_FILENO, kMsg, sizeof(kMsg) - 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` around lines 86 - 88, The call to write() that logs "POSSIBLE FIBER STACK OVERFLOW DETECTED" returns a value marked warn_unused_result; to avoid -Wunused-result warnings cast its return value to void (i.e., use (void)write(...)) so the result is intentionally ignored—modify the write(STDERR_FILENO, msg, sizeof(msg) - 1) invocation associated with msg and STDERR_FILENO accordingly.
237-239: Startup race: overflows on non-zero-index proactors before pool 0 finishesInitbypass this handler.
InstallSigSegvHandlerruns only on thepool_index == 0thread'sInit. Other proactor threads finish their ownInit(includingsigaltstack) independently and can start executing fibers before pool 0 completes; a fiber-stack overflow during that window is delivered to whatever SIGSEGV disposition existed previously (Abseil/default), missing the banner. You'd still get a stack trace from Abseil, so this is cosmetic for diagnostics, but it can be avoided by installing the handler once in any proactor'sInitviaabsl::call_onceinstead of gating onpool_index:♻️ Suggested change
- if (pool_index == 0) { - InstallSigSegvHandler(); - } + static absl::once_flag sigsegv_once; + absl::call_once(sigsegv_once, &InstallSigSegvHandler);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` around lines 237 - 239, The current InstallSigSegvHandler call is gated by pool_index == 0 in Proactor::Init, which allows other proactor threads to finish Init and run fibers before pool 0 installs the SIGSEGV handler; change this to install the handler once unconditionally using a once-guard (e.g., absl::call_once) so InstallSigSegvHandler is executed exactly once from any Proactor::Init regardless of pool_index, ensuring the custom SIGSEGV handler is in place before other proactors start executing fibers.
90-94: One-shot detection after restoration — worth documenting.Restoring
g_old_sigsegv_sais unconditional, so the very first SIGSEGV anywhere in the process (even one unrelated to a fiber stack overflow) permanently un-installsOnSigSegvfor all threads. In practice this is fine because SIGSEGV is terminal under Abseil's failure-signal handler, but if the previous handler ever decides to continue execution (e.g., a tool that swallows the signal, or a test harness that intercepts it), subsequent fiber overflows will silently go undetected. Consider either:
- Only restoring when you're confident the fault is terminal (i.e., leave
OnSigSegvinstalled ifsp >= stack_bottom, and let the fault re-fire throughOnSigSegv→ old handler chain), or- Adding a brief comment on the intentional one-shot behavior so future readers don't mistake it for a bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` around lines 90 - 94, The unconditional sigaction(SIGSEGV, &g_old_sigsegv_sa, nullptr) in OnSigSegv makes this handler one-shot: once any SIGSEGV occurs the old handler is restored and OnSigSegv will no longer catch future fiber overflows; update the code by adding a concise comment next to the sigaction call explaining this intentional one-shot behavior (mention g_old_sigsegv_sa, OnSigSegv, sp and stack_bottom) and why it is safe in your environment, or alternatively change the restore to be conditional (only restore when sp >= stack_bottom) so OnSigSegv remains installed for subsequent overflows — choose one approach and document the rationale clearly inline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 86-88: The call to write() that logs "POSSIBLE FIBER STACK
OVERFLOW DETECTED" returns a value marked warn_unused_result; to avoid
-Wunused-result warnings cast its return value to void (i.e., use
(void)write(...)) so the result is intentionally ignored—modify the
write(STDERR_FILENO, msg, sizeof(msg) - 1) invocation associated with msg and
STDERR_FILENO accordingly.
- Around line 237-239: The current InstallSigSegvHandler call is gated by
pool_index == 0 in Proactor::Init, which allows other proactor threads to finish
Init and run fibers before pool 0 installs the SIGSEGV handler; change this to
install the handler once unconditionally using a once-guard (e.g.,
absl::call_once) so InstallSigSegvHandler is executed exactly once from any
Proactor::Init regardless of pool_index, ensuring the custom SIGSEGV handler is
in place before other proactors start executing fibers.
- Around line 90-94: The unconditional sigaction(SIGSEGV, &g_old_sigsegv_sa,
nullptr) in OnSigSegv makes this handler one-shot: once any SIGSEGV occurs the
old handler is restored and OnSigSegv will no longer catch future fiber
overflows; update the code by adding a concise comment next to the sigaction
call explaining this intentional one-shot behavior (mention g_old_sigsegv_sa,
OnSigSegv, sp and stack_bottom) and why it is safe in your environment, or
alternatively change the restore to be conditional (only restore when sp >=
stack_bottom) so OnSigSegv remains installed for subsequent overflows — choose
one approach and document the rationale clearly inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 567222d4-b74a-4f60-bdfb-b82b532f3377
📒 Files selected for processing (1)
util/fibers/epoll_proactor.cc
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
util/fibers/epoll_proactor.cc (1)
56-57: Extract the platform/sanitizer guard into a single macro.This 2-line expression is duplicated verbatim at lines 216-217. If the set of supported platforms or sanitizer checks ever changes, both sites must be kept in sync. Defining a feature macro once (e.g.
HELIO_FIBER_SIGSEGV_DETECTION) reduces the drift surface and makes the intent self-documenting.♻️ Proposed refactor
+// SIGSEGV-based fiber stack-overflow detection is only supported on Linux/FreeBSD +// where the mcontext_t layout is known, and is disabled under sanitizers (they own +// sigaltstack) and on macOS (incompatible ucontext_t). +#if !defined(__APPLE__) && !defined(__SANITIZE_ADDRESS__) && !defined(__SANITIZE_THREAD__) && \ + !(defined(__has_feature) && (__has_feature(address_sanitizer) || __has_feature(thread_sanitizer))) +#define HELIO_FIBER_SIGSEGV_DETECTION 1 +#else +#define HELIO_FIBER_SIGSEGV_DETECTION 0 +#endif + -// Stack-overflow detection via SIGSEGV is only supported on Linux and FreeBSD where the -// mcontext_t layout is known. Excluded on macOS (incompatible ucontext_t) and under -// sanitizers (they manage their own sigaltstack; our static buffer would corrupt their teardown). -#if !defined(__APPLE__) && !defined(__SANITIZE_ADDRESS__) && !defined(__SANITIZE_THREAD__) && \ - !(defined(__has_feature) && (__has_feature(address_sanitizer) || __has_feature(thread_sanitizer))) +#if HELIO_FIBER_SIGSEGV_DETECTION…and mirror at the
Initcall site:-#if !defined(__APPLE__) && !defined(__SANITIZE_ADDRESS__) && !defined(__SANITIZE_THREAD__) && \ - !(defined(__has_feature) && (__has_feature(address_sanitizer) || __has_feature(thread_sanitizer))) +#if HELIO_FIBER_SIGSEGV_DETECTION🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` around lines 56 - 57, Extract the two identical platform/sanitizer preprocessor conditions into a single named macro (e.g., HELIO_FIBER_SIGSEGV_DETECTION) and replace both duplicated conditionals with that macro; specifically, define the macro once near the top of epoll_proactor.cc using the existing combined expression (the current `#if` !defined(__APPLE__) && !defined(__SANITIZE_ADDRESS__) ... ), then use HELIO_FIBER_SIGSEGV_DETECTION in place of the long `#if` at the site that surrounds the epoll proactor code and at the corresponding Init call site so both checks remain consistent and self-document the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 87-89: The write() call that writes the msg ("POSSIBLE FIBER STACK
OVERFLOW DETECTED\n") ignores its return value which triggers unused-result
warnings; fix by explicitly discarding the return value (cast to void) for the
call to write(STDERR_FILENO, msg, sizeof(msg) - 1) so the compiler knows the
result was intentionally ignored while keeping the existing msg and
STDERR_FILENO usage.
- Around line 91-95: EpollProactor captures the previous SIGSEGV handler into
g_old_sigsegv_sa in InstallSigSegvHandler (called from
EpollProactor::Init(pool_index == 0)) but there is no guard against other
components (e.g., absl::InstallFailureSignalHandler in MainInitGuard) installing
a handler afterwards, which will break overflow detection; update the code by
either (A) adding a runtime assertion in InstallSigSegvHandler that checks the
currently installed SIGSEGV handler matches expectations (or that no
non-default/non-expected handler was installed) and fail-fast with a clear
message if not, or (B) add a clear comment at the EpollProactor::Init call site
documenting that no SIGSEGV handlers must be registered after proactor startup
(mention g_old_sigsegv_sa, InstallSigSegvHandler,
absl::InstallFailureSignalHandler, and MainInitGuard) so future maintainers
don’t introduce ordering regressions.
---
Nitpick comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 56-57: Extract the two identical platform/sanitizer preprocessor
conditions into a single named macro (e.g., HELIO_FIBER_SIGSEGV_DETECTION) and
replace both duplicated conditionals with that macro; specifically, define the
macro once near the top of epoll_proactor.cc using the existing combined
expression (the current `#if` !defined(__APPLE__) &&
!defined(__SANITIZE_ADDRESS__) ... ), then use HELIO_FIBER_SIGSEGV_DETECTION in
place of the long `#if` at the site that surrounds the epoll proactor code and at
the corresponding Init call site so both checks remain consistent and
self-document the intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15bce9cc-d225-4dcd-9f2e-766683b9ed15
📒 Files selected for processing (1)
util/fibers/epoll_proactor.cc
07b5834 to
5477125
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
util/fibers/epoll_proactor.cc (2)
67-67:⚠️ Potential issue | 🟠 MajorInstall the SIGSEGV wrapper exactly once.
sigaction(SIGSEGV, ...)is process-global. If a proactor pool is recreated andpool_index == 0runs again, the second install can saveOnSigSegvintog_old_sigsegv_sa; the handler then restores itself and re-faults instead of reaching Abseil/default crash handling. Wrap the install withabsl::call_onceor equivalent.Proposed install-once guard
+#include <absl/base/call_once.h> `#include` <absl/time/clock.h> @@ static struct sigaction g_old_sigsegv_sa; +absl::once_flag g_sigsegv_handler_once; @@ if (pool_index == 0) { - InstallSigSegvHandler(); + absl::call_once(g_sigsegv_handler_once, InstallSigSegvHandler); }#!/bin/bash # Verify existing Abseil call_once usage/import conventions in this repository. rg -n -C2 --type=cc --type=h '\babsl::call_once\b|\babsl::once_flag\b|absl/base/call_once\.h'Also applies to: 242-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` at line 67, The SIGSEGV handler install is currently done unguarded (saving previous handler into g_old_sigsegv_sa) and can be invoked multiple times; change the install to run exactly once using absl::call_once with a static absl::once_flag (or equivalent) so sigaction(SIGSEGV, ...) that sets OnSigSegv and stores g_old_sigsegv_sa is executed only once (protect the block that calls sigaction and assigns g_old_sigsegv_sa), leaving the rest of the proactor logic (including pool_index checks and the OnSigSegv handler) unchanged.
96-97:⚠️ Potential issue | 🟡 MinorDiscard the
write()result explicitly.Fortified libc headers can mark
write()as warn-unused-result; assign the return value and intentionally discard it. This also avoids copying the message onto the alternate stack.Proposed signal-safe cleanup
- const char msg[] = "POSSIBLE FIBER STACK OVERFLOW DETECTED\n"; - write(STDERR_FILENO, msg, sizeof(msg) - 1); + static constexpr char msg[] = "POSSIBLE FIBER STACK OVERFLOW DETECTED\n"; + ssize_t res = write(STDERR_FILENO, msg, sizeof(msg) - 1); + (void)res;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` around lines 96 - 97, The write() call that prints the overflow message must have its return value explicitly captured and discarded to satisfy warn-unused-result and avoid fortify warnings; update the call that writes the msg constant (the write(STDERR_FILENO, msg, sizeof(msg) - 1) invocation) to store the result into a local variable (e.g., a volatile or otherwise clearly-unused ssize_t) and then ignore it, ensuring the code remains signal-safe and does not rely on implicit discard.
🧹 Nitpick comments (1)
util/fibers/epoll_proactor.cc (1)
13-21: Either broaden sanitizer detection or say ASAN-only.The guard/comment says this is disabled under “sanitizers”, but the macro only detects AddressSanitizer. If TSAN/MSAN builds are supported, they will still use this static 64KB alt stack; Abseil’s failure handler treats ASAN/MSAN/TSAN as needing larger alternate stacks. Broaden the macro or narrow the comment/guard to ASAN-only.
Possible direction if TSAN/MSAN are supported
+#ifndef __has_feature +#define __has_feature(x) 0 +#endif + -// Portable ASAN detection: Clang exposes __has_feature; GCC defines __SANITIZE_ADDRESS__. -#if defined(__has_feature) -# if __has_feature(address_sanitizer) -# define HELIO_ASAN 1 -# endif -#endif -#if !defined(HELIO_ASAN) && defined(__SANITIZE_ADDRESS__) -# define HELIO_ASAN 1 +// Portable sanitizer detection for builds where sanitizer runtimes manage signal stacks. +#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__) || \ + __has_feature(thread_sanitizer) || defined(__SANITIZE_THREAD__) || \ + __has_feature(memory_sanitizer) +#define HELIO_SANITIZER 1 `#endif` @@ -#if !defined(__APPLE__) && !defined(HELIO_ASAN) +#if !defined(__APPLE__) && !defined(HELIO_SANITIZER)References used for this check: Abseil documents
use_alternate_stack = trueinFailureSignalHandlerOptionsand its implementation grows the alt stack under ASAN/MSAN/TSAN: https://chromium.googlesource.com/external/github.com/abseil/abseil-cpp/+/HEAD/absl/debugging/failure_signal_handler.h and https://chromium.googlesource.com/external/github.com/abseil/abseil-cpp/+/HEAD/absl/debugging/failure_signal_handler.cc.Also applies to: 63-66, 225-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/fibers/epoll_proactor.cc` around lines 13 - 21, The sanitizer guard currently only detects AddressSanitizer via HELIO_ASAN, but the comment implies broader "sanitizers" coverage; either broaden detection to also detect ThreadSanitizer and MemorySanitizer (e.g., define HELIO_TSAN/HELIO_MSAN or a single HELIO_SANITIZER when __has_feature(thread_sanitizer)/__has_feature(memory_sanitizer) or __SANITIZE_THREAD__/__SANITIZE_MEMORY__ are present) and use that to gate the alt-stack logic, or change the comment/guard to explicitly state ASAN-only and leave HELIO_ASAN as-is; update occurrences of HELIO_ASAN in the file (and the related checks at the other noted locations) to match the chosen approach so Abseil-like handlers get the correct behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 78-80: Add the GCC no-instrument attribute to the FiberActive
function so the signal handler call chain remains uninstrumented: modify the
definition of detail::FiberActive() to include
__attribute__((no_instrument_function)) (i.e., mark the FiberActive function
with no_instrument_function) so that when OnSigSegv() calls
detail::FiberActive() it will not get instrumented by -finstrument-functions.
---
Duplicate comments:
In `@util/fibers/epoll_proactor.cc`:
- Line 67: The SIGSEGV handler install is currently done unguarded (saving
previous handler into g_old_sigsegv_sa) and can be invoked multiple times;
change the install to run exactly once using absl::call_once with a static
absl::once_flag (or equivalent) so sigaction(SIGSEGV, ...) that sets OnSigSegv
and stores g_old_sigsegv_sa is executed only once (protect the block that calls
sigaction and assigns g_old_sigsegv_sa), leaving the rest of the proactor logic
(including pool_index checks and the OnSigSegv handler) unchanged.
- Around line 96-97: The write() call that prints the overflow message must have
its return value explicitly captured and discarded to satisfy warn-unused-result
and avoid fortify warnings; update the call that writes the msg constant (the
write(STDERR_FILENO, msg, sizeof(msg) - 1) invocation) to store the result into
a local variable (e.g., a volatile or otherwise clearly-unused ssize_t) and then
ignore it, ensuring the code remains signal-safe and does not rely on implicit
discard.
---
Nitpick comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 13-21: The sanitizer guard currently only detects AddressSanitizer
via HELIO_ASAN, but the comment implies broader "sanitizers" coverage; either
broaden detection to also detect ThreadSanitizer and MemorySanitizer (e.g.,
define HELIO_TSAN/HELIO_MSAN or a single HELIO_SANITIZER when
__has_feature(thread_sanitizer)/__has_feature(memory_sanitizer) or
__SANITIZE_THREAD__/__SANITIZE_MEMORY__ are present) and use that to gate the
alt-stack logic, or change the comment/guard to explicitly state ASAN-only and
leave HELIO_ASAN as-is; update occurrences of HELIO_ASAN in the file (and the
related checks at the other noted locations) to match the chosen approach so
Abseil-like handlers get the correct behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 042b5935-b7aa-4dde-8d7e-32555cd5f687
📒 Files selected for processing (1)
util/fibers/epoll_proactor.cc
2 participants
This PR adds two safeguards:
sigaltstack + SIGSEGV handler (epoll_proactor.cc): each proactor thread registers a 64KB alternate signal stack and a SA_ONSTACK | SA_SIGINFO handler. When a stack overflow triggers a fault, the handler reads the interrupted SP from ucontext_t, compares it against the active fiber's stack_bottom_, and if it has overflowed writes a clear message to stderr and aborts _ before the corrupted heap produces a misleading crash.
InitStackBottom for the dispatcher fiber (scheduler.cc): the dispatcher was the only fiber that never called InitStackBottom, leaving stack_bottom_ null and making overflow detection impossible for it. Now it paints canaries and records its stack bounds the same way worker fibers do.
This won't catch every case _ by the time SIGSEGV fires the heap may already be partially corrupted _ but it reliably produces an actionable message instead of a 285-frame backtrace ending in for example
std::string::_M_disposelike seen in: dragonflydb/dragonfly#3464Summary by CodeRabbit