fix(crash): replace async-signal-unsafe TLS with pthread-specifics#8271
fix(crash): replace async-signal-unsafe TLS with pthread-specifics#8271jpnurmi wants to merge 7 commits into
Conversation
Accessing _Thread_local storage from a signal handler is async-signal-unsafe and undefined behavior. Replace it with a global tid-keyed circular buffer so managed runtimes can still suppress crash reports for signals raised on any thread. Use atomic_fetch_add_explicit for thread-safe index assignment.
jpnurmi
requested review from
NinjaLikesCheez,
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Fixes
- replace async-signal-unsafe TLS with pthread-specifics ([#8271](https://github.com/getsentry/sentry-cocoa/pull/8271))If none of the above apply, you can opt out of this check by adding |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
The ring buffer entry for sentrycrashcm_signal_ignore_next was only cleared when the signum matched the raised signal. This left stale entries when a different signal arrived on the same thread, breaking the documented contract that the ignore is consumed on the next delivery regardless of signum. Now all entries for the current thread are cleared on any signal delivery, but the crash report is only suppressed when the signum also matches.
tid and signum were written and read as separate non-atomic stores, allowing handleSignal to observe a partially-written slot (e.g. new tid with stale signum), causing the ignore entry to be missed or misapplied. Fix by making both fields _Atomic with release/acquire ordering: writer stores signum first (release), then tid (release); reader loads tid first (acquire) to synchronize, guaranteeing a consistent signum when tid matches.
Replace pthread_threadid_np + separate volatile fields with a single _Atomic uint64_t per slot encoding (mach_port << 32 | signum). This eliminates the data race from tid/signum being written as separate stores and replaces an async-signal-unsafe call with pthread_mach_thread_np.
The single _Atomic uint64_t encoding (mach_port << 32 | signum) uses mach_port_t, but Mach thread ports are recycled by the kernel after thread exit. This can cause false ignore matches on a later thread with a recycled port. Switch to pthread_threadid_np (unique per thread lifetime, never recycled) and pack both fields in a _Atomic 16-byte struct. On arm64 and x86_64 Apple targets this compiles to a single lock-free atomic load/store, so the pair is always read and written atomically. pthread_threadid_np is not formally async-signal-safe per POSIX, but on Darwin it reads from the pthread struct without allocation or locking, matching the accepted trade-off for pthread_self() already used in signal handlers throughout SentryCrash.
supervacuus
left a comment
supervacuus
left a comment
There was a problem hiding this comment.
While the solution addresses atomic updates to a thread-id/signum slot via 16-byte atomics, it is limited to x86_64 and arm64. This might be fine for most targets; however, there are at least watches (the Cocoa SDK supports watchOS 8) within the supported range that could be running on older ARM. I am not sure how this translates to downstream usage; just meant as a fair warning.
The bigger point here is that atomicity in this PR is only considered for in-slot writes. However, slot publishing and associated races are currently not considered.
Two examples:
The current handler consume looks abstractly like this:
entry = atomic_load(&slot);
if (entry.tid == currentTid) {
atomic_store(&slot, zero);
}In this approach the 8 slots must be just for safety, because if you ever have situation where two threads reach the same slot (one in the handler, one in ignore_next), the current code allows for accidental overwrites:
- Slot
icontains {thread=A, signum=SIGSEGV} - Thread A receives a SIGBUS
- Handler on A loads slot
i. It sees thread=A but signum does not match, but it will still consume/clear the slot - Before A stores zero, thread B calls ingore_next(SIGSEGV)
- the ringbuffer also lands on Slot
iand stores {thread=B, signum=SIGSEGV} - Handler on A resumes and stores zero.
Nothing in the current code prevents this. If you say that this will never happen because both threads and singals raised are bounded enough, that is a fair assumption, but that should be documented.
Another example would be with multiple pending entries, where a stale entry can suppress a real crash because this code treats any matching entry for the thread as enough to ignore:
- Thread A calls
ignore_next(SIGSEGV) - No signal arrives and the entry becomes stale
- Later, thread A calls
ignore_next(SIGBUS) - Now two entries may exist for A
- old {A, SIGSEGV}
- new {A, SIGBUS}
- A real native SIGSEGV happens on A
The handler sees the old {A, SIGSEGV} and ignores the crash. That would be a false negative or unreported crash.
Again it is fair to assume that in your case a thread that calls ingore_next() will always receive a signal. And then this is a non-issue. But again, i would add this to an invariant list, rather than just expose a public function.
I like the simplicity of the current approach but without concrete boundaries it is short of underspecfying the problem. That can be the right choice and keeping things simple has merit. But it could also be that you need a protocol to claim slots explicitly and maybe even offer generational bounds so that if you have multiple entries for a single thread, you know which one is the youngest.
Use pthread-specific data outside the signal handler to cache one ignore entry per thread, and publish those entries through an append-only atomic list. This keeps allocation and pthread-specific access outside the signal handler for async-signal safety while preserving latest-call-wins behavior per thread.
jpnurmi
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ed65745. Configure here.
Publish a new per-thread ignore entry before storing its pending signum. This makes the signum store the point where the ignore becomes active, so the signal handler can find the entry once the request is visible.
2 participants
📜 Description
Use pthread-specific data outside the signal handler to cache one ignore entry per thread, then publish those entries through an append-only atomic list. The signal handler scans the published entries and atomically consumes the pending signum.
💡 Motivation and Context
Accessing lazily allocated thread-local storage from a signal handler is async-signal-unsafe. This keeps allocation and pthread-specific access outside the signal handler while preserving the previous latest-ignore-wins behavior per thread.
Close: #8134
💚 How did you test it?
With sentry-dotnet/integration-test/ios.Tests.ps1:
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.