marketing-shibata50/cmux
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: guard inherited terminal config against stale surfaces (#2101)

Browse source 
* test: add stale inherited surface regression

* fix: guard inherited terminal config against stale surfaces

* fix: address stale surface review feedback
This commit is contained in:
Austin Wang 2026-03-25 16:49:54 -07:00 • committed by GitHub
parent 99ca3c9b9a
commit 9f2adce830
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 253 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

103
Sources/GhosttyTerminalView.swift
View file

@ -2789,6 +2789,7 @@ final class TerminalSurfaceRegistry {
private let lock = NSLock()
private let surfaces = NSHashTable<AnyObject>.weakObjects()
private var runtimeSurfaceOwners: [UInt: UUID] = [:]
private init() {}
@ -2798,6 +2799,26 @@ final class TerminalSurfaceRegistry {
surfaces.add(surface)
}
func registerRuntimeSurface(_ surface: ghostty_surface_t, ownerId: UUID) {
lock.lock()
defer { lock.unlock() }
runtimeSurfaceOwners[UInt(bitPattern: surface)] = ownerId
}
func unregisterRuntimeSurface(_ surface: ghostty_surface_t, ownerId: UUID) {
lock.lock()
defer { lock.unlock() }
let key = UInt(bitPattern: surface)
guard runtimeSurfaceOwners[key] == ownerId else { return }
runtimeSurfaceOwners.removeValue(forKey: key)
}
func runtimeSurfaceOwnerId(_ surface: ghostty_surface_t) -> UUID? {
lock.lock()
defer { lock.unlock() }
return runtimeSurfaceOwners[UInt(bitPattern: surface)]
}
func allSurfaces() -> [TerminalSurface] {
lock.lock()
let objects = surfaces.allObjects.compactMap { $0 as? TerminalSurface }
@ -2828,10 +2849,11 @@ final class TerminalSurface: Identifiable, ObservableObject {
/// Whether the runtime Ghostty surface exists and has not begun teardown.
///
/// Use this before passing `surface` to Ghostty C APIs that dereference the
/// pointer (e.g. `ghostty_surface_inherited_config`, `ghostty_surface_quicklook_font`).
/// A non-nil `surface` alone is not sufficient because the underlying native
/// state may already be closing or closed.
/// Use this as a quick availability check. Before passing `surface` to
/// Ghostty C APIs that dereference the pointer (e.g.
/// `ghostty_surface_inherited_config`, `ghostty_surface_quicklook_font`),
/// call `liveSurfaceForGhosttyAccess(reason:)` so stale freed pointers are
/// rejected and quarantined.
var hasLiveSurface: Bool { surface != nil && portalLifecycleState == .live }
/// Whether the terminal surface view is currently attached to a window.
@ -2884,6 +2906,7 @@ final class TerminalSurface: Identifiable, ObservableObject {
private var desiredFocusState: Bool = true
#if DEBUG
private var needsConfirmCloseOverrideForTesting: Bool?
private var runtimeSurfaceFreedOutOfBandForTesting = false
#endif
private enum PortalLifecycleState: String {
case live
@ -3074,6 +3097,34 @@ final class TerminalSurface: Identifiable, ObservableObject {
return true
}
@MainActor
func liveSurfaceForGhosttyAccess(reason: String) -> ghostty_surface_t? {
guard hasLiveSurface, let surface else { return nil }
let registry = TerminalSurfaceRegistry.shared
let registeredOwnerId = registry.runtimeSurfaceOwnerId(surface)
guard registeredOwnerId == id,
cmuxSurfacePointerAppearsLive(surface) else {
let callbackContext = surfaceCallbackContext
surfaceCallbackContext = nil
registry.unregisterRuntimeSurface(surface, ownerId: id)
self.surface = nil
activePortalHostLease = nil
recordTeardownRequest(reason: reason)
markPortalLifecycleClosed(reason: reason)
#if DEBUG
let registeredOwnerToken = registeredOwnerId.map { String($0.uuidString.prefix(5)) } ?? "nil"
dlog(
"surface.lifecycle.stale surface=\(id.uuidString.prefix(5)) " +
"workspace=\(tabId.uuidString.prefix(5)) reason=\(reason) " +
"registryOwner=\(registeredOwnerToken)"
)
#endif
callbackContext?.release()
return nil
}
return surface
}
private static let portalHostAreaThreshold: CGFloat = 4
private static func portalHostArea(for bounds: CGRect) -> CGFloat {
@ -3259,6 +3310,9 @@ final class TerminalSurface: Identifiable, ObservableObject {
surfaceCallbackContext = nil
let surfaceToFree = surface
if let surfaceToFree {
TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id)
}
surface = nil
guard let surfaceToFree else {
@ -3266,6 +3320,14 @@ final class TerminalSurface: Identifiable, ObservableObject {
return
}
#if DEBUG
if runtimeSurfaceFreedOutOfBandForTesting {
runtimeSurfaceFreedOutOfBandForTesting = false
callbackContext?.release()
return
}
#endif
Task { @MainActor in
// Keep free behavior aligned with deinit: perform the runtime teardown on
// the next main-actor turn so SIGHUP delivery is deterministic but non-reentrant.
@ -3649,6 +3711,7 @@ final class TerminalSurface: Identifiable, ObservableObject {
return
}
guard let createdSurface = surface else { return }
TerminalSurfaceRegistry.shared.registerRuntimeSurface(createdSurface, ownerId: id)
recordRuntimeSurfaceCreation()
// Session scrollback replay must be one-shot. Reusing it on a later runtime
@ -4066,10 +4129,31 @@ final class TerminalSurface: Identifiable, ObservableObject {
return
}
TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id)
surface = nil
ghostty_surface_free(surfaceToFree)
callbackContext?.release()
}
/// Test-only helper to simulate a stale Swift wrapper whose native surface
/// was already freed out-of-band.
@MainActor
func replaceSurfaceWithFreedPointerForTesting() {
guard !runtimeSurfaceFreedOutOfBandForTesting else { return }
let callbackContext = surfaceCallbackContext
surfaceCallbackContext = nil
guard let surfaceToFree = surface else {
callbackContext?.release()
return
}
TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id)
ghostty_surface_free(surfaceToFree)
runtimeSurfaceFreedOutOfBandForTesting = true
callbackContext?.release()
}
#endif
deinit {
@ -4083,6 +4167,9 @@ final class TerminalSurface: Identifiable, ObservableObject {
// before this object is fully deallocated will see nil and bail out,
// rather than passing a freed pointer to ghostty_surface_refresh (#432).
let surfaceToFree = surface
if let surfaceToFree {
TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id)
}
surface = nil
guard let surfaceToFree else {
@ -4096,6 +4183,14 @@ final class TerminalSurface: Identifiable, ObservableObject {
return
}
#if DEBUG
if runtimeSurfaceFreedOutOfBandForTesting {
runtimeSurfaceFreedOutOfBandForTesting = false
callbackContext?.release()
return
}
#endif
#if DEBUG
let surfaceToken = String(id.uuidString.prefix(5))
let workspaceToken = String(tabId.uuidString.prefix(5))

5
Sources/TabManager.swift
View file

@ -2216,8 +2216,9 @@ class TabManager: ObservableObject {
workspace: Workspace?
) -> ghostty_surface_config_s? {
if let panel = terminalPanelForWorkspaceConfigInheritanceSource(workspace: workspace),
panel.surface.hasLiveSurface,
let sourceSurface = panel.surface.surface {
let sourceSurface = panel.surface.liveSurfaceForGhosttyAccess(
reason: "tabManager.inheritedTerminalConfigForNewWorkspace"
) {
return cmuxInheritedSurfaceConfig(
sourceSurface: sourceSurface,
context: GHOSTTY_SURFACE_CONTEXT_TAB

58
Sources/Workspace.swift
View file

@ -21,20 +21,35 @@ func cmuxSurfaceContextName(_ context: ghostty_surface_context_e) -> String {
}
}
private func cmuxPointerAppearsLive(_ pointer: UnsafeMutableRawPointer?) -> Bool {
guard let pointer,
malloc_zone_from_ptr(pointer) != nil else {
return false
}
return malloc_size(pointer) > 0
}
func cmuxSurfacePointerAppearsLive(_ surface: ghostty_surface_t) -> Bool {
// Best-effort check: reject pointers that no longer belong to an active
// malloc zone allocation. A Swift wrapper around `ghostty_surface_t` can
// remain non-nil after the backing native surface has already been freed.
cmuxPointerAppearsLive(surface)
}
func cmuxCurrentSurfaceFontSizePoints(_ surface: ghostty_surface_t) -> Float? {
guard cmuxSurfacePointerAppearsLive(surface) else {
return nil
}
guard let quicklookFont = ghostty_surface_quicklook_font(surface) else {
return nil
}
// Best-effort check: reject pointers that are not the start of a live
// malloc allocation. ghostty_surface_quicklook_font returns an unretained
// pointer whose lifetime is managed by Ghostty; on Intel Macs the pointer
// can become stale after the internal font is freed (#1496, #1870).
// malloc_size is safe to call with any address (returns 0 for non-malloc
// pointers without dereferencing them). This does not guarantee the memory
// still contains a valid CTFont, but it catches the common case of
// fully-freed or unmapped allocations that would otherwise SIGSEGV.
guard malloc_size(quicklookFont) > 0 else {
// Best-effort check: reject unretained font pointers that no longer belong
// to a live malloc allocation. This does not prove the object is still a
// valid CTFont, but it filters out the common fully-freed/unmapped cases
// that previously crashed on Intel Macs (#1496, #1870).
guard cmuxPointerAppearsLive(quicklookFont) else {
return nil
}
@ -7161,8 +7176,9 @@ final class Workspace: Identifiable, ObservableObject {
private func rememberTerminalConfigInheritanceSource(_ terminalPanel: TerminalPanel) {
lastTerminalConfigInheritancePanelId = terminalPanel.id
if terminalPanel.surface.hasLiveSurface,
let sourceSurface = terminalPanel.surface.surface,
if let sourceSurface = terminalPanel.surface.liveSurfaceForGhosttyAccess(
reason: "workspace.rememberConfigInheritanceSource"
),
let runtimePoints = cmuxCurrentSurfaceFontSizePoints(sourceSurface) {
let existing = terminalInheritanceFontPointsByPanelId[terminalPanel.id]
if existing == nil || abs((existing ?? runtimePoints) - runtimePoints) > 0.05 {
@ -7254,14 +7270,25 @@ final class Workspace: Identifiable, ObservableObject {
preferredPanelId: UUID? = nil,
inPane preferredPaneId: PaneID? = nil
) -> ghostty_surface_config_s? {
var staleRootedFontFallback: Float?
// Walk candidates in priority order and use the first panel with a live surface.
// This avoids returning nil when the top candidate exists but is not attached yet.
for terminalPanel in terminalPanelConfigInheritanceCandidates(
preferredPanelId: preferredPanelId,
inPane: preferredPaneId
) {
guard terminalPanel.surface.hasLiveSurface,
let sourceSurface = terminalPanel.surface.surface else { continue }
let rootedFontFallback = terminalInheritanceFontPointsByPanelId[terminalPanel.id]
guard let sourceSurface = terminalPanel.surface.liveSurfaceForGhosttyAccess(
reason: "workspace.inheritedTerminalConfig"
) else {
if staleRootedFontFallback == nil,
let rootedFontFallback,
rootedFontFallback > 0 {
staleRootedFontFallback = rootedFontFallback
}
continue
}
var config = cmuxInheritedSurfaceConfig(
sourceSurface: sourceSurface,
context: GHOSTTY_SURFACE_CONTEXT_SPLIT
@ -7281,12 +7308,13 @@ final class Workspace: Identifiable, ObservableObject {
return config
}
if let fallbackFontPoints = lastTerminalConfigInheritanceFontPoints {
if let fallbackFontPoints = staleRootedFontFallback ?? lastTerminalConfigInheritanceFontPoints {
var config = ghostty_surface_config_new()
config.font_size = fallbackFontPoints
#if DEBUG
let fallbackSource = staleRootedFontFallback != nil ? "quarantinedRootedFont" : "lastKnownFont"
dlog(
"zoom.inherit fallback=lastKnownFont context=split font=\(String(format: "%.2f", fallbackFontPoints))"
"zoom.inherit fallback=\(fallbackSource) context=split font=\(String(format: "%.2f", fallbackFontPoints))"
)
#endif
return config

108
cmuxTests/WorkspaceUnitTests.swift
View file

@ -906,6 +906,48 @@ final class WorkspaceTeardownTests: XCTestCase {
@MainActor
final class WorkspaceSplitWorkingDirectoryTests: XCTestCase {
private func waitForCondition(
timeout: TimeInterval = 2,
pollInterval: TimeInterval = 0.01,
_ condition: () -> Bool
) -> Bool {
let deadline = Date().addingTimeInterval(timeout)
while Date() < deadline {
if condition() {
return true
}
RunLoop.current.run(until: Date().addingTimeInterval(pollInterval))
}
return condition()
}
private func hostTerminalPanelInWindow(_ panel: TerminalPanel) throws -> NSWindow {
let window = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 420, height: 280),
styleMask: [.titled, .closable],
backing: .buffered,
defer: false
)
let contentView = try XCTUnwrap(window.contentView, "Expected content view")
let hostedView = panel.hostedView
hostedView.frame = contentView.bounds
hostedView.autoresizingMask = [.width, .height]
contentView.addSubview(hostedView)
window.makeKeyAndOrderFront(nil)
window.displayIfNeeded()
contentView.layoutSubtreeIfNeeded()
XCTAssertTrue(
waitForCondition {
panel.surface.surface != nil
},
"Expected runtime surface to materialize after hosting panel in a window"
)
return window
}
func testNewTerminalSplitFallsBackToRequestedWorkingDirectoryWhenReportedDirectoryIsStale() {
let workspace = Workspace()
guard let sourcePaneId = workspace.bonsplitController.focusedPaneId else {
@ -950,6 +992,72 @@ final class WorkspaceSplitWorkingDirectoryTests: XCTestCase {
"Expected split to inherit the source terminal's requested cwd when no reported cwd exists yet"
)
}
func testNewTerminalSplitSkipsFreedInheritedSurfacePointer() throws {
#if DEBUG
let workspace = Workspace()
guard let sourcePanelId = workspace.focusedPanelId,
let sourcePanel = workspace.terminalPanel(for: sourcePanelId) else {
XCTFail("Expected focused terminal panel")
return
}
let window = try hostTerminalPanelInWindow(sourcePanel)
defer { window.orderOut(nil) }
XCTAssertNotNil(sourcePanel.surface.surface, "Expected runtime surface before forcing stale pointer")
sourcePanel.surface.replaceSurfaceWithFreedPointerForTesting()
XCTAssertNotNil(
sourcePanel.surface.surface,
"Expected Swift wrapper to remain non-nil while simulating a stale native surface"
)
let splitPanel = workspace.newTerminalSplit(
from: sourcePanelId,
orientation: .horizontal,
focus: false
)
XCTAssertNotNil(splitPanel, "Expected split creation to survive a stale inherited surface pointer")
XCTAssertNil(sourcePanel.surface.surface, "Expected stale surface pointer to be quarantined")
#else
throw XCTSkip("Debug-only regression test")
#endif
}
func testNewTerminalSurfaceSkipsFreedInheritedSurfacePointer() throws {
#if DEBUG
let workspace = Workspace()
guard let sourcePanelId = workspace.focusedPanelId,
let sourcePanel = workspace.terminalPanel(for: sourcePanelId),
let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) else {
XCTFail("Expected focused terminal panel and pane")
return
}
let window = try hostTerminalPanelInWindow(sourcePanel)
defer { window.orderOut(nil) }
XCTAssertNotNil(sourcePanel.surface.surface, "Expected runtime surface before forcing stale pointer")
sourcePanel.surface.replaceSurfaceWithFreedPointerForTesting()
XCTAssertNotNil(
sourcePanel.surface.surface,
"Expected Swift wrapper to remain non-nil while simulating a stale native surface"
)
let createdPanel = workspace.newTerminalSurface(
inPane: sourcePaneId,
focus: false
)
XCTAssertNotNil(createdPanel, "Expected terminal creation to survive a stale inherited surface pointer")
XCTAssertNil(sourcePanel.surface.surface, "Expected stale surface pointer to be quarantined")
#else
throw XCTSkip("Debug-only regression test")
#endif
}
}