diff --git a/Sources/GhosttyApp.swift b/Sources/GhosttyApp.swift index 17cd5deb..429b2f70 100644 --- a/Sources/GhosttyApp.swift +++ b/Sources/GhosttyApp.swift @@ -66,6 +66,81 @@ final class GhosttySurfaceCallbackContext { } } +/// Registry of currently-live `GhosttySurfaceCallbackContext` userdata pointers. +/// +/// 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 +/// enqueued by the renderer thread (ghostty/src/renderer/generic.zig:1477-1483) and later +/// drained on main via `App.tick` -> `drainMailbox` -> `Surface.updateScrollbar` -> +/// `performAction`, or a clipboard read/confirm callback invoked synchronously off-main +/// (see `runtimeReadClipboardCallback` and `confirm_read_clipboard_cb` below). Resolving +/// `Unmanaged.fromOpaque(_:).takeUnretainedValue()` on a pointer whose backing object was +/// already released is undefined behavior: it crashed as an `objc_retain` `EXC_BAD_ACCESS` +/// in `GhosttySurfaceCallbackContext.tabId.getter` in 6 identical CI minidumps (and once as +/// `doesNotRecognizeSelector`, when the freed page had been reused by a live object of a +/// 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. +enum GhosttySurfaceUserdataRegistry { + private static let lock = NSLock() + private static var livePointers: Set = [] + + /// 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) { + lock.lock() + livePointers.insert(pointer) + lock.unlock() + } + + /// Unregisters `unmanaged`'s pointer and releases it, atomically with respect to + /// `resolve(from:)`. Use this instead of calling `.release()` directly at every + /// teardown site, so a concurrent resolver can never observe a pointer as live after + /// it's been freed. + static func release(_ unmanaged: Unmanaged?) { + guard let unmanaged else { return } + let pointer = unmanaged.toOpaque() + lock.lock() + livePointers.remove(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. + static func resolve(from pointer: UnsafeMutableRawPointer?) -> GhosttySurfaceCallbackSnapshot? { + guard let pointer else { return nil } + lock.lock() + defer { lock.unlock() } + guard livePointers.contains(pointer) else { +#if DEBUG + 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) + } +} + +/// 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. +struct GhosttySurfaceCallbackSnapshot { + let surfaceId: UUID + let tabId: UUID? +} + // Minimal Ghostty wrapper for terminal rendering // This uses libghostty (GhosttyKit.xcframework) for actual terminal emulation @@ -1486,9 +1561,8 @@ class GhosttyApp { } } - private static func callbackContext(from userdata: UnsafeMutableRawPointer?) -> GhosttySurfaceCallbackContext? { - guard let userdata else { return nil } - return Unmanaged.fromOpaque(userdata).takeUnretainedValue() + private static func callbackContext(from userdata: UnsafeMutableRawPointer?) -> GhosttySurfaceCallbackSnapshot? { + GhosttySurfaceUserdataRegistry.resolve(from: userdata) } // MARK: - Main-thread-only live object resolution @@ -2234,4 +2308,16 @@ class GhosttyApp { "\(timestamp) seq=\(sequence) t+\(String(format: "%.3f", uptimeMs))ms thread=\(threadLabel) frame60=\(frame60) frame120=\(frame120) cmux bg: \(message)\n" backgroundLogWriter.append(line) } + +#if DEBUG + /// Test-only seam exposing the private `callbackContext(from:)` resolution (which + /// wraps `GhosttySurfaceUserdataRegistry.resolve(from:)`) so tests can verify a stale + /// userdata pointer resolves to `nil` instead of dereferencing freed memory. Widened + /// from `private` to an internal, DEBUG-only static func rather than exposing the + /// registry or the callback context type directly, matching the DEBUG-only test + /// scaffolding pattern used elsewhere (e.g. `TerminalSurface.replaceSurfaceWithFreedPointerForTesting`). + static func debugCallbackContextResolves(from userdata: UnsafeMutableRawPointer?) -> Bool { + callbackContext(from: userdata) != nil + } +#endif } diff --git a/Sources/TerminalSurface.swift b/Sources/TerminalSurface.swift index 694d6664..f024f4d1 100644 --- a/Sources/TerminalSurface.swift +++ b/Sources/TerminalSurface.swift @@ -653,7 +653,7 @@ final class TerminalSurface: Identifiable, ObservableObject { "registryOwner=\(registeredOwnerToken)" ) #endif - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) return nil } return surface @@ -903,7 +903,7 @@ final class TerminalSurface: Identifiable, ObservableObject { "workspace=\(tabId.uuidString.prefix(5))" ) #endif - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) tapContext?.release() return } @@ -911,7 +911,7 @@ final class TerminalSurface: Identifiable, ObservableObject { #if DEBUG if runtimeSurfaceFreedOutOfBandForTesting { runtimeSurfaceFreedOutOfBandForTesting = false - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) tapContext?.release() return } @@ -933,7 +933,7 @@ final class TerminalSurface: Identifiable, ObservableObject { // it's torn down. SessionWALStore.shared.unregister(surface: surfaceToFree, surfaceId: surfaceIdForTap, deleteDirectory: true) ghostty_surface_free(surfaceToFree) - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) tapContext?.release() #if DEBUG dlog( @@ -1156,8 +1156,9 @@ final class TerminalSurface: Identifiable, ObservableObject { nsview: Unmanaged.passUnretained(view).toOpaque() )) let callbackContext = Unmanaged.passRetained(GhosttySurfaceCallbackContext(surfaceId: id, tabId: tabId)) + GhosttySurfaceUserdataRegistry.register(callbackContext.toOpaque()) surfaceConfig.userdata = callbackContext.toOpaque() - surfaceCallbackContext?.release() + GhosttySurfaceUserdataRegistry.release(surfaceCallbackContext) surfaceCallbackContext = callbackContext surfaceConfig.scale_factor = scaleFactors.layer surfaceConfig.context = surfaceContext @@ -1418,7 +1419,7 @@ final class TerminalSurface: Identifiable, ObservableObject { pendingReviveSeed = nil pendingReviveWinchPGID = nil pendingReviveWinchChildPID = nil - surfaceCallbackContext?.release() + GhosttySurfaceUserdataRegistry.release(surfaceCallbackContext) surfaceCallbackContext = nil print("Failed to create ghostty surface") #if DEBUG @@ -2363,6 +2364,15 @@ final class TerminalSurface: Identifiable, ObservableObject { needsConfirmCloseOverrideForTesting = value } + /// Test-only seam exposing the raw `ghostty_surface_userdata` pointer this surface + /// currently owns, so a test can capture it before teardown and then attempt to + /// resolve it afterward (`GhosttyApp.debugCallbackContextResolves(from:)`) to prove a + /// stale pointer is rejected instead of dereferencing freed memory. + @MainActor + func debugCallbackUserdataPointer() -> UnsafeMutableRawPointer? { + surfaceCallbackContext?.toOpaque() + } + /// Test-only helper to deterministically simulate a released runtime surface. @MainActor func releaseSurfaceForTesting() { @@ -2372,7 +2382,7 @@ final class TerminalSurface: Identifiable, ObservableObject { outputTapContext = nil guard let surfaceToFree = surface else { - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) tapContext?.release() return } @@ -2382,7 +2392,7 @@ final class TerminalSurface: Identifiable, ObservableObject { // Test-only teardown, not a real close: keep the WAL directory. SessionWALStore.shared.unregister(surface: surfaceToFree, surfaceId: id.uuidString) ghostty_surface_free(surfaceToFree) - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) tapContext?.release() } @@ -2398,7 +2408,7 @@ final class TerminalSurface: Identifiable, ObservableObject { outputTapContext = nil guard let surfaceToFree = surface else { - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) tapContext?.release() return } @@ -2408,7 +2418,7 @@ final class TerminalSurface: Identifiable, ObservableObject { SessionWALStore.shared.unregister(surface: surfaceToFree, surfaceId: id.uuidString) ghostty_surface_free(surfaceToFree) runtimeSurfaceFreedOutOfBandForTesting = true - callbackContext?.release() + GhosttySurfaceUserdataRegistry.release(callbackContext) tapContext?.release() } #endif diff --git a/programaTests/WorkspaceUnitTests.swift b/programaTests/WorkspaceUnitTests.swift index 0f53be0a..09dd83b4 100644 --- a/programaTests/WorkspaceUnitTests.swift +++ b/programaTests/WorkspaceUnitTests.swift @@ -2184,6 +2184,50 @@ final class WorkspaceSplitWorkingDirectoryTests: XCTestCase { XCTAssertNil(sourcePanel.surface.surface, "Expected stale surface pointer to be quarantined") #else throw XCTSkip("Debug-only regression test") +#endif + } + + /// Regression test for the occluded-render re-land crash: 6 identical CI minidumps + /// symbolicated to `objc_retain` `EXC_BAD_ACCESS` in + /// `GhosttySurfaceCallbackContext.tabId.getter`, reached from + /// `GhosttyApp.callbackContext(from:)` resolving a `ghostty_surface_userdata` pointer + /// whose backing context had already been released by teardown. This exercises the + /// 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. + func testStaleSurfaceUserdataResolvesToNilInsteadOfCrashing() 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 before teardown") + return + } + + XCTAssertTrue( + GhosttyApp.debugCallbackContextResolves(from: userdata), + "Expected the live userdata pointer to resolve before teardown" + ) + + // Tear down through the real teardown path: this is the same release site + // (performSurfaceTeardown/liveSurfaceForGhosttyAccess's sibling) that frees the + // callback context in production. + sourcePanel.surface.replaceSurfaceWithFreedPointerForTesting() + + XCTAssertFalse( + GhosttyApp.debugCallbackContextResolves(from: userdata), + "Expected a stale userdata pointer to resolve to nil instead of dereferencing freed memory" + ) +#else + throw XCTSkip("Debug-only regression test") #endif } }