Fix Cmd+N crash from workspace creation config snapshots - #2178
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughTabManager's workspace-creation field replaces a pointer-backed terminal config snapshot with a simple inherited font-size Float value. New methods extract font size from inherited config and construct sanitized config templates for new workspaces. Tests verify that the sanitized template preserves font size while stripping other inherited values. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a use-after-free crash in the Cmd+N workspace creation path by stopping
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User (Cmd+N)
participant TM as TabManager
participant Snap as WorkspaceCreationSnapshot
participant Surface as ghostty surface (may teardown)
participant Config as workspaceCreationConfigTemplate
User->>TM: addWorkspace()
TM->>TM: workspaceCreationSnapshot()
TM->>Surface: inheritedTerminalConfigForNewWorkspace()
Surface-->>TM: ghostty_surface_config_s (with raw pointers)
TM->>TM: inheritedTerminalFontPointsForNewWorkspace()<br/>extracts font_size Float only
TM->>Snap: store inheritedTerminalFontPoints: Float?<br/>(NO raw pointers retained)
Note over Surface: Surface may teardown here —<br/>safe because no pointer was stored
TM->>Config: workspaceCreationConfigTemplate(inheritedTerminalFontPoints:)
Config->>Config: ghostty_surface_config_new()<br/>config.font_size = inheritedTerminalFontPoints
Config-->>TM: clean ghostty_surface_config_s (pointer-free)
TM->>TM: makeWorkspaceForCreation(configTemplate: clean config)
Reviews (1): Last reviewed commit: "Sanitize workspace creation config snaps..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func inheritedTerminalConfigForNewWorkspace( | ||
| workspace: Workspace? | ||
| ) -> ghostty_surface_config_s? { |
There was a problem hiding this comment.
private widened to internal for test subclassing
inheritedTerminalConfigForNewWorkspace(workspace:) is now internal (no explicit access modifier) so UnsafeConfigSnapshotTabManager in the test target can override it. This is a pragmatic and functional approach given the test harness, but it permanently exposes this method to all callers within the module, not just the test.
An alternative that keeps the method private and avoids a permanent API surface increase is injecting the config source as a closure or a small protocol:
// In TabManager, keep private:
private var inheritedConfigProvider: (Workspace?) -> ghostty_surface_config_s? = { [weak self] in
self?.inheritedTerminalConfigForNewWorkspace(workspace: $0)
}The test then just swaps out manager.inheritedConfigProvider instead of subclassing. Not a blocker — the current approach is correct and testable — but worth considering if the codebase discourages widening private to internal solely for tests.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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
🧹 Nitpick comments (1)
cmuxTests/WorkspaceUnitTests.swift (1)
522-553: Exercise the post-snapshot teardown window in this regression.The injected C buffers stay alive until
UnsafeConfigSnapshotTabManagerdeinit, so this only proves the finalconfigTemplateis sanitized. It will still pass ifWorkspaceCreationSnapshotstarts retaining pointer-backed data again and only strips it later. Consider invalidating/freeing the injected config right afterdidCaptureWorkspaceCreationSnapshot()and then assertingaddWorkspace()still survives that path.Also applies to: 581-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 522 - 553, The test currently leaves the C-backed buffers live until UnsafeConfigSnapshotTabManager deinit, so it only verifies final config sanitization; modify the test to free/invalidate the injected config immediately after calling didCaptureWorkspaceCreationSnapshot() for the WorkspaceCreationSnapshot path: call the existing teardown logic (deallocate retainedEnvVars, free retainedCStringPointers and null out injectedConfig/its pointers) right after didCaptureWorkspaceCreationSnapshot() and then assert that addWorkspace() still succeeds; focus changes around installInjectedConfig, didCaptureWorkspaceCreationSnapshot(), and addWorkspace() to ensure the snapshot consumer does not retain pointer-backed data beyond that call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TabManager.swift`:
- Around line 2286-2298: The function workspaceCreationConfigTemplate creates a
ghostty_surface_config_s via ghostty_surface_config_new() but currently only
sets font_size, leaving other pointer fields uninitialized; before returning
from workspaceCreationConfigTemplate, explicitly zero or nil out all
pointer/reference members of the ghostty_surface_config_s (the fields that could
hold C pointers) so the struct contains no indeterminate pointer values when
later passed to ghostty_surface_new(); update workspaceCreationConfigTemplate to
set those pointer fields to nil/0 (or otherwise clear them) after calling
ghostty_surface_config_new() and before returning the config.
---
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 522-553: The test currently leaves the C-backed buffers live until
UnsafeConfigSnapshotTabManager deinit, so it only verifies final config
sanitization; modify the test to free/invalidate the injected config immediately
after calling didCaptureWorkspaceCreationSnapshot() for the
WorkspaceCreationSnapshot path: call the existing teardown logic (deallocate
retainedEnvVars, free retainedCStringPointers and null out injectedConfig/its
pointers) right after didCaptureWorkspaceCreationSnapshot() and then assert that
addWorkspace() still succeeds; focus changes around installInjectedConfig,
didCaptureWorkspaceCreationSnapshot(), and addWorkspace() to ensure the snapshot
consumer does not retain pointer-backed data beyond that call.
🪄 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: bcf76334-8a37-4d99-b688-72875778d586
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/WorkspaceUnitTests.swift
| private func workspaceCreationConfigTemplate( | ||
| inheritedTerminalFontPoints: Float? | ||
| ) -> ghostty_surface_config_s? { | ||
| guard let inheritedTerminalFontPoints, inheritedTerminalFontPoints > 0 else { | ||
| return nil | ||
| } | ||
| // ghostty_surface_config_s can carry raw C pointers owned by the source surface. | ||
| // New workspace creation only needs the inherited zoom level, so rebuild a clean | ||
| // config instead of snapshotting pointer-backed fields across workspace creation. | ||
| var config = ghostty_surface_config_new() | ||
| config.font_size = inheritedTerminalFontPoints | ||
| return config | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if TabManager.swift exists and read the specified lines
if [ -f "Sources/TabManager.swift" ]; then
wc -l Sources/TabManager.swift
echo "---"
sed -n '2276,2298p' Sources/TabManager.swift
else
# Try to locate TabManager.swift
find . -name "TabManager.swift" -type f
fiRepository: manaflow-ai/cmux
Length of output: 1086
🏁 Script executed:
# Search for ghostty_surface_config definition and usage
rg "ghostty_surface_config" -A 3 -B 1 --max-count 20Repository: manaflow-ai/cmux
Length of output: 9223
🏁 Script executed:
# Look for the Ghostty header file in the repo
find . -name "ghostty.h" -o -name "*ghostty*.h" | head -20Repository: manaflow-ai/cmux
Length of output: 71
🏁 Script executed:
# Read the ghostty.h header to understand the struct and constructor
cat -n ghostty.h | grep -A 40 "ghostty_surface_config_s" | head -60Repository: manaflow-ai/cmux
Length of output: 2266
🏁 Script executed:
# Look for the implementation or documentation of ghostty_surface_config_new()
# Check if there's a build directory or any implementation hints
rg "ghostty_surface_config_new" -B 3 -A 10Repository: manaflow-ai/cmux
Length of output: 6034
🏁 Script executed:
# Check how the config is actually used across the codebase to understand the risk
rg "ghostty_surface_config_new\(\)" -A 5 | head -80Repository: manaflow-ai/cmux
Length of output: 2722
🏁 Script executed:
# Search for any comments or documentation about field initialization
rg "zero.init|zero-init|zero.initialize" -i -C 2Repository: manaflow-ai/cmux
Length of output: 1237
🏁 Script executed:
# Get the exact struct definition to see all pointer fields
sed -n '400,453p' ghostty.h | cat -nRepository: manaflow-ai/cmux
Length of output: 1575
Explicitly clear uninitialized pointer fields before returning.
The Ghostty header doesn't document that ghostty_surface_config_new() zero-initializes all members, and C struct constructors across FFI boundaries don't guarantee initialization. Since this config is passed to ghostty_surface_new(), uninitialized pointer values could introduce undefined behavior. The test code (WorkspaceUnitTests.swift) explicitly sets all pointer fields after construction, establishing the expected pattern. Clear the fields you don't use.
Suggested hardening
var config = ghostty_surface_config_new()
+ config.working_directory = nil
+ config.command = nil
+ config.env_vars = nil
+ config.env_var_count = 0
+ config.initial_input = nil
config.font_size = inheritedTerminalFontPoints
return config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 2286 - 2298, The function
workspaceCreationConfigTemplate creates a ghostty_surface_config_s via
ghostty_surface_config_new() but currently only sets font_size, leaving other
pointer fields uninitialized; before returning from
workspaceCreationConfigTemplate, explicitly zero or nil out all
pointer/reference members of the ghostty_surface_config_s (the fields that could
hold C pointers) so the struct contains no indeterminate pointer values when
later passed to ghostty_surface_new(); update workspaceCreationConfigTemplate to
set those pointer fields to nil/0 (or otherwise clear them) after calling
ghostty_surface_config_new() and before returning the config.
Ingests all upstream fixes since 2026-03-22 including: - Fix Cmd+N crash: retain snapshot workspaces (manaflow-ai#2183, manaflow-ai#2181, manaflow-ai#2178, manaflow-ai#2173) - Fix browser pane restore after reopen (manaflow-ai#2141) - Fix Ghostty resize_split keybind (manaflow-ai#1899) - Reduce shell integration prompt latency (manaflow-ai#2109) - Fix command palette focus after terminal find (manaflow-ai#2089) - Add Codex CLI hooks (manaflow-ai#2103) - Add cmux.json custom commands (manaflow-ai#2011) - Fix window position restore on relaunch (manaflow-ai#2129) Conflict resolution: - BrowserPanel.swift: accepted upstream configureWebViewConfiguration() refactor (already includes our forMainFrameOnly:true CAPTCHA fix from PR manaflow-ai#1877) Fork-specific files preserved: - Sources/Panels/WebAuthn{Coordinator,BridgeJavaScript}.swift - Sources/FIDO2/module.modulemap - vendor/ctap2 submodule - cmux.entitlements (with camera/audio-input removed) - cmux.embedded.entitlements - .github/workflows/fork-{ci,release}.yml
…#2178) * Add workspace config sanitization regression test * Sanitize workspace creation config snapshots --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
2 participants
Summary
WorkspaceCreationSnapshotfrom storing the fullghostty_surface_config_s; snapshot only inherited font points and rebuild a clean config template during workspace creationContext
PR #2133 stopped snapshotting live
Workspacereferences, butWorkspaceCreationSnapshotstill retained a fullghostty_surface_config_s. That C struct can include raw pointers owned by the source terminal surface. If Cmd+N captures the snapshot and the source workspace or surface tears down before the new workspace is created, the snapshot can carry dangling pointers and crash in the Cmd+N path described in #2157.Closes #2157
Validation
./scripts/reload.sh --tag cmd-n-fix --launchSummary by CodeRabbit