Fix sentry-init racing environ mutation during global init - #174
Open
azooz2003-bit wants to merge 1 commit into
Open
Fix sentry-init racing environ mutation during global init#174azooz2003-bit wants to merge 1 commit into
azooz2003-bit wants to merge 1 commit into
Conversation
Three iOS crashes on 2026-07-30 (cmux INTERNAL builds 20260730090940 and
20260730213932) shared one interleaving: the main thread was inside
ghostty_init -> ensureLocale -> setlocale during the first terminal-surface
mount, and the sentry-init thread died on a byte-read SIGSEGV walking the
environ snapshot. ensureLocale's setenv("LANG"/"LANGUAGE") can realloc the
environ pointer array, freeing the block the ghostty_init snapshot points
at; syncEnviron() only re-syncs after ensureLocale returns, and crash.init
had already spawned sentry-init, which read the stale snapshot through
global.environMap() with no concurrency control.
Two changes close the race:
- global.init now runs ensureLocale() + syncEnviron() BEFORE crash.init,
so no other thread exists while the environment is mutated.
- sentry.init resolves the Sentry cache directory on the spawning thread
from an explicit environ map and passes it into initThread; the spawned
thread no longer touches the shared environ snapshot at all, so any
later setenv on another thread cannot revive this crash class.
The regression test pins the new seam: cache-dir resolution must consume
the caller-provided map (a value absent from the live process
environment), so it fails if the resolution ever reaches back into global
process state. A second test pins the preserved macOS behavior (empty or
unset XDG_CACHE_HOME falls back to NSCachesDirectory).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughSentry cache resolution now occurs on the spawning thread using a caller-provided environment map. Global initialization synchronizes locale and environment state before crash reporting initialization, and targeted tests cover environment isolation and macOS fallback behavior. ChangesSentry environment initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crash/sentry.zig`:
- Around line 365-382: Extend the macOS cache-directory tests around
resolveCacheDir with a separate case whose environ_map contains no
XDG_CACHE_HOME entry. Verify it also resolves to the NSCachesDirectory-based
path ending in “sentry”, while keeping the existing empty-value case intact.
🪄 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 Plus
Run ID: c271c41f-8267-4420-8d74-c716453550ca
📒 Files selected for processing (2)
src/crash/sentry.zigsrc/global.zig
Comment on lines
+365
to
+382
| test "sentry cache dir prefers NSCachesDirectory on macOS without XDG" { | ||
| // Pins the behavior the refactor preserved from the old thread-side | ||
| // code: on macOS an unset OR empty XDG_CACHE_HOME falls back to the | ||
| // NSCachesDirectory-based path instead of the XDG path. | ||
| if (comptime builtin.os.tag != .macos) return error.SkipZigTest; | ||
|
|
||
| const testing = std.testing; | ||
| const alloc = testing.allocator; | ||
|
|
||
| var environ_map = try testing.environ.createMap(alloc); | ||
| defer environ_map.deinit(); | ||
| try environ_map.put("XDG_CACHE_HOME", ""); | ||
|
|
||
| const cache_dir = try resolveCacheDir(alloc, &environ_map); | ||
| defer alloc.free(cache_dir); | ||
|
|
||
| try testing.expect(std.mem.indexOf(u8, cache_dir, "Caches") != null); | ||
| try testing.expect(std.mem.endsWith(u8, cache_dir, "sentry")); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the unset XDG_CACHE_HOME fallback separately.
This test sets XDG_CACHE_HOME to an empty value, but never exercises the missing-key branch of environ_map.get(...) orelse "". Add a macOS case where the supplied map has no XDG_CACHE_HOME entry, matching the stated unset-or-empty contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/crash/sentry.zig` around lines 365 - 382, Extend the macOS
cache-directory tests around resolveCacheDir with a separate case whose
environ_map contains no XDG_CACHE_HOME entry. Verify it also resolves to the
NSCachesDirectory-based path ending in “sentry”, while keeping the existing
empty-value case intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the iOS crash that hit cmux INTERNAL three times on 2026-07-30 (builds
20260730090940and20260730213932, twice back-to-back): EXC_BAD_ACCESS byte read on the sentry-init thread while the main thread was insideghostty_init -> ensureLocale -> setlocaleduring the first terminal-surface mount of a session.Mechanism:
ghostty_initsnapshots the raw Cenvironpointer array.global.initspawned the sentry-init thread (crash.init) BEFOREensureLocale(), whosesetenv("LANG"/"LANGUAGE")can realloc the environ array and free the snapshotted block;syncEnviron()re-syncs only after. On iOS (non-macOS branch), sentry-init then walked the freed snapshot viaglobal.environMap()(src/crash/sentry.zig), which has no concurrency control, and byte-scanned a garbage entry pointer (near-null0x2fb1in one crash, wild pointers in the other two, matching freed-block reuse).Fix, two layers:
global.initrunsensureLocale()+syncEnviron()beforecrash.init, so no other thread exists while init mutates the environment.sentry.initresolves the Sentry cache directory on the spawning thread from an explicitEnviron.Mapand passes it intoinitThread; the spawned thread no longer touches the shared environ snapshot at all, so this crash class stays dead even if some later code callssetenvafter startup.The regression test pins the seam: cache-dir resolution must consume the caller-provided map (whose test value does not exist in the live process environment), so it fails if resolution ever reaches back into global process state; a mutation check confirmed the test executes and fails when broken (
73 pass, 1 fail). A second macOS-gated test pins the preserved NSCachesDirectory fallback for unset/emptyXDG_CACHE_HOME. A red/green two-commit structure is not applicable here: the defect is a data race whose pre-fix failure mode is undefined behavior, and the testable seam does not exist pre-fix.Verified with
zig build test -Dtest-filter="sentry cache dir"on zig 0.16.0 (86/86 steps, 74/74 tests).Full triage with the crash evidence and most-likely-cause ranking lives in cmuxterm-hq
out/crash-20260730/triage.md; the cmux submodule bump PR follows and must merge only after this lands on main.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes an iOS startup crash caused by the
sentry-initthread reading a freedenvironsnapshot during locale setup. We now finish locale/env setup before spawning Sentry and avoid reading the global env from the Sentry thread.ensureLocale()+syncEnviron()beforecrash.initso no thread reads the environment while it’s being mutated.Environ.Map, pass it intoinitThread, and stop touchingglobal.environin the spawned thread.XDG_CACHE_HOMEis unset or empty.Written for commit abcf569. Summary will update on new commits.