From 03df613e392b764d828941cb4f5864c5a75edb83 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 24 May 2026 14:09:28 -0700 Subject: [PATCH] libghostty: detach tracked grid refs on free Tracked grid references previously held a raw terminal wrapper pointer and were required to be freed before the terminal. If callers kept one past terminal destruction, later tracked-ref calls could dereference freed terminal or page-list memory before detecting that the reference was no longer meaningful. Track live C tracked-grid-ref handles from the terminal wrapper and detach them before tearing down terminal storage. Detached refs now report no value through the tracked-ref APIs and can still be freed by the caller. Update the C API docs to describe this lifetime behavior and add a regression test for using a tracked ref after terminal free. This introduces some overhead but tracked pins shouldn't be numerous and this dramatically improves safety. --- include/ghostty/vt/grid_ref.h | 9 ++++--- include/ghostty/vt/grid_ref_tracked.h | 13 ++++++--- include/ghostty/vt/terminal.h | 5 ++-- include/ghostty/vt/types.h | 4 ++- src/terminal/c/grid_ref_tracked.zig | 38 +++++++++++++++++++++++++++ src/terminal/c/terminal.zig | 19 ++++++++++++-- 6 files changed, 75 insertions(+), 13 deletions(-) diff --git a/include/ghostty/vt/grid_ref.h b/include/ghostty/vt/grid_ref.h index ca857a499..c43791dc2 100644 --- a/include/ghostty/vt/grid_ref.h +++ b/include/ghostty/vt/grid_ref.h @@ -70,8 +70,9 @@ extern "C" { * the tracked reference between screens. * * Tracked references are owned by the caller and must be freed with - * ghostty_tracked_grid_ref_free() before the terminal that created them is - * freed. + * ghostty_tracked_grid_ref_free(). If the terminal that created a tracked + * reference is freed first, the handle remains valid only for tracked-grid-ref + * APIs: it reports no value and can still be freed. * * Each tracked reference adds bookkeeping to terminal mutations. Use them * sparingly for long-lived anchors such as selections, search state, marks, @@ -85,8 +86,8 @@ extern "C" { * operation (including free). * * A tracked reference is allocated and must be freed when it is no - * longer needed. All tracked references must be freed prior to the - * terminal being freed. + * longer needed. A tracked reference may outlive the terminal that created it; + * after terminal free, it reports no value and can still be freed. * * ## Examples * diff --git a/include/ghostty/vt/grid_ref_tracked.h b/include/ghostty/vt/grid_ref_tracked.h index f80d7fbad..b56aefacd 100644 --- a/include/ghostty/vt/grid_ref_tracked.h +++ b/include/ghostty/vt/grid_ref_tracked.h @@ -27,8 +27,8 @@ extern "C" { /** * Free a tracked grid reference. * - * Passing NULL is allowed and has no effect. The reference must be freed before - * the terminal that created it is freed. + * Passing NULL is allowed and has no effect. A tracked reference may be freed + * after the terminal that created it is freed. * * @param ref Tracked grid reference to free. * @@ -39,6 +39,9 @@ GHOSTTY_API void ghostty_tracked_grid_ref_free(GhosttyTrackedGridRef ref); /** * Return whether a tracked grid reference currently has a meaningful value. * + * If the terminal that created the tracked reference has been freed, this + * returns false. + * * @param ref Tracked grid reference. * @return true if the reference currently has a meaningful value. * @@ -62,7 +65,8 @@ GHOSTTY_API bool ghostty_tracked_grid_ref_has_value( * * If the tracked reference no longer has a meaningful value, this returns * GHOSTTY_NO_VALUE. GHOSTTY_NO_VALUE is also returned when the reference cannot - * be represented in the requested coordinate space. + * be represented in the requested coordinate space, including after the + * terminal that created the tracked reference has been freed. * * @param ref Tracked grid reference. * @param tag Coordinate space to convert into. @@ -114,7 +118,8 @@ GHOSTTY_API GhosttyResult ghostty_tracked_grid_ref_set( * ghostty_grid_ref_style(). * * If the tracked reference no longer has a meaningful value, this returns - * GHOSTTY_NO_VALUE. + * GHOSTTY_NO_VALUE. This includes references whose owning terminal has been + * freed. * * @param ref Tracked grid reference. * @param[out] out_ref On success, receives an untracked snapshot. May be NULL. diff --git a/include/ghostty/vt/terminal.h b/include/ghostty/vt/terminal.h index 1751aa126..0acd921b7 100644 --- a/include/ghostty/vt/terminal.h +++ b/include/ghostty/vt/terminal.h @@ -1134,8 +1134,9 @@ GHOSTTY_API GhosttyResult ghostty_terminal_grid_ref(GhosttyTerminal terminal, * If the point is outside the requested coordinate space, this returns * GHOSTTY_INVALID_VALUE and writes NULL to out_ref. * - * The returned handle must be freed with ghostty_tracked_grid_ref_free() before - * the terminal is freed. + * The returned handle must be freed with ghostty_tracked_grid_ref_free(). If + * the terminal is freed first, the handle remains valid only for + * tracked-grid-ref APIs: it reports no value and can still be freed. * * @param terminal Terminal instance. * @param point Point to track. diff --git a/include/ghostty/vt/types.h b/include/ghostty/vt/types.h index e8e976207..502b01679 100644 --- a/include/ghostty/vt/types.h +++ b/include/ghostty/vt/types.h @@ -98,7 +98,9 @@ typedef struct GhosttyTerminalImpl* GhosttyTerminal; * Opaque handle to a tracked grid reference. * * A tracked grid reference is owned by the caller and must be freed with - * ghostty_tracked_grid_ref_free() before the terminal that created it is freed. + * ghostty_tracked_grid_ref_free(). If the terminal that created it is freed + * first, the handle remains valid only for tracked-grid-ref APIs: it reports no + * value and can still be freed. * * @ingroup grid_ref */ diff --git a/src/terminal/c/grid_ref_tracked.zig b/src/terminal/c/grid_ref_tracked.zig index 3233bb054..60091715f 100644 --- a/src/terminal/c/grid_ref_tracked.zig +++ b/src/terminal/c/grid_ref_tracked.zig @@ -33,6 +33,9 @@ pub const TrackedGridRef = struct { pub fn tracked_grid_ref_free(ref_: CTrackedGridRef) callconv(lib.calling_conv) void { const ref = ref_ orelse return; + if (ref.terminal) |wrapper| { + _ = wrapper.tracked_grid_refs.swapRemove(ref); + } if (ref.pageList()) |list| list.untrackPin(ref.pin); ref.alloc.destroy(ref); } @@ -185,3 +188,38 @@ test "tracked_grid_ref reports no value after alternate screen reset" { try testing.expect(tracked_grid_ref_has_value(ref)); try testing.expectEqual(Result.success, tracked_grid_ref_snapshot(ref, &snapshot)); } + +test "tracked_grid_ref reports no value after terminal free" { + var terminal: terminal_c.Terminal = null; + try testing.expectEqual(Result.success, terminal_c.new( + &lib.alloc.test_allocator, + &terminal, + .{ .cols = 5, .rows = 2, .max_scrollback = 10_000 }, + )); + + terminal_c.vt_write(terminal, "A", 1); + + var ref: CTrackedGridRef = null; + try testing.expectEqual(Result.success, terminal_c.grid_ref_track( + terminal, + point.Point.cval(.{ .active = .{ .x = 0, .y = 0 } }), + &ref, + )); + + terminal_c.free(terminal); + try testing.expect(!tracked_grid_ref_has_value(ref)); + + var snapshot: grid_ref_c.CGridRef = undefined; + try testing.expectEqual(Result.no_value, tracked_grid_ref_snapshot(ref, &snapshot)); + + var coord: point.Coordinate = undefined; + try testing.expectEqual(Result.no_value, tracked_grid_ref_point(ref, .active, &coord)); + + try testing.expectEqual(Result.invalid_value, tracked_grid_ref_set( + ref, + terminal, + point.Point.cval(.{ .active = .{ .x = 0, .y = 0 } }), + )); + + tracked_grid_ref_free(ref); +} diff --git a/src/terminal/c/terminal.zig b/src/terminal/c/terminal.zig index 662a2ec03..a38afd485 100644 --- a/src/terminal/c/terminal.zig +++ b/src/terminal/c/terminal.zig @@ -35,6 +35,7 @@ const TerminalWrapper = struct { terminal: *ZigTerminal, stream: Stream, effects: Effects = .{}, + tracked_grid_refs: std.AutoArrayHashMapUnmanaged(*grid_ref_tracked_c.TrackedGridRef, void) = .{}, }; /// C callback state for terminal effects. Trampolines are always @@ -758,6 +759,18 @@ pub fn grid_ref_track( .pin = tracked_pin, }; + // Store the tracked ref in the terminal so that when we free + // the terminal the tracked ref can be detached safely. + wrapper.tracked_grid_refs.putNoClobber( + alloc, + ref, + {}, + ) catch { + list.untrackPin(tracked_pin); + alloc.destroy(ref); + return .out_of_memory; + }; + out.* = ref; return .success; } @@ -779,9 +792,11 @@ pub fn point_from_grid_ref( pub fn free(terminal_: Terminal) callconv(lib.calling_conv) void { const wrapper = terminal_ orelse return; const t = wrapper.terminal; - - wrapper.stream.deinit(); const alloc = t.gpa(); + + for (wrapper.tracked_grid_refs.keys()) |ref| ref.terminal = null; + wrapper.tracked_grid_refs.deinit(alloc); + wrapper.stream.deinit(); t.deinit(alloc); alloc.destroy(t); alloc.destroy(wrapper);