Fix use-after-move memory corruption in Wayland backends#238
Fix use-after-move memory corruption in Wayland backends#238paperbenni wants to merge 1 commit intomainfrom
Conversation
- In `winit.rs`, heap-allocate `Wm` with `Box::new` so its address does not change when moved into the `event_loop.run` closure. This prevents `WaylandState.globals` from being a dangling pointer. - In `drm.rs`, allocate `Wm` in `Rc<RefCell<Wm>>` before obtaining a reference to `wm.g` to avoid the same dangling pointer issue. - Fix `drm.rs` `run_event_loop` borrowing issue by accepting `Rc<RefCell<Wm>>` instead of taking a long-lived `RefMut`, allowing event sources like libinput to access it without a `BorrowMutError`. - Update `WaylandBackend::state` to hold an `Option<NonNull<...>>` so that calling `with_state` before initialization doesn't segfault. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideThis PR fixes a use-after-move / dangling-pointer bug in the Wayland DRM and winit runtimes by ensuring Wm is heap-allocated before its internals are referenced from long-lived Wayland state, adjusts the event loop API to work with Rc<RefCell> to avoid overlapping borrows with libinput callbacks, and makes WaylandBackend’s stored compositor state pointer optional instead of dangling, preventing test panics when no state is attached. Sequence diagram for run_event_loop with Rc<RefCell> and Wayland statesequenceDiagram
participant DRMRun as drm_run
participant WmCell as Rc_RefCell_Wm
participant WmInner as Wm
participant WlBackend as WaylandBackend
participant WlState as WaylandState
participant EventLoop as EventLoop_WaylandState
participant Libinput as LibinputCallback
DRMRun->>WmCell: new Rc<RefCell<Wm>>(Wm::new(WmBackend::new_wayland(WaylandBackend::new())))
DRMRun->>WmCell: borrow_mut()
activate WmCell
WmCell-->>DRMRun: &mut WmInner
DRMRun->>WmInner: backend.wayland_data_mut()
DRMRun->>WmInner: init_wayland_globals(&mut g, wayland)
deactivate WmCell
DRMRun->>WlState: WaylandState::new(display, loop_handle)
DRMRun->>WmCell: borrow_mut()
activate WmCell
WmCell-->>DRMRun: &mut WmInner
DRMRun->>WmInner: globals_ptr = &mut g as *mut Globals
DRMRun->>WlState: attach_globals(unsafe { &mut *globals_ptr })
DRMRun->>WmInner: if Wayland backend: backend.attach_state(&mut WlState)
deactivate WmCell
DRMRun->>EventLoop: run_event_loop(EventLoop, Rc::clone(&WmCell), &mut WlState, ...)
loop every_frame
EventLoop->>WlState: run(timeout, &mut WlState, closure)
activate EventLoop
EventLoop->>WmCell: borrow_mut()
activate WmCell
WmCell-->>EventLoop: &mut WmInner
EventLoop->>WlState: process_completed_crtcs(...)
EventLoop->>WmInner: arrange_layout_if_dirty(&mut WmInner, &mut WlState)
EventLoop->>WmInner: process_ipc(ipc_server, &mut WmInner, shared)
EventLoop->>WmInner: process_animations(&mut WmInner, &mut WlState, shared)
EventLoop->>WlState: process_cursor_warp(&mut WlState, pointer_handle, shared)
EventLoop->>WmInner: render_outputs(&mut WmInner, &mut WlState, renderer, outputs, shared)
deactivate WmCell
deactivate EventLoop
Libinput-->>EventLoop: callbacks may access WlState
end
WlBackend->>WlBackend: with_state(f)
WlBackend->>WlBackend: state.borrow()
alt state is Some(ptr)
WlBackend-->>WlState: f(ptr.as_mut())
else state is None
WlBackend-->>WlBackend: return None (no dangling NonNull)
end
Updated class diagram for WaylandBackend and Wm lifetime handlingclassDiagram
class WaylandBackend {
- state: RefCell~Option~NonNull~WaylandState~~~
+ new() WaylandBackend
+ attach_state(state: &mut WaylandState) void
+ close_window(window: WindowId) bool
+ with_state<T>(f: FnOnce(&mut WaylandState) -> T) Option~T~
}
class WaylandState {
+ new(display: Display~WaylandState~, handle: LoopHandle~WaylandState~) WaylandState
+ attach_globals(globals: &mut Globals) void
}
class WmBackend {
<<enum>>
+ Wayland(data: WaylandBackendData)
+ new_wayland(backend: WaylandBackend) WmBackend
+ wayland_data_mut(&mut self) Option~&mut WaylandBackendData~
}
class WaylandBackendData {
+ backend: WaylandBackend
+ wayland_systray_runtime: Option~WaylandSystrayRuntime~
+ attach_state(state: &mut WaylandState) void
}
class Globals {
+ dirty: DirtyFlags
}
class DirtyFlags {
+ input_config: bool
}
class Wm {
+ g: Globals
+ backend: WmBackend
+ ctx(&mut self) Ctx
+ new(backend: WmBackend) Wm
}
class Rc~T~ {
+ clone(self) Rc~T~
}
class RefCell~T~ {
+ borrow(&self) Ref~T~
+ borrow_mut(&self) RefMut~T~
}
class Box~T~ {
+ new(value: T) Box~T~
}
class EventLoop~T~ {
+ try_new() Result~EventLoop~T~~
+ run(timeout: Duration, state: &mut T, f: FnMut(&mut T)) !
}
class WaylandStatePtr {
+ inner: RefCell~Option~NonNull~WaylandState~~~
}
WaylandBackend --> WaylandState : holds NonNull pointer (Option)
WaylandBackendData --> WaylandBackend : has backend
Wm --> WmBackend : has
Wm --> Globals : has
Globals --> DirtyFlags : has
Rc~Wm~ --> RefCell~Wm~ : wraps
RefCell~Wm~ --> Wm : interior mutability
Box~Wm~ --> Wm : heap allocation
EventLoop~WaylandState~ --> WaylandState : manages lifecycle
WmBackend ..> WaylandBackendData : variant Wayland
WaylandBackend ..> NonNull~WaylandState~ : optional raw pointer
WaylandBackend ..> WaylandStatePtr : wrapped pointer state
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis change refactors pointer and memory management in the Wayland compositor backend. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
globals_ptrhandling indrm::runcreates a long-lived&mut WaylandGlobalsviaunsafeafter aRefCell::borrow_muthas been dropped, which can violateRefCell’s borrowing rules and alias with laterborrow_mutcalls onwm_cell; consider keeping theRefMutalive for as long asWaylandStateneeds access (or otherwise restructuring so you don’t manufacture an independent&mutfrom aRefCell-managed value). - The
Rc<RefCell<Wm>>passed intorun_event_loopis consumed by value and then cloned multiple times (e.g. for the closure), which is unnecessary given it’s already anRc; taking&Rc<RefCell<Wm>>or cloning once at the call site would make ownership and refcounting clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `globals_ptr` handling in `drm::run` creates a long-lived `&mut WaylandGlobals` via `unsafe` after a `RefCell::borrow_mut` has been dropped, which can violate `RefCell`’s borrowing rules and alias with later `borrow_mut` calls on `wm_cell`; consider keeping the `RefMut` alive for as long as `WaylandState` needs access (or otherwise restructuring so you don’t manufacture an independent `&mut` from a `RefCell`-managed value).
- The `Rc<RefCell<Wm>>` passed into `run_event_loop` is consumed by value and then cloned multiple times (e.g. for the closure), which is unnecessary given it’s already an `Rc`; taking `&Rc<RefCell<Wm>>` or cloning once at the call site would make ownership and refcounting clearer.
## Individual Comments
### Comment 1
<location path="src/wayland/runtime/drm.rs" line_range="64-68" />
<code_context>
- state.attach_globals(&mut wm.g);
- if let WmBackend::Wayland(data) = &mut wm.backend {
+
+ // Unsafe: Obtain a pointer to the inner Globals and extend its lifetime.
+ // This is safe because the `Wm` instance is allocated on the heap inside `Rc<RefCell<Wm>>`
+ // and will not move.
+ let globals_ptr = &mut wm_cell.borrow_mut().g as *mut _;
+ state.attach_globals(unsafe { &mut *globals_ptr });
+
+ if let WmBackend::Wayland(data) = &mut wm_cell.borrow_mut().backend {
</code_context>
<issue_to_address>
**issue (bug_risk):** The raw pointer/lifetime extension for `wm.g` can violate aliasing and `RefCell` borrowing guarantees.
This pattern takes a `&mut Globals` out of `RefCell<Wm>`’s borrow tracking and Rust’s aliasing model. If `attach_globals` stores that `&mut Globals` for later use (e.g., across event-loop iterations), later `wm_cell.borrow_mut()` calls can overlap mutable access to `wm.g` (and other fields) while the stored `&mut` is still live, which is UB even if `RefCell` doesn’t panic.
Prefer instead:
- having `WaylandState` hold an `Rc<RefCell<Globals>>`/`Rc<Wm>` and borrow when needed, or
- changing `attach_globals` to take a callback/handle instead of a long-lived `&mut Globals`.
If `attach_globals` uses the reference only within the call and does not store it, you don’t need the raw pointer or `unsafe`; just call `state.attach_globals(&mut wm_cell.borrow_mut().g);` in a small scope so the borrow ends promptly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Unsafe: Obtain a pointer to the inner Globals and extend its lifetime. | ||
| // This is safe because the `Wm` instance is allocated on the heap inside `Rc<RefCell<Wm>>` | ||
| // and will not move. | ||
| let globals_ptr = &mut wm_cell.borrow_mut().g as *mut _; | ||
| state.attach_globals(unsafe { &mut *globals_ptr }); |
There was a problem hiding this comment.
issue (bug_risk): The raw pointer/lifetime extension for wm.g can violate aliasing and RefCell borrowing guarantees.
This pattern takes a &mut Globals out of RefCell<Wm>’s borrow tracking and Rust’s aliasing model. If attach_globals stores that &mut Globals for later use (e.g., across event-loop iterations), later wm_cell.borrow_mut() calls can overlap mutable access to wm.g (and other fields) while the stored &mut is still live, which is UB even if RefCell doesn’t panic.
Prefer instead:
- having
WaylandStatehold anRc<RefCell<Globals>>/Rc<Wm>and borrow when needed, or - changing
attach_globalsto take a callback/handle instead of a long-lived&mut Globals.
If attach_globals uses the reference only within the call and does not store it, you don’t need the raw pointer or unsafe; just call state.attach_globals(&mut wm_cell.borrow_mut().g); in a small scope so the borrow ends promptly.
![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/wayland/runtime/drm.rs`:
- Around line 64-68: attach_globals currently stores a transmuted &'static mut
Globals, causing UB from overlapping mutable borrows when run_event_loop holds
wm_cell.borrow_mut() while code calls state.globals_mut(); instead, change
WaylandState's storage for Globals to avoid creating a &'static mutable
reference (e.g., store a raw pointer wrapped in Cell/UnsafeCell or an
Arc<Mutex/RefCell<Globals>> or store a Weak<Rc<RefCell<Globals>>> and upgrade
when needed) and update attach_globals, globals_mut, and any callers (including
run_event_loop, arrange_layout_if_dirty and code in winit.rs) to access Globals
through that safe interior-mutability/ownership API rather than via transmute to
&'static mut. Ensure attach_globals no longer transmutes to a static lifetime
and that globals_mut returns a borrow or lock guard tied to the chosen
interior-mutable container to prevent overlapping mutable borrows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81b99ffd-0a16-49f4-9067-79d20a637df8
📒 Files selected for processing (3)
src/backend/wayland/mod.rssrc/wayland/runtime/drm.rssrc/wayland/runtime/winit.rs
| // Unsafe: Obtain a pointer to the inner Globals and extend its lifetime. | ||
| // This is safe because the `Wm` instance is allocated on the heap inside `Rc<RefCell<Wm>>` | ||
| // and will not move. | ||
| let globals_ptr = &mut wm_cell.borrow_mut().g as *mut _; | ||
| state.attach_globals(unsafe { &mut *globals_ptr }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust, is it sound to take &mut fieldfromRefCell::borrow_mut(), cast it to *mut T, store it as a long-lived &'static mut T, and later borrow the parent RefCell mutably again?
💡 Result:
No, it is not sound. Taking &mut field from RefCell::borrow_mut, casting it to *mut T, and storing it as a long-lived &'static mut T (via &mut *ptr as &'static mut T or similar) bypasses RefCell's runtime borrow tracking. The RefMut guard drops and clears the mutable borrow flag, allowing subsequent borrow_mut calls on the RefCell to succeed at runtime without panicking. However, dereferencing the long-lived &'static mut T creates a mutable reference while another mutable borrow (from the new borrow_mut) is active, violating Rust's aliasing rules and causing undefined behavior.
Citations:
- 1: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.borrow
- 2: https://doc.rust-lang.org/nightly/std/cell/struct.RefMut.html
- 3: https://stackoverflow.com/questions/57364654/do-aliasing-mutable-raw-pointers-mut-t-cause-undefined-behaviour
- 4: https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html
- 5: Tracking Issue for
std::cell::{Ref, RefMut}::leakrust-lang/rust#69099
🏁 Script executed:
# First, let's find the WaylandState struct and its attach_globals method
fd -t f "\.rs$" src/wayland | head -20Repository: instantOS/instantWM
Length of output: 634
🏁 Script executed:
# Search for attach_globals method definition
rg -A 10 "fn attach_globals" --type rustRepository: instantOS/instantWM
Length of output: 1127
🏁 Script executed:
# Find the run_event_loop function
rg -A 15 "fn run_event_loop" --type rustRepository: instantOS/instantWM
Length of output: 1111
🏁 Script executed:
# Check winit.rs for similar patterns
fd -t f "winit\.rs$"Repository: instantOS/instantWM
Length of output: 145
🏁 Script executed:
# Look at the full drm.rs file context to understand the issue
wc -l src/wayland/runtime/drm.rsRepository: instantOS/instantWM
Length of output: 93
🏁 Script executed:
# Get the full WaylandState struct definition
rg -B 5 -A 30 "struct WaylandState" src/backend/wayland/compositor/state.rsRepository: instantOS/instantWM
Length of output: 1505
🏁 Script executed:
# Check the globals field type and how it's accessed
rg -A 5 "fn globals_mut" src/backend/wayland/compositor/state.rsRepository: instantOS/instantWM
Length of output: 275
🏁 Script executed:
# Look at the context around run_event_loop to see borrow patterns
rg -A 50 "fn run_event_loop" src/wayland/runtime/drm.rs | head -80Repository: instantOS/instantWM
Length of output: 1928
🏁 Script executed:
# Check winit.rs runtime for similar patterns
rg -B 5 -A 15 "attach_globals\|run_event_loop" src/wayland/runtime/winit.rsRepository: instantOS/instantWM
Length of output: 45
🏁 Script executed:
# Find the globals field definition in WaylandState
rg "globals:" src/backend/wayland/compositor/state.rsRepository: instantOS/instantWM
Length of output: 223
🏁 Script executed:
# Look for all usages of globals_mut in the codebase
rg "globals_mut()" src/wayland/ -A 3Repository: instantOS/instantWM
Length of output: 2181
🏁 Script executed:
# Check the full WaylandState struct to find globals field
sed -n '/^pub struct WaylandState/,/^}/p' src/backend/wayland/compositor/state.rs | grep -A 2 -B 2 "globals"Repository: instantOS/instantWM
Length of output: 302
🏁 Script executed:
# Look at winit.rs runtime initialization to see if it has the same pattern
cat -n src/wayland/runtime/winit.rs | head -100Repository: instantOS/instantWM
Length of output: 4469
🏁 Script executed:
# Check if arrange_layout_if_dirty or other event loop functions call globals_mut
rg "globals_mut()" src/wayland/runtime/common.rs -B 5 -A 5Repository: instantOS/instantWM
Length of output: 447
🏁 Script executed:
# Look at the event loop closure more carefully - see all calls
sed -n '28,100p' src/wayland/runtime/drm.rsRepository: instantOS/instantWM
Length of output: 2634
🏁 Script executed:
# Check if ctx operations during event loop could call globals_mut
rg "arrange_layout_if_dirty|process_ipc|process_animations" src/wayland/runtime/drm.rs -A 3Repository: instantOS/instantWM
Length of output: 1011
🏁 Script executed:
# Verify that ctx.core_mut() or similar gives access to globals_mut
rg "fn core_mut|fn globals_mut" src/ -B 2 -A 5Repository: instantOS/instantWM
Length of output: 1248
🏁 Script executed:
# Check if winit.rs really has the same issue - look for Rc<RefCell> or attach_globals patterns
rg "Rc<RefCell<Wm>>|attach_globals" src/wayland/runtime/winit.rsRepository: instantOS/instantWM
Length of output: 99
🏁 Script executed:
# Also check winit init path more carefully
cat -n src/wayland/runtime/winit.rs | sed -n '30,50p'Repository: instantOS/instantWM
Length of output: 1006
🏁 Script executed:
# Verify the event loop in winit and see if it keeps wm alive
rg "event_loop.run|run_app" src/wayland/runtime/winit.rs -A 20Repository: instantOS/instantWM
Length of output: 45
🏁 Script executed:
# Check the full winit event loop to see if wm is kept alive during event processing
sed -n '100,200p' src/wayland/runtime/winit.rsRepository: instantOS/instantWM
Length of output: 3305
🏁 Script executed:
# Look for the event_loop.run closure in winit
rg -A 30 "event_loop.run\|run_app" src/wayland/runtime/winit.rsRepository: instantOS/instantWM
Length of output: 45
attach_globals is creating undefined behavior through aliased mutable references.
WaylandState::attach_globals() uses transmute to store the reference as &'static mut Globals, but the actual Globals lives within Rc<RefCell<Wm>>. During run_event_loop(), the event closure keeps let mut wm = wm_cell.borrow_mut() alive while calling arrange_layout_if_dirty(), which internally calls state.globals_mut() to dereference the stored 'static reference. This creates overlapping mutable references to wm.g — one from the active borrow_mut() and one from dereferencing the transmuted pointer — violating Rust's aliasing rules.
The fix requires changing how WaylandState stores Globals (e.g., using interior mutability or a different ownership model) rather than just adjusting how the pointer is obtained. This same storage pattern affects src/wayland/runtime/winit.rs as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wayland/runtime/drm.rs` around lines 64 - 68, attach_globals currently
stores a transmuted &'static mut Globals, causing UB from overlapping mutable
borrows when run_event_loop holds wm_cell.borrow_mut() while code calls
state.globals_mut(); instead, change WaylandState's storage for Globals to avoid
creating a &'static mutable reference (e.g., store a raw pointer wrapped in
Cell/UnsafeCell or an Arc<Mutex/RefCell<Globals>> or store a
Weak<Rc<RefCell<Globals>>> and upgrade when needed) and update attach_globals,
globals_mut, and any callers (including run_event_loop, arrange_layout_if_dirty
and code in winit.rs) to access Globals through that safe
interior-mutability/ownership API rather than via transmute to &'static mut.
Ensure attach_globals no longer transmutes to a static lifetime and that
globals_mut returns a borrow or lock guard tied to the chosen interior-mutable
container to prevent overlapping mutable borrows.
1 participant
Fixes memory corruption caused by dangling pointers to
wm.gin theWaylandStateglobals.When
Wmwas allocated on the stack and then moved into the closure forevent_loop.run()or into aRc<RefCell<Wm>>, its memory address changed, causingstate.globalsto become a dangling pointer. By allocatingWmon the heap first, we ensure its memory address is stable before acquiring the pointer.I also modified
WaylandBackend::stateto store anOption<NonNull<WaylandState>>instead of unconditionally usingNonNull::dangling()which caused test panics. Finally, I fixedrun_event_loopto take anRc<RefCell<Wm>>to prevent long-livedRefMuts from overlapping with libinput callback accesses.PR created automatically by Jules for task 12150865575506268394 started by @paperbenni
Summary by Sourcery
Fix use-after-move and dangling state pointers in Wayland backends and event loop wiring to prevent memory corruption and test panics.
Bug Fixes:
Wminstance in the DRM/KMS Wayland runtime to avoid dangling pointers fromWaylandStateglobals.WaylandBackendby storing it as anOption<NonNull<WaylandState>>and handling the empty case safely.Wmwith libinput callbacks in the DRM event loop by passing anRc<RefCell<Wm>>intorun_event_loop.Summary by CodeRabbit