From 73ac36fa5679f3990b39cdcae75dfd10816e8ee9 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 9 Jul 2026 23:22:26 -0700 Subject: [PATCH] terminal/search: discard destroyed screen state Search selection could run after the terminal removed a screen but before the next refresh reconciled the cached searchers. Reloading that stale ScreenSearch dereferenced freed PageList nodes, while normal cleanup also tried to untrack pins from the destroyed list. Reconcile under the terminal lock before selecting, track ScreenSet generations so allocator address reuse cannot hide replacements, and release stale search buffers without touching pins already freed with the screen. Cleanup now takes the same lock when live pins must be untracked. --- src/terminal/search/Thread.zig | 100 ++++++++++++++++++++++++++----- src/terminal/search/pagelist.zig | 7 +++ src/terminal/search/screen.zig | 23 ++++++- 3 files changed, 112 insertions(+), 18 deletions(-) diff --git a/src/terminal/search/Thread.zig b/src/terminal/search/Thread.zig index fa09af5f0..d7f0d5625 100644 --- a/src/terminal/search/Thread.zig +++ b/src/terminal/search/Thread.zig @@ -121,7 +121,11 @@ pub fn deinit(self: *Thread) void { // Nothing can possibly access the mailbox anymore, destroy it. self.mailbox.destroy(self.alloc); - if (self.search) |*s| s.deinit(); + if (self.search) |*s| { + self.opts.mutex.lock(); + defer self.opts.mutex.unlock(); + s.deinit(self.opts.terminal); + } } /// The main entrypoint for the thread. @@ -252,11 +256,15 @@ fn drainMailbox(self: *Thread) !void { fn select(self: *Thread, sel: ScreenSearch.Select) !void { const s = if (self.search) |*s| s else return; - const screen_search = s.screens.getPtr(s.last_screen.key) orelse return; self.opts.mutex.lock(); defer self.opts.mutex.unlock(); + // A screen can be removed or replaced between refresh ticks. Reconcile + // while holding the terminal lock before touching any ScreenSearch pins. + s.feed(self.alloc, self.opts.terminal); + const screen_search = s.screens.getPtr(s.last_screen.key) orelse return; + // Make the selection. Ignore the result because we don't // care if the selection didn't change. _ = try screen_search.select(sel); @@ -308,7 +316,11 @@ fn changeNeedle(self: *Thread, needle: []const u8) !void { // If our search is unchanged, do nothing. if (std.ascii.eqlIgnoreCase(s.viewport.needle(), needle)) return; - s.deinit(); + { + self.opts.mutex.lock(); + defer self.opts.mutex.unlock(); + s.deinit(self.opts.terminal); + } self.search = null; // When the search changes then we need to emit that it stopped. @@ -497,6 +509,11 @@ const Search = struct { /// The searchers for all the screens. screens: std.EnumMap(ScreenSet.Key, ScreenSearch), + /// ScreenSet generations captured when each searcher was initialized. + /// Allocators may reuse a destroyed Screen address, so pointer equality + /// alone cannot distinguish replacement screens from stale handles. + screen_generations: std.EnumMap(ScreenSet.Key, usize), + /// All state related to screen switches, collected so that when /// we switch screens it makes everything related stale, too. last_screen: ScreenState, @@ -537,16 +554,35 @@ const Search = struct { return .{ .viewport = vp, .screens = .init(.{}), + .screen_generations = .init(.{}), .last_screen = .{ .key = .primary }, .last_complete = false, .stale_viewport_matches = true, }; } - pub fn deinit(self: *Search) void { + pub fn deinit(self: *Search, t: *Terminal) void { self.viewport.deinit(); var it = self.screens.iterator(); - while (it.next()) |entry| entry.value.deinit(); + while (it.next()) |entry| { + if (self.screenIsValid(&t.screens, entry.key, entry.value)) { + entry.value.deinit(); + } else { + entry.value.deinitScreenInvalid(); + } + } + } + + fn screenIsValid( + self: *const Search, + screens: *const ScreenSet, + key: ScreenSet.Key, + search: *const ScreenSearch, + ) bool { + const generation = self.screen_generations.get(key) orelse return false; + if (generation != screens.generation(key)) return false; + const actual = screens.get(key) orelse return false; + return actual == search.screen; } /// Returns true if all searches on all screens are complete. @@ -629,18 +665,16 @@ const Search = struct { // Remove screens we have that no longer exist or changed. var it = self.screens.iterator(); while (it.next()) |entry| { - const remove: bool = remove: { - // If the screen doesn't exist at all, remove it. - const actual = t.screens.all.get(entry.key) orelse break :remove true; - - // If the screen pointer changed, remove it, the screen - // was totally reinitialized. - break :remove actual != entry.value.screen; - }; + const remove = !self.screenIsValid( + &t.screens, + entry.key, + entry.value, + ); if (remove) { - entry.value.deinit(); + entry.value.deinitScreenInvalid(); _ = self.screens.remove(entry.key); + _ = self.screen_generations.remove(entry.key); } } } @@ -649,7 +683,7 @@ const Search = struct { var it = t.screens.all.iterator(); while (it.next()) |entry| { if (self.screens.contains(entry.key)) continue; - self.screens.put(entry.key, ScreenSearch.init( + const screen_search = ScreenSearch.init( alloc, entry.value.*, self.viewport.needle(), @@ -664,7 +698,12 @@ const Search = struct { ); continue; }, - }); + }; + self.screens.put(entry.key, screen_search); + self.screen_generations.put( + entry.key, + t.screens.generation(entry.key), + ); } } @@ -903,3 +942,32 @@ test { } }, t.screens.active.pages.pointFromPin(.screen, sel.end).?); } } + +test "select after active screen removal" { + const alloc = testing.allocator; + var mutex: std.Thread.Mutex = .{}; + var t: Terminal = try .init(alloc, .{ .cols = 20, .rows = 2 }); + defer t.deinit(alloc); + + _ = try t.switchScreen(.alternate); + + var search: Search = try .init(alloc, "needle"); + search.feed(alloc, &t); + try testing.expectEqual(ScreenSet.Key.alternate, search.last_screen.key); + try testing.expect(search.screens.contains(.alternate)); + + var thread: Thread = undefined; + thread.search = search; + thread.opts = .{ + .mutex = &mutex, + .terminal = &t, + }; + defer if (thread.search) |*active| active.deinit(&t); + + _ = try t.switchScreen(.primary); + t.screens.remove(alloc, .alternate); + + try thread.select(.next); + try testing.expectEqual(ScreenSet.Key.primary, thread.search.?.last_screen.key); + try testing.expect(!thread.search.?.screens.contains(.alternate)); +} diff --git a/src/terminal/search/pagelist.zig b/src/terminal/search/pagelist.zig index 6907417de..98a24c187 100644 --- a/src/terminal/search/pagelist.zig +++ b/src/terminal/search/pagelist.zig @@ -85,6 +85,13 @@ pub const PageListSearch = struct { self.list.untrackPin(self.pin); } + /// Release owned search buffers after the PageList itself has already + /// been destroyed. Its pin belonged to the PageList pool and was freed + /// with that list, so it must not be untracked here. + pub fn deinitListInvalid(self: *PageListSearch) void { + self.window.deinit(); + } + /// Return the next match in the loaded page nodes. If this returns /// null then the PageList search needs to be fed the next node(s). /// Call, `feed` to do this. diff --git a/src/terminal/search/screen.zig b/src/terminal/search/screen.zig index 6b3a487af..448bb98c3 100644 --- a/src/terminal/search/screen.zig +++ b/src/terminal/search/screen.zig @@ -157,10 +157,29 @@ pub const ScreenSearch = struct { } pub fn deinit(self: *ScreenSearch) void { + self.deinitInternal(true); + } + + /// Release owned search state after the underlying Screen has already + /// been destroyed. Tracked pins belonged to that Screen's PageList pool + /// and were freed with it, so only independently owned memory is freed. + pub fn deinitScreenInvalid(self: *ScreenSearch) void { + self.deinitInternal(false); + } + + fn deinitInternal(self: *ScreenSearch, screen_valid: bool) void { const alloc = self.allocator(); self.active.deinit(); - if (self.history) |*h| h.deinit(self.screen); - if (self.selected) |*m| m.deinit(self.screen); + if (self.history) |*h| { + if (screen_valid) { + h.deinit(self.screen); + } else { + h.searcher.deinitListInvalid(); + } + } + if (screen_valid) { + if (self.selected) |*m| m.deinit(self.screen); + } for (self.active_results.items) |*hl| hl.deinit(alloc); self.active_results.deinit(alloc); for (self.history_results.items) |*hl| hl.deinit(alloc);