From ab82b8ab720ce46183a58a55554e4a4a7423e3f5 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Mon, 1 Jun 2026 19:58:20 -0400 Subject: [PATCH] core: fix use-after-free in Surface.setSelection setSelection captured the previous selection, then called Screen.select (which deinits the previous selection's tracked pins), then compared the new selection against the now-freed previous pin via `sel.eql(prev)`. That read freed pin memory (use-after-free). The comparison was a copy-on-select optimization ("only re-copy if the selection changed"). Remove it rather than repair it because: - It never fired correctly. It compared against freed memory, so the shipped behavior was already "always copy". - It can't be repaired by copying `prev`'s pin before Screen.select. That fixes the use-after-free but not the logic: the call sites (e.g. mouse drag release) pass a selection equal to the one already set, so a working `eql` skip would suppress the very copy those sites exist to perform. A correct optimization would have to compare against the last-copied selection (before the mouse event mutated the live one), which would require extra state. - It isn't worth tracking that additional state. The copy runs once per selection gesture (mouse up, double-click), which isn't in a hot path, so skipping a redundant re-copy only saves a single clipboard write. Removing the skip eliminates the use-after-free and keeps the behavior consistent with what we've already been doing. --- src/Surface.zig | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 410f717b0..99c740c89 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -2330,18 +2330,14 @@ fn copySelectionToClipboards( /// /// This must be called with the renderer mutex held. fn setSelection(self: *Surface, sel_: ?terminal.Selection) !void { - const prev_ = self.io.terminal.screens.active.selection; try self.io.terminal.screens.active.select(sel_); // If copy on select is false then exit early. if (self.config.copy_on_select == .false) return; // Set our selection clipboard. If the selection is cleared we do not - // clear the clipboard. If the selection is set, we only set the clipboard - // again if it changed, since setting the clipboard can be an expensive - // operation. + // clear the clipboard. const sel = sel_ orelse return; - if (prev_) |prev| if (sel.eql(prev)) return; switch (self.config.copy_on_select) { .false => unreachable, // handled above with an early exit