Fix use-after-free in copy-on-select mouse release path#36
Fix use-after-free in copy-on-select mouse release path#36wzoom wants to merge 1 commit intomanaflow-ai:mainfrom
Conversation
Override copy-on-select = clipboard so that selecting text in the terminal copies to the system clipboard (NSPasteboard.general), matching macOS user expectations. Ghostty's default of .true writes to a private named pasteboard that is not accessible via Cmd+V. Update the ghostty submodule to include the fix for the use-after-free in the mouse release copy-on-select path (manaflow-ai/ghostty#36). Update docs/ghostty-fork.md with section 8 documenting the fork change. Fixes manaflow-ai#2664
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughsetSelection now captures previous-selection equality before calling Changes
Sequence Diagram(s)sequenceDiagram
participant Mouse
participant Surface
participant Screen
participant Renderer
participant Clipboard
Mouse->>Surface: left-button release event
Surface->>Screen: read prev selection, compute `same`
Surface->>Screen: Screen.select(sel_) --(calls)-->
Note right of Screen: selection may reallocate/deinit old pins
Surface->>Renderer: lock renderer mutex
Renderer->>Surface: held
Surface->>Clipboard: copySelectionToClipboards(active.selection, targets)
Clipboard-->>Surface: copy complete
Surface->>Renderer: unlock mutex
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a bug where
Confidence Score: 4/5Safe to merge as a targeted fix, but the root use-after-free in The mouse release fix is correct and well-reasoned. One P1 remains: the UAF in src/Surface.zig — specifically
|
| Filename | Overview |
|---|---|
| src/Surface.zig | Mouse release path correctly bypasses setSelection() to fix the copy-on-select UAF, but the root use-after-free in setSelection() (line 2377) remains and still affects other callers. |
Sequence Diagram
sequenceDiagram
participant U as User
participant MBC as mouseButtonCallback
participant SS as setSelection()
participant Scr as Screen.select()
participant CTC as copySelectionToClipboards()
Note over U,CTC: BEFORE FIX (broken path)
U->>MBC: left mouse release (drag selection)
MBC->>SS: setSelection(Selection.init(start, end))
SS->>Scr: Screen.select(new_sel)
Scr-->>Scr: old.deinit() — frees tracked pins
SS->>SS: eql(prev) — USE-AFTER-FREE on freed *Pin
SS-->>MBC: returns early (eql=true), clipboard never copied
Note over U,CTC: AFTER FIX (correct path)
U->>MBC: left mouse release (drag selection)
MBC->>MBC: renderer_state.mutex.lock()
MBC->>MBC: read screens.active.selection (existing tracked sel)
MBC->>CTC: copySelectionToClipboards(sel, clipboards, .mixed)
CTC-->>MBC: clipboard updated
MBC->>MBC: mutex.unlock() (deferred)
Comments Outside Diff (1)
-
src/Surface.zig, line 2365-2377 (link)Use-after-free in
setSelection()is not fixedsetSelection()itself still has the UAF. At line 2367,Screen.select()callsold.deinit(self)which frees the tracked pins (s.pages.untrackPin(v.start),untrackPin(v.end)). Line 2377 then callssel.eql(prev), which dispatches toprev.start()→v.start.*(dereferencing the freed pointer).This PR correctly bypasses
setSelection()for the mouse release path, butsetSelection()is still called with a potentially-equal non-null selection fromselectCursorCell()(line 2066) and from right-click context menu word selection (lines 4116, 4122). In a Zig safety-checked build,untrackPinmay poison the freed slot, causing theeqlcheck to misbehave or panic.A minimal fix is to evaluate
eqlbefore freeing old pins:fn setSelection(self: *Surface, sel_: ?terminal.Selection) !void { // Capture equality BEFORE Screen.select() frees the old pins. const prev_ = self.io.terminal.screens.active.selection; const same = if (prev_) |prev| if (sel_) |sel| sel.eql(prev) else false else false; try self.io.terminal.screens.active.select(sel_); if (self.config.copy_on_select == .false) return; if (sel_ == null) return; if (same) return; switch (self.config.copy_on_select) { ... } }
Reviews (1): Last reviewed commit: "Fix use-after-free in copy-on-select mou..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (self.config.copy_on_select != .false) { | ||
| self.renderer_state.mutex.lock(); | ||
| defer self.renderer_state.mutex.unlock(); | ||
| const prev_ = self.io.terminal.screens.active.selection; | ||
| if (prev_) |prev| { | ||
| try self.setSelection(terminal.Selection.init( | ||
| prev.start(), | ||
| prev.end(), | ||
| prev.rectangle, | ||
| )); | ||
| if (self.io.terminal.screens.active.selection) |sel| { | ||
| switch (self.config.copy_on_select) { | ||
| .false => unreachable, | ||
| .clipboard => try self.copySelectionToClipboards( | ||
| sel, | ||
| &.{ .standard, .selection }, | ||
| .mixed, | ||
| ), | ||
| .true => { | ||
| const clipboard: apprt.Clipboard = if (self.rt_surface.supportsClipboard(.selection)) | ||
| .selection | ||
| else | ||
| .standard; | ||
| try self.copySelectionToClipboards( | ||
| sel, | ||
| &.{clipboard}, | ||
| .mixed, | ||
| ); | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
setSelection()
The switch (self.config.copy_on_select) block here (lines 3852–3870) is identical to the one in setSelection() (lines 2379–2401). Extracting this into a private helper (e.g., copySelectionToConfiguredClipboards) would eliminate the duplication and make future copy_on_select variant changes a single-site edit.
There was a problem hiding this comment.
Valid observation. The two switch blocks are identical. We intentionally kept them duplicated to minimize the fork diff against upstream Ghostty. Extracting a helper would be a clean improvement, but it increases the surface area for merge conflicts when syncing with upstream. If this fix lands upstream, the refactor would be better done there as a follow-up.
There was a problem hiding this comment.
The two switch blocks are identical, yeah. We kept them duplicated on purpose to minimize the fork diff against upstream Ghostty. Extracting a helper would be cleaner but increases the surface area for merge conflicts when syncing with upstream. If this fix lands upstream, the refactor would be better done there.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Surface.zig`:
- Around line 3840-3870: The setSelection() path still snapshots prev_ then
calls Screen.select(sel_) which frees old tracked pins (old.deinit) and later
dereferences prev, causing a use-after-free; move the eql()/clipboard decision
so it runs before calling Screen.select or extract a safe helper that computes
selection equality and determines which clipboards to copy to (reusing
copySelectionToClipboards and the same clipboard-choice logic used in the
mouse-release path), then replace the existing logic in setSelection(), the
mouse-release flow, and other callers (e.g., the blocks around lines referencing
sel_ and prev_) to call that new helper prior to calling Screen.select() to
ensure no access to freed tracked pins after deinit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
wzoom
force-pushed
the
fix-copy-on-select-uaf
branch
from
75ec7bf to
353d9b5
Compare
setSelection() captured the old tracked selection pointer, then called Screen.select() which frees the old tracked pins via old.deinit(). The subsequent eql() check dereferenced the freed pointer. Since the freed memory usually retains its old values, eql() returned true and copySelectionToClipboards was silently skipped — producing the one-character-paste symptom reported in cmux ghostty-org#2664. Fix: evaluate eql() BEFORE Screen.select() frees the old pins. Additionally, copy the final selection to the clipboard directly on mouse-up (left-button release) to guarantee the clipboard is always up-to-date at the end of a drag selection.
wzoom
force-pushed
the
fix-copy-on-select-uaf
branch
from
353d9b5 to
8eaee4d
Compare
1 participant
Summary
mouseButtonCallbackthat silently skipscopySelectionToClipboardson mouse release after a drag selectionsetSelection()eql optimization by reading the existing tracked selection and copying to clipboard directlyProblem
When the left mouse button is released after a drag selection, the mouse release handler calls
setSelection()with a new untrackedSelectionbuilt from the existing tracked selection'sstart()/end().Inside
setSelection():Screen.select()frees the old tracked pins viaold.deinit(self)if (prev_) |prev| if (sel.eql(prev)) return;then dereferences the freedprevpointer (use-after-free)eql()returnstruecopySelectionToClipboardsis never calledThe bug is intermittent: when the pool happens to reuse the freed slots before the
eqlcheck, the comparison fails and the copy succeeds.Fix
The mouse release path doesn't need to change the selection — it only needs to copy it to the clipboard. Read the existing tracked selection directly and call
copySelectionToClipboards, bypassingsetSelection()entirely.Fixes manaflow-ai/cmux#2664
Summary by cubic
Fixes a use-after-free in selection handling and ensures copy-on-select always updates the clipboard on mouse release.
setSelection(), evaluateeqlbeforeScreen.select()frees the old pins to avoid dereferencing a freedprev.copySelectionToClipboards, honoringcopy_on_select(.clipboardcopies to both.standardand.selection;.trueprefers.selectionwith fallback to.standard).Written for commit 8eaee4d. Summary will update on new commits.
Summary by CodeRabbit