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.
pull/12894/head
Jon Parise 2026-06-01 19:58:20 -04:00
parent 0f7cd84b88
commit ab82b8ab72
1 changed files with 1 additions and 5 deletions

View File

@ -2330,18 +2330,14 @@ fn copySelectionToClipboards(
/// ///
/// This must be called with the renderer mutex held. /// This must be called with the renderer mutex held.
fn setSelection(self: *Surface, sel_: ?terminal.Selection) !void { fn setSelection(self: *Surface, sel_: ?terminal.Selection) !void {
const prev_ = self.io.terminal.screens.active.selection;
try self.io.terminal.screens.active.select(sel_); try self.io.terminal.screens.active.select(sel_);
// If copy on select is false then exit early. // If copy on select is false then exit early.
if (self.config.copy_on_select == .false) return; if (self.config.copy_on_select == .false) return;
// Set our selection clipboard. If the selection is cleared we do not // 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 // clear the clipboard.
// again if it changed, since setting the clipboard can be an expensive
// operation.
const sel = sel_ orelse return; const sel = sel_ orelse return;
if (prev_) |prev| if (sel.eql(prev)) return;
switch (self.config.copy_on_select) { switch (self.config.copy_on_select) {
.false => unreachable, // handled above with an early exit .false => unreachable, // handled above with an early exit