core: fix use-after-free in Surface.setSelection (#12894)
`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.
---
_AI Disclosure_: Claude Opus 4.8 found this in a review while I was
working on adjacent code.
pull/11173/merge
commit
6246c288ae
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -7652,6 +7652,32 @@ test "Screen: select untracked" {
|
|||
try testing.expectEqual(tracked, s.pages.countTrackedPins());
|
||||
}
|
||||
|
||||
test "Screen: select replaces existing pins" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
||||
var s = try init(alloc, .{ .cols = 10, .rows = 10, .max_scrollback = 0 });
|
||||
defer s.deinit();
|
||||
try s.testWriteString("ABC DEF\n 123\n456");
|
||||
|
||||
const tracked = s.pages.countTrackedPins();
|
||||
try s.select(Selection.init(
|
||||
s.pages.pin(.{ .active = .{ .x = 0, .y = 0 } }).?,
|
||||
s.pages.pin(.{ .active = .{ .x = 3, .y = 0 } }).?,
|
||||
false,
|
||||
));
|
||||
try testing.expectEqual(tracked + 2, s.pages.countTrackedPins());
|
||||
|
||||
// Replacing the selection must untrack the prior selection's pins
|
||||
// rather than leak them.
|
||||
try s.select(Selection.init(
|
||||
s.pages.pin(.{ .active = .{ .x = 0, .y = 1 } }).?,
|
||||
s.pages.pin(.{ .active = .{ .x = 2, .y = 1 } }).?,
|
||||
false,
|
||||
));
|
||||
try testing.expectEqual(tracked + 2, s.pages.countTrackedPins());
|
||||
}
|
||||
|
||||
test "Screen: selectAll" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
|
|
|||
Loading…
Reference in New Issue