From 7638b2b145df46b0b80eaf06a1b24ffdeae890df Mon Sep 17 00:00:00 2001 From: arzafran Date: Wed, 5 Aug 2026 15:42:50 -0300 Subject: [PATCH] fix: make userdata registry own callback data instead of dereferencing pointer GhosttySurfaceUserdataRegistry (added in #262 to stop ghostty callbacks resolving freed GhosttySurfaceCallbackContext pointers) held its lock across a DEBUG dlog() call on the stale-pointer path, and resolve(from:) nested the context's own tabIdLock inside the registry lock while dereferencing the pointer. Both are gone: the registry now stores the (surfaceId, tabId) snapshot directly in a dictionary keyed by pointer, so resolve(from:) is a pure lookup under one lock, and the stale dlog fires after the lock is released. - GhosttySurfaceCallbackContext no longer stores tabId; it's now just an identity/lifetime token for Unmanaged retain/release bookkeeping (its tabIdLock/_tabId/tabId getter/updateTabId were only read by the old resolve(from:), which no longer dereferences the class at all). - TerminalSurface.updateWorkspaceId now propagates a tabId reassignment into the registry via GhosttySurfaceUserdataRegistry.updateTabId(pointer:tabId:), the same snapshot callbacks resolve. - Regression test testStaleSurfaceUserdataResolvesToNilInsteadOfCrashing still passes unmodified (comment updated to note the mechanism change); added testSurfaceUserdataResolvesUpdatedTabIdAfterWorkspaceReassignment to pin the new tabId-propagation behavior. --- Sources/GhosttyApp.swift | 108 ++++++++++++++----------- Sources/TerminalSurface.swift | 12 +-- programaTests/WorkspaceUnitTests.swift | 50 ++++++++++++ 3 files changed, 117 insertions(+), 53 deletions(-) diff --git a/Sources/GhosttyApp.swift b/Sources/GhosttyApp.swift index 429b2f70..32df2ef5 100644 --- a/Sources/GhosttyApp.swift +++ b/Sources/GhosttyApp.swift @@ -38,35 +38,20 @@ private func programaRuntimeReadClipboardCallback( // `(tabId, surfaceId)` through the tab manager / workspace panels registry -- see // `GhosttyApp.resolveTerminalPanel(tabId:surfaceId:)` and friends. final class GhosttySurfaceCallbackContext { - /// Immutable for the lifetime of the surface. + /// Immutable for the lifetime of the surface. Kept only as an identity/lifetime token + /// for `Unmanaged.passRetained`/`.release()` bookkeeping -- callbacks resolve + /// `(surfaceId, tabId)` from `GhosttySurfaceUserdataRegistry`'s own dictionary (see its + /// doc comment), never by dereferencing this object, so it carries no mutable state of + /// its own. let surfaceId: UUID - // `tabId` can change (a surface can be reassigned to a different workspace via - // `TerminalSurface.updateWorkspaceId`). Written only from main; read from any thread, - // so it's guarded by a lock rather than left as a plain `var`. - private let tabIdLock = NSLock() - private var _tabId: UUID? - - init(surfaceId: UUID, tabId: UUID?) { + init(surfaceId: UUID) { self.surfaceId = surfaceId - self._tabId = tabId - } - - var tabId: UUID? { - tabIdLock.lock() - defer { tabIdLock.unlock() } - return _tabId - } - - /// Must only be called from the main thread. - func updateTabId(_ newTabId: UUID?) { - tabIdLock.lock() - _tabId = newTabId - tabIdLock.unlock() } } -/// Registry of currently-live `GhosttySurfaceCallbackContext` userdata pointers. +/// Registry of currently-live `GhosttySurfaceCallbackContext` userdata pointers, mapped to +/// a value snapshot of the data callbacks need (`surfaceId`/`tabId`). /// /// Ghostty can invoke callbacks carrying a surface's `ghostty_surface_userdata` pointer /// well after the surface itself has been torn down -- e.g. a `.scrollbar` surface message @@ -81,23 +66,27 @@ final class GhosttySurfaceCallbackContext { /// different class). /// /// `read_clipboard_cb`/`confirm_read_clipboard_cb` resolve this off the ghostty IO thread -/// (see the "off-main" comment on `runtimeReadClipboardCallback` below, and -/// `GhosttySurfaceCallbackContext`'s own doc comment) -- so a main-actor-only registry is -/// NOT sound here. This uses a lock instead. `register`/`release`/`resolve` all take the -/// same lock, and `resolve` copies out the needed value fields (`surfaceId`/`tabId`) into a -/// plain struct while still holding it, so no caller anywhere ever sees a strong/unretained -/// reference to the live class outside that critical section -- a concurrent `release` can -/// never race a concurrent `resolve` into observing a pointer as "live" and then reading a -/// freed object. +/// (see the "off-main" comment on `runtimeReadClipboardCallback` below) -- so a +/// main-actor-only registry is NOT sound here. This uses a lock instead. Unlike the +/// original fix (#262), the registry now OWNS the data: `register`/`updateTabId` store the +/// snapshot directly, keyed by pointer, so `resolve(from:)` is a pure dictionary lookup -- +/// it never calls `Unmanaged.fromOpaque(_:).takeUnretainedValue()` and never touches the +/// `GhosttySurfaceCallbackContext` instance at all. This removes two defects the pointer-set +/// version had: (1) the DEBUG stale-pointer `dlog` used to fire while `lock` was held, and +/// dlog does synchronous real-time file I/O -- on the frequent stale-teardown path under +/// surface churn that serialized every ghostty callback on file I/O; now the log is emitted +/// after the lock is released. (2) `resolve` used to read `context.tabId`, which took the +/// context's own lock while still holding the registry lock (a nested-lock pattern) and +/// required dereferencing a pointer that is exactly what we're trying to make un-dereferenceable. enum GhosttySurfaceUserdataRegistry { private static let lock = NSLock() - private static var livePointers: Set = [] + private static var liveContexts: [UnsafeMutableRawPointer: GhosttySurfaceCallbackSnapshot] = [:] /// Call once, right after creating the context, before handing its pointer to ghostty /// (`ghostty_surface_new`/`ghostty_surface_config_s.userdata`). - static func register(_ pointer: UnsafeMutableRawPointer) { + static func register(_ pointer: UnsafeMutableRawPointer, surfaceId: UUID, tabId: UUID?) { lock.lock() - livePointers.insert(pointer) + liveContexts[pointer] = GhosttySurfaceCallbackSnapshot(surfaceId: surfaceId, tabId: tabId) lock.unlock() } @@ -109,33 +98,46 @@ enum GhosttySurfaceUserdataRegistry { guard let unmanaged else { return } let pointer = unmanaged.toOpaque() lock.lock() - livePointers.remove(pointer) + liveContexts.removeValue(forKey: pointer) lock.unlock() unmanaged.release() } - /// Resolves `pointer` to a value snapshot of its `GhosttySurfaceCallbackContext`, or - /// `nil` if `pointer` is nil or stale (already torn down). Safe to call from any - /// thread. + /// Updates the stored snapshot's `tabId` for an already-registered pointer, so a + /// surface reassigned to a different workspace (`TerminalSurface.updateWorkspaceId`) + /// doesn't leave callbacks resolving a stale `tabId`. No-op if `pointer` isn't + /// currently registered (e.g. called racing a teardown). + static func updateTabId(pointer: UnsafeMutableRawPointer, tabId: UUID?) { + lock.lock() + if let existing = liveContexts[pointer] { + liveContexts[pointer] = GhosttySurfaceCallbackSnapshot(surfaceId: existing.surfaceId, tabId: tabId) + } + lock.unlock() + } + + /// Resolves `pointer` to a value snapshot, or `nil` if `pointer` is nil or stale + /// (already torn down). Safe to call from any thread. Does the minimum work under + /// `lock` (a dictionary lookup/copy) and never dereferences the pointer as an object -- + /// the DEBUG stale-pointer log, if it fires, is emitted after the lock is released. static func resolve(from pointer: UnsafeMutableRawPointer?) -> GhosttySurfaceCallbackSnapshot? { guard let pointer else { return nil } lock.lock() - defer { lock.unlock() } - guard livePointers.contains(pointer) else { + let snapshot = liveContexts[pointer] + lock.unlock() #if DEBUG + if snapshot == nil { dlog("surface.userdata stale pointer rejected ptr=\(pointer)") -#endif - return nil } - let context = Unmanaged.fromOpaque(pointer).takeUnretainedValue() - return GhosttySurfaceCallbackSnapshot(surfaceId: context.surfaceId, tabId: context.tabId) +#endif + return snapshot } } -/// Value snapshot of a `GhosttySurfaceCallbackContext`, taken atomically with the liveness -/// check in `GhosttySurfaceUserdataRegistry.resolve(from:)`. Callers never see the live -/// class reference itself, so there's no way to retain (or read a property of) a -/// `GhosttySurfaceCallbackContext` from outside that registry's lock. +/// Value snapshot of the data a callback needs for a live `GhosttySurfaceCallbackContext` +/// pointer, owned directly by `GhosttySurfaceUserdataRegistry`'s dictionary. Callers never +/// see the live class reference itself -- there's no way to retain (or read a property of) +/// a `GhosttySurfaceCallbackContext` from a callback at all, since `resolve(from:)` never +/// dereferences the pointer. struct GhosttySurfaceCallbackSnapshot { let surfaceId: UUID let tabId: UUID? @@ -2319,5 +2321,15 @@ class GhosttyApp { static func debugCallbackContextResolves(from userdata: UnsafeMutableRawPointer?) -> Bool { callbackContext(from: userdata) != nil } + + /// Test-only seam exposing the resolved snapshot's `tabId`, so a test can verify + /// `TerminalSurface.updateWorkspaceId` propagates through to + /// `GhosttySurfaceUserdataRegistry.updateTabId(pointer:tabId:)` and is visible to a + /// subsequent `resolve(from:)` -- i.e. the same pointer callbacks would see. Callers + /// should first assert `debugCallbackContextResolves(from:)` to distinguish "pointer is + /// stale" from "tabId happens to be nil". + static func debugCallbackContextTabId(from userdata: UnsafeMutableRawPointer?) -> UUID? { + callbackContext(from: userdata)?.tabId + } #endif } diff --git a/Sources/TerminalSurface.swift b/Sources/TerminalSurface.swift index f024f4d1..e81b9871 100644 --- a/Sources/TerminalSurface.swift +++ b/Sources/TerminalSurface.swift @@ -385,9 +385,11 @@ final class TerminalSurface: Identifiable, ObservableObject { tabId = newTabId attachedView?.tabId = newTabId surfaceView.tabId = newTabId - // Called on main (this whole method mutates main-thread-only view state) -- - // satisfies GhosttySurfaceCallbackContext.updateTabId's main-thread contract. - surfaceCallbackContext?.takeUnretainedValue().updateTabId(newTabId) + // Keep the registry's snapshot in sync so callbacks resolving this pointer see the + // new tabId -- the registry (not this object) is what callbacks actually read. + if let pointer = surfaceCallbackContext?.toOpaque() { + GhosttySurfaceUserdataRegistry.updateTabId(pointer: pointer, tabId: newTabId) + } } private static func mergedNormalizedEnvironment( @@ -1155,8 +1157,8 @@ final class TerminalSurface: Identifiable, ObservableObject { surfaceConfig.platform = ghostty_platform_u(macos: ghostty_platform_macos_s( nsview: Unmanaged.passUnretained(view).toOpaque() )) - let callbackContext = Unmanaged.passRetained(GhosttySurfaceCallbackContext(surfaceId: id, tabId: tabId)) - GhosttySurfaceUserdataRegistry.register(callbackContext.toOpaque()) + let callbackContext = Unmanaged.passRetained(GhosttySurfaceCallbackContext(surfaceId: id)) + GhosttySurfaceUserdataRegistry.register(callbackContext.toOpaque(), surfaceId: id, tabId: tabId) surfaceConfig.userdata = callbackContext.toOpaque() GhosttySurfaceUserdataRegistry.release(surfaceCallbackContext) surfaceCallbackContext = callbackContext diff --git a/programaTests/WorkspaceUnitTests.swift b/programaTests/WorkspaceUnitTests.swift index 09dd83b4..c85c5eed 100644 --- a/programaTests/WorkspaceUnitTests.swift +++ b/programaTests/WorkspaceUnitTests.swift @@ -2195,6 +2195,14 @@ final class WorkspaceSplitWorkingDirectoryTests: XCTestCase { /// exact stale-pointer shape: capture the live userdata pointer, tear the surface down /// through the real teardown path, then resolve that same pointer and assert it comes /// back `nil` instead of dereferencing freed memory. + /// + /// Mechanism note: the fix originally shipped in #262 kept a `Set` of live pointers and + /// still dereferenced `context.tabId` (the crash site above) under the registry's lock. + /// A follow-up made `GhosttySurfaceUserdataRegistry` own the `(surfaceId, tabId)` data + /// directly in a dictionary keyed by pointer, so `resolve(from:)` is now a pure lookup + /// that never dereferences the pointer as an object at all -- this test's assertions + /// are unchanged, but the "instead of crashing" is now structurally guaranteed rather + /// than dependent on the liveness check running before the dereference. func testStaleSurfaceUserdataResolvesToNilInsteadOfCrashing() throws { #if DEBUG let workspace = Workspace() @@ -2228,6 +2236,48 @@ final class WorkspaceSplitWorkingDirectoryTests: XCTestCase { ) #else throw XCTSkip("Debug-only regression test") +#endif + } + + /// Pins new behavior from the registry redesign: `TerminalSurface.updateWorkspaceId` + /// must propagate the new `tabId` into `GhosttySurfaceUserdataRegistry`'s own snapshot, + /// not just onto the (no-longer-read-by-callbacks) `GhosttySurfaceCallbackContext` + /// instance, or a callback resolving this pointer after a workspace reassignment would + /// see a stale `tabId`. + func testSurfaceUserdataResolvesUpdatedTabIdAfterWorkspaceReassignment() 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) } + + guard let userdata = sourcePanel.surface.debugCallbackUserdataPointer() else { + XCTFail("Expected a live callback-context userdata pointer") + return + } + + let originalTabId = GhosttyApp.debugCallbackContextTabId(from: userdata) + + let reassignedTabId = UUID() + sourcePanel.surface.updateWorkspaceId(reassignedTabId) + + XCTAssertNotEqual( + originalTabId, + reassignedTabId, + "Test setup invariant: the reassigned tabId must actually differ from the original" + ) + XCTAssertEqual( + GhosttyApp.debugCallbackContextTabId(from: userdata), + reassignedTabId, + "Expected the registry's stored snapshot to reflect the reassigned tabId, since callbacks resolve tabId from the registry, not from the callback-context instance" + ) +#else + throw XCTSkip("Debug-only regression test") #endif } }