From f0c918fc4b72c6c7e746db68f753d102a02bb206 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 31 Aug 2026 13:48:56 -0700 Subject: [PATCH] terminal/c: allow freeing a search and its terminal in any order --- src/terminal/c/search.zig | 130 ++++++++++++++++++++++++++++--- src/terminal/c/terminal.zig | 4 + src/terminal/search/terminal.zig | 48 ++++++++++-- 3 files changed, 165 insertions(+), 17 deletions(-) diff --git a/src/terminal/c/search.zig b/src/terminal/c/search.zig index 9f66c7395..e46c66509 100644 --- a/src/terminal/c/search.zig +++ b/src/terminal/c/search.zig @@ -14,18 +14,27 @@ const log = std.log.scoped(.search_c); /// C: GhosttySearch pub const Search = ?*SearchWrapper; -const SearchWrapper = struct { +pub const SearchWrapper = struct { alloc: std.mem.Allocator, /// The terminal this search is bound to. This is borrowed, so the - /// search never frees it and the terminal must outlive the search. - terminal: *terminal_c.ZigTerminal, + /// search never frees it. If the terminal is freed first, it sets + /// this to null to detach the search: calls that need the terminal + /// then fail cleanly and free releases only search-owned memory. + terminal: terminal_c.Terminal, search: TerminalSearch, /// The scroll policy applied by the select options. Persistent /// across selects, set via the select_scroll option. select_scroll: TerminalSearch.SelectScroll = .if_needed, + + /// The Zig terminal this search is bound to, or null if the + /// terminal was freed before the search. + fn zigTerminal(self: *const SearchWrapper) ?*terminal_c.ZigTerminal { + const terminal_wrapper = self.terminal orelse return null; + return terminal_wrapper.terminal; + } }; /// C: GhosttySearchStatus @@ -117,7 +126,7 @@ pub fn new( const out = out_search orelse return .invalid_value; out.* = null; - const t = terminal_c.zigTerminal(terminal) orelse return .invalid_value; + const terminal_wrapper = terminal orelse return .invalid_value; const opts = options orelse return .invalid_value; if (opts.size < @sizeOf(Options)) return .invalid_value; if (opts.needle.len == 0) return .invalid_value; @@ -132,16 +141,36 @@ pub fn new( }; wrapper.* = .{ .alloc = alloc, - .terminal = t, + .terminal = terminal_wrapper, .search = search, }; + + // Store the search in the terminal so that when the terminal is + // freed the search can be detached safely. + terminal_wrapper.searches.putNoClobber( + terminal_wrapper.terminal.gpa(), + wrapper, + {}, + ) catch { + wrapper.search.deinit(terminal_wrapper.terminal); + alloc.destroy(wrapper); + return .out_of_memory; + }; + out.* = wrapper; return .success; } pub fn free(search_: Search) callconv(lib.calling_conv) void { const wrapper = search_ orelse return; - wrapper.search.deinit(wrapper.terminal); + if (wrapper.terminal) |terminal_wrapper| { + _ = terminal_wrapper.searches.swapRemove(wrapper); + wrapper.search.deinit(terminal_wrapper.terminal); + } else { + // The terminal was freed first. Tracked state died with it, + // so only search-owned memory is freed. + wrapper.search.deinit(null); + } const alloc = wrapper.alloc; alloc.destroy(wrapper); } @@ -158,26 +187,28 @@ pub fn tick( pub fn feed(search_: Search) callconv(lib.calling_conv) Result { const wrapper = search_ orelse return .invalid_value; + const t = wrapper.zigTerminal() orelse return .invalid_value; // The C API has no renderer cooperation to know whether the active // area changed, so it is always re-scanned. This is correct without // any dirty tracking and cheap because the active area search was // built for exactly this. - wrapper.search.feed(wrapper.terminal, true); + wrapper.search.feed(t, true); return .success; } pub fn run(search_: Search) callconv(lib.calling_conv) Result { const wrapper = search_ orelse return .invalid_value; + const t = wrapper.zigTerminal() orelse return .invalid_value; // Always start with a feed: complete only means caught up as of // the last feed, so run doubles as the "terminal changed, catch // up" convenience for one-shot embedders. - wrapper.search.feed(wrapper.terminal, true); + wrapper.search.feed(t, true); while (true) { switch (wrapper.search.status()) { .complete => return .success, - .feed_required => wrapper.search.feed(wrapper.terminal, true), + .feed_required => wrapper.search.feed(t, true), .running => _ = wrapper.search.tick(), } } @@ -215,8 +246,9 @@ fn setTyped( // The value is reserved for future use and must be NULL. if (value != null) return .invalid_value; + const t = wrapper.zigTerminal() orelse return .invalid_value; const selected = wrapper.search.select( - wrapper.terminal, + t, switch (option) { .select_next => .next, .select_prev => .prev, @@ -839,6 +871,84 @@ test "search new validates options" { )); } +test "search free after terminal free" { + var terminal: terminal_c.Terminal = null; + try testing.expectEqual(Result.success, terminal_c.new( + &lib.alloc.test_allocator, + &terminal, + 10, + 4, + )); + + terminal_c.vt_write(terminal, "Fizz\r\nFizz", 10); + + var search: Search = null; + const opts: Options = .{ .needle = testString("Fizz") }; + try testing.expectEqual(Result.success, new( + &lib.alloc.test_allocator, + &search, + terminal, + &opts, + )); + + // Run and select so the search holds tracked pins within the + // terminal's page storage. + try testing.expectEqual(Result.success, run(search)); + try testing.expectEqual(Result.success, set(search, .select_next, null)); + + // Free the terminal first. The search detaches: calls that need + // the terminal fail cleanly instead of touching freed memory. + terminal_c.free(terminal); + try testing.expectEqual(Result.invalid_value, feed(search)); + try testing.expectEqual(Result.invalid_value, run(search)); + try testing.expectEqual(Result.invalid_value, set(search, .select_next, null)); + + // Reads that only touch search-owned state still answer. + var needle_out: lib.String = undefined; + try testing.expectEqual(Result.success, get(search, .needle, &needle_out)); + try testing.expectEqualStrings("Fizz", needle_out.ptr[0..needle_out.len]); + const scroll: Scroll = .none; + try testing.expectEqual(Result.success, set(search, .select_scroll, &scroll)); + + // The search can still be freed, releasing only its own memory. + free(search); +} + +test "search freed before terminal detaches from the registry" { + var terminal: terminal_c.Terminal = null; + try testing.expectEqual(Result.success, terminal_c.new( + &lib.alloc.test_allocator, + &terminal, + 10, + 4, + )); + defer terminal_c.free(terminal); + + // Create two searches and free one while the terminal is alive. + // The freed search must be unregistered so the later terminal free + // only detaches the survivor. + const opts: Options = .{ .needle = testString("Fizz") }; + var a: Search = null; + try testing.expectEqual(Result.success, new( + &lib.alloc.test_allocator, + &a, + terminal, + &opts, + )); + var b: Search = null; + try testing.expectEqual(Result.success, new( + &lib.alloc.test_allocator, + &b, + terminal, + &opts, + )); + defer free(b); + + try testing.expectEqual(Result.success, run(a)); + free(a); + try testing.expectEqual(Result.success, run(b)); +} + test "search free null" { free(null); } diff --git a/src/terminal/c/terminal.zig b/src/terminal/c/terminal.zig index 70d7ab849..5bf2c750c 100644 --- a/src/terminal/c/terminal.zig +++ b/src/terminal/c/terminal.zig @@ -24,6 +24,7 @@ const cell_c = @import("cell.zig"); const row_c = @import("row.zig"); const grid_ref_c = @import("grid_ref.zig"); const grid_ref_tracked_c = @import("grid_ref_tracked.zig"); +const search_c = @import("search.zig"); const selection_c = @import("selection.zig"); const style_c = @import("style.zig"); const color = @import("../color.zig"); @@ -113,6 +114,7 @@ const TerminalWrapper = struct { stream: Stream, effects: Effects = .{}, tracked_grid_refs: std.AutoArrayHashMapUnmanaged(*grid_ref_tracked_c.TrackedGridRef, void) = .{}, + searches: std.AutoArrayHashMapUnmanaged(*search_c.SearchWrapper, void) = .{}, /// Fetches a `TerminalWrapper` reference from a `Handler`. fn fromHandler(handler: *Handler) *TerminalWrapper { @@ -1855,6 +1857,8 @@ pub fn free(terminal_: Terminal) callconv(lib.calling_conv) void { for (wrapper.tracked_grid_refs.keys()) |ref| ref.terminal = null; wrapper.tracked_grid_refs.deinit(alloc); + for (wrapper.searches.keys()) |search| search.terminal = null; + wrapper.searches.deinit(alloc); wrapper.stream.deinit(); t.deinit(alloc); wrapper.io.deinit(alloc); diff --git a/src/terminal/search/terminal.zig b/src/terminal/search/terminal.zig index eca292778..062cc0049 100644 --- a/src/terminal/search/terminal.zig +++ b/src/terminal/search/terminal.zig @@ -101,21 +101,23 @@ pub const TerminalSearch = struct { }; } - /// Release all state, including tracked pins held within the - /// terminal, so this must be called before the terminal is - /// deinitialized. The terminal must be the same one given to - /// every other call. - pub fn deinit(self: *TerminalSearch, t: *Terminal) void { + /// Release all state. The terminal must be the same one given to + /// every other call, or null if the terminal has already been + /// deinitialized. When the terminal is alive this releases tracked + /// pins held within it. When it is null, those pins died with the + /// terminal's page storage, so only search-owned memory is freed. + pub fn deinit(self: *TerminalSearch, t_: ?*Terminal) void { self.clearViewportMatches(); self.viewport_matches.deinit(self.alloc); self.viewport.deinit(); var it = self.screens.iterator(); while (it.next()) |entry| { - if (self.screenIsValid( + const valid = if (t_) |t| self.screenIsValid( &t.screens, entry.key, entry.value, - )) { + ) else false; + if (valid) { entry.value.deinit(); } else { entry.value.deinitScreenInvalid(); @@ -607,6 +609,38 @@ test "select scrolls the viewport only when needed" { try testing.expect(!Visible.check(&t, &search)); } +test "deinit after the terminal is gone" { + const alloc = testing.allocator; + const io = testing.io; + var t: Terminal = try .init(io, alloc, .{ + .cols = 10, + .rows = 2, + .max_scrollback_bytes = std.math.maxInt(usize), + }); + + var stream = t.vtStream(); + stream.nextSlice("Fizz\r\nBuzz\r\nFizz"); + + // Run to complete and select a match so the search holds tracked + // pins within the terminal's page storage. + var search: TerminalSearch = try .init(alloc, "Fizz"); + while (search.status() != .complete) { + switch (search.status()) { + .feed_required => search.feed(&t, true), + .running => _ = search.tick(), + .complete => unreachable, + } + } + try testing.expect(try search.select(&t, .next, .none)); + + // Deinitialize the terminal first. The pins died with the page + // storage, so deinit with a null terminal must free only + // search-owned memory without touching the terminal. + stream.deinit(); + t.deinit(alloc); + search.deinit(null); +} + test "no matches selects nothing" { const alloc = testing.allocator; const io = testing.io;