Issue about `sentry__enter_signal_handler`

[craftoq]

Description
The implementation of sentry__enter_signal_handler in sentry_sync.c has potential issues related to atomicity and race conditions.

The function is implemented as:

void sentry__enter_signal_handler(void)
{
    sentry__block_for_signal_handler();
    g_signal_handling_thread = sentry__current_thread();
    __sync_fetch_and_or(&g_in_signal_handler, 1);
}

Issues:

1. Non-atomic entry:
   The check-and-set for entering the signal handler is not atomic. After sentry__block_for_signal_handler() returns, another thread may also reach this line and set the flags, causing multiple threads to believe they are in the handler.

2. Return value ignored:
   The return value of sentry__block_for_signal_handler() is not checked. If the current thread is already the signal-handling thread, the function should probably return early, but it continues, possibly overwriting g_signal_handling_thread and setting the flag again.

3. Split flag and thread ID:
   Setting g_signal_handling_thread and g_in_signal_handler happens in two separate steps, which can lead to inconsistent states in a multithreaded or signal-driven environment.

[linear]

NATIVE-117 Issue about `sentry__enter_signal_handler`

[supervacuus]

Hi @craftoq, thanks for the report.

The check-and-set for entering the signal handler is not atomic. After sentry__block_for_signal_handler() returns, another thread may also reach this line and set the flags, causing multiple threads to believe they are in the handler.

That’s correct. There are a couple of issues here. They mostly stem from the fact that the original implementation tried to compose the spin loop and the acquire from two separate functions (because the spin loop is also reused in other places, e.g., to block entry into critical sections while a signal is processed).

With this split, we don’t actually know which thread “won” the race. In addition, the current spin loop performs an RMW (fetch_and_add(…, 0)), which unnecessarily dirties the cache line on every poll; a read-only fetch would be better.

I will fix these issues.

Did you observe a concrete situation where this led to concurrent signal handling?

The return value of sentry__block_for_signal_handler() is not checked. If the current thread is already the signal-handling thread, the function should probably return early, but it continues, possibly overwriting g_signal_handling_thread and setting the flag again.

That is intentional, although not consistently successful. The guard is meant to serve two purposes:

• other threads that raise a signal (we only register terminal signals) while we already process a signal should block indefinitely, we only want to record the first crash (and also can't represent multiple crashed threads in the corresponding payload in the Sentry API).
• We do, however, allow re-entrancy for the same thread, because that would mean the signal handler itself crashed (with a different signal), and this sometimes leads to helpful reports that we or users can act on.

Setting g_signal_handling_thread and g_in_signal_handler happens in two separate steps, which can lead to inconsistent states in a multithreaded or signal-driven environment.

Yes, and that’s related to the first point: splitting the spin-wait and the acquire created an inconsistent sequence. The upcoming change will make the guard a single atomic acquire, after which the owner thread is published in a way that allows waiters to read it consistently.