Repository navigation
fix(Workspace): fix EXC_BAD_ACCESS caused by over-releasing ghostty font - #1496
Merged
Merged
Conversation
The ghostty_surface_quicklook_font function returns an unretained pointer to a font object. Using takeRetainedValue() transferred non-existent ownership to ARC, leading to an over-release and an eventual EXC_BAD_ACCESS (SIGSEGV) crash when creating new surfaces (like cmd+t or cmd+d) on certain systems such as Intel Macs. Replaced takeRetainedValue() with takeUnretainedValue() to correctly manage memory.
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughA single-line memory management change in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
21-30: Fix correctly uses unretained pointer semantics.The change from
takeRetainedValue()totakeUnretainedValue()is correct. The C functionghostty_surface_quicklook_font()returns a borrowed (unretained) reference that the Ghostty library retains ownership of, not the Swift caller. UsingtakeUnretainedValue()prevents ARC from incorrectly releasing the font. The short scope ofctFont—used only forCTFontGetSize()on line 27—ensures the borrowed reference remains valid.Consider documenting the ownership semantics to prevent future regressions:
+ // ghostty_surface_quicklook_font returns a borrowed (unretained) pointer; + // use takeUnretainedValue so ARC does not release the font. let ctFont = Unmanaged<CTFont>.fromOpaque(quicklookFont).takeUnretainedValue()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 21 - 30, The change in cmuxCurrentSurfaceFontSizePoints should keep using Unmanaged<CTFont>.fromOpaque(...).takeUnretainedValue() because ghostty_surface_quicklook_font() returns a borrowed (unretained) reference; ensure the code does not call takeRetainedValue(). Add a brief inline comment above the Unmanaged call explaining that ghostty_surface_quicklook_font() returns an unretained/borrrowed CTFont owned by Ghostty so ARC must not release it, and consider adding a short doc comment on cmuxCurrentSurfaceFontSizePoints to record the ownership semantics to prevent future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 21-30: The change in cmuxCurrentSurfaceFontSizePoints should keep
using Unmanaged<CTFont>.fromOpaque(...).takeUnretainedValue() because
ghostty_surface_quicklook_font() returns a borrowed (unretained) reference;
ensure the code does not call takeRetainedValue(). Add a brief inline comment
above the Unmanaged call explaining that ghostty_surface_quicklook_font()
returns an unretained/borrrowed CTFont owned by Ghostty so ARC must not release
it, and consider adding a short doc comment on cmuxCurrentSurfaceFontSizePoints
to record the ownership semantics to prevent future regressions.
Closed
1 of 2 tasks
|
Thank you mate!!! |
elvistranhere
added a commit
to elvistranhere/cmux
that referenced
this pull request
Mar 21, 2026
…le font pointer ghostty_surface_quicklook_font returns an unretained CTFont pointer that can become stale on Intel Macs, leading to EXC_BAD_ACCESS (SIGSEGV) when creating a split. This is a follow-up to the same crash pattern fixed in manaflow-ai#1496. Add malloc_size validation in cmuxCurrentSurfaceFontSizePoints to detect freed heap allocations before interpreting the pointer as a CTFont. Also add hasLiveSurface guards in inheritedTerminalConfig and rememberTerminalConfigInheritanceSource to skip surfaces whose native state is closing or closed. All callers already handle nil gracefully by falling back to inherited config values.
3 tasks
This was referenced Mar 23, 2026
4 of 6 tasks
This was referenced Mar 26, 2026
kokagex
added a commit
to kokagex/cmux
that referenced
this pull request
Mar 28, 2026
…ed config ghostty_surface_inherited_config() returns a config struct containing raw C pointers (env_vars, working_directory, command) owned by the source surface. On Intel x86_64, freed memory is recycled more aggressively than ARM64, causing EXC_BAD_ACCESS when the new pane dereferences these stale pointers during createSurface(). Three-layer fix: - cmuxInheritedSurfaceConfig() now rebuilds a clean config with only font_size, matching the pattern already used in TabManager.workspaceCreationConfigTemplate() - Added cmuxSurfacePointerAppearsLive() guard before ghostty_surface_inherited_config() - Added malloc_size validation for env_vars pointer in createSurface() as defense-in-depth Fixes manaflow-ai#1496, manaflow-ai#1870 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…ont (manaflow-ai#1496) The ghostty_surface_quicklook_font function returns an unretained pointer to a font object. Using takeRetainedValue() transferred non-existent ownership to ARC, leading to an over-release and an eventual EXC_BAD_ACCESS (SIGSEGV) crash when creating new surfaces (like cmd+t or cmd+d) on certain systems such as Intel Macs. Replaced takeRetainedValue() with takeUnretainedValue() to correctly manage memory. Co-authored-by: LeonLeung <leonleung.tech@gmail.com>
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…le font pointer ghostty_surface_quicklook_font returns an unretained CTFont pointer that can become stale on Intel Macs, leading to EXC_BAD_ACCESS (SIGSEGV) when creating a split. This is a follow-up to the same crash pattern fixed in manaflow-ai#1496. Add malloc_size validation in cmuxCurrentSurfaceFontSizePoints to detect freed heap allocations before interpreting the pointer as a CTFont. Also add hasLiveSurface guards in inheritedTerminalConfig and rememberTerminalConfigInheritanceSource to skip surfaces whose native state is closing or closed. All callers already handle nil gracefully by falling back to inherited config values.
6 tasks
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
3 participants
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 #1452.
Root Cause Analysis
According to issue #1452, cmux consistently crashes with
EXC_BAD_ACCESS (SIGSEGV)on Intel Macs when creating a new tab (Cmd + T) or splitting a pane (Cmd + D).The issue stems from the
cmuxCurrentSurfaceFontSizePointsfunction:The
ghostty_surface_quicklook_font()C API returns an unretained inner pointer (Get semantics). Forcing.takeRetainedValue()incorrectly transfers ownership of this pointer to Swift's ARC. When the variable goes out of scope, ARC attempts to release it, causing an over-release of the underlying font object, which leads to a segmentation fault on certain architectures (like Intel).Fix
Replaced
takeRetainedValue()withtakeUnretainedValue()to prevent ARC from consuming the memory reference, fixing the over-release crashSummary by cubic
Prevent EXC_BAD_ACCESS on Intel Macs when creating a tab (Cmd+T) or splitting a pane (Cmd+D) by fixing CoreText font ownership in
cmux. Fixes #1452..takeRetainedValue()with.takeUnretainedValue()when converting the pointer fromghostty_surface_quicklook_font()toCTFont(Get semantics) to avoid ARC over-release and the crash.Written for commit b3bac4f. Summary will update on new commits.
Summary by CodeRabbit
No user-facing changes in this release. This update includes internal optimizations to memory management that improve code reliability without affecting application functionality or user experience.