Repository navigation
Port Wayland backend to wayland-rs 0.31/0.32 - #432
Conversation
The 0.29 line is unmaintained. This moves the Wayland backend to wayland-client 0.31 / wayland-protocols 0.32 / wayland-cursor 0.31, keeping the xkbcommon keyboard path intact. The 0.29 event model (Main<T> plus quick_assign closures) is gone in 0.31; every event now arrives through Dispatch impls on a single state type. WaylandState holds those, queueing the keyboard and pointer events for Window::update to drain, which is where the xkb state and the KeyHandler live. That keeps the level-0 keysym derivation, the modifier tracking, the held-key release bookkeeping and the add_char unicode path exactly as they were; key_level_tests still covers them. wayland-client needs the system feature: raw-window-handle has to hand out a real wl_surface*/wl_display*, and only the libwayland backend can produce those. dlopen still controls whether libwayland is loaded at runtime or linked, so the default build links neither libwayland nor libxkbcommon, exactly as before. Fixes carried by the rewrite: - Buffers track release properly. The old flag was set on wl_buffer::Release and never cleared, and a resize dropped the new flag on the floor, so from the third frame on a buffer was reused whether or not the compositor had released it. The flag is now the buffer's dispatch user data, cleared on attach. - With every buffer held and the pool at its cap, the frame is dropped rather than written into a buffer the compositor may still be scanning out. - The shm file is sized before the pool is created and grown before the pool is; touching a mapping past EOF is a SIGBUS. - The event channel is gone. sync_channel(1024) was filled and drained from the same thread, so an event burst could block dispatch with nothing draining it. Events queue in a Vec instead. - The keymap fd is an OwnedFd and is closed. A keymap change unrefs the previous keymap and state rather than leaking them. - A malformed or non-XKB keymap is an error, not a panic. A compositor disconnect during update() closes the window instead of panicking. - Surface size arithmetic is overflow-checked, both the scale multiply and the pool byte count. - get_position returns (0, 0) with a comment; Wayland does not tell a client where its surface is. Drops nix 0.24, memoffset, wayland-commons and xml-rs; no duplicate crate versions remain in the graph.
There was a problem hiding this comment.
🟡 Changes recommended
Unchecked size arithmetic and incomplete flush-error handling can still cause crashes or miss compositor disconnects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Ports the Wayland backend to current wayland-rs APIs while improving event handling, buffer safety, and error recovery.
Changes:
- Replaces callbacks/channels with centralized
Dispatchstate. - Fixes shared-memory buffer lifecycle and keymap resource handling.
- Updates Wayland dependencies and native-handle support.
File summaries
| File | Description |
|---|---|
src/os/posix/wayland.rs |
Rewrites the Wayland backend and adds safety tests. |
Cargo.toml |
Updates Wayland dependencies and features. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The i64 byte-count guard could itself overflow: i32::MAX * i32::MAX * 4 exceeds i64::MAX, so a large window request panicked in the check meant to reject it. Size validation now lives in one checked helper that every path taking a size goes through -- window creation, compositor configure and the shm pool -- so width * height cannot overflow downstream. A configure carries compositor-controlled dimensions and was stored unvalidated, letting an oversized one reach scale_buffer's i32 multiply. It is now refused, with 0x0 still treated as the normal "you choose" configure. A non-WouldBlock flush error was only logged, so a broken pipe left the window open despite the read and dispatch paths closing it.
|
All three addressed in 1addb1e. Byte-count overflow — correct, and the guard itself was the bug: Flush errors — right, a broken pipe on flush left the window open while the read and dispatch paths closed it. Unvalidated configure — right, the dimensions are compositor-controlled and went straight into |
There was a problem hiding this comment.
🟡 Changes recommended
Configure events with one zero dimension are mishandled, and unknown pointer states can incorrectly release buttons.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/os/posix/wayland.rs:1067
- An unknown extensible
ButtonStatecurrently evaluates tofalse, so it is interpreted as a release and can clear a pressed button. As with the key and axis handlers, ignore unknown enum values rather than changing input state.
let pressed = matches!(state, WEnum::Value(ButtonState::Pressed));
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| if let Some((width, height)) = self.display.state.resolution.take() { | ||
| if self.resizable { | ||
| // The compositor picks these, so a size the framebuffer maths | ||
| // cannot represent has to be refused rather than stored; 0x0 | ||
| // is the normal "you choose" configure. | ||
| if framebuffer_bytes(width, height).is_some() { | ||
| self.width = width; | ||
| self.height = height; | ||
| } else if (width, height) != (0, 0) { | ||
| eprintln!("Ignoring unusable configure size {}x{}", width, height); | ||
| } | ||
| } | ||
| } |
Size validation was duplicated across window creation, the compositor configure path and the shm pool, each with its own error. A SurfaceSize now carries the checked width, height and byte count, and new() is its only constructor, so holding one is the proof the size was checked and nothing downstream revalidates. A configure may zero either axis on its own, meaning the compositor is leaving that dimension to the client. The previous code rejected the whole pair, so a (0, 600) configure was dropped rather than applied to the height alone. An unknown wl_pointer ButtonState was read as a release and could clear a button still being held; it is now ignored, as in the key and axis handlers.
|
Both addressed in 7a6a825, along with consolidating the size checks that were accumulating. Zero configure axis — correct, Unknown Rather than adding a fourth size check, validation is consolidated into a 33 unit + 34 doctests. |
There was a problem hiding this comment.
🟡 Changes recommended
Malformed compositor keymap and key events can still crash or corrupt input handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
| if let WEnum::Value(state) = state { | ||
| Self::handle_key( | ||
| self.xkb_keymap, | ||
| self.xkb_state, | ||
| key + KEY_XKB_OFFSET, | ||
| state, | ||
| &mut self.key_handler, | ||
| &mut self.held, | ||
| ); | ||
| } |
| )?; | ||
| let size = SurfaceSize::scaled(width, height, scale).ok_or_else(|| { | ||
| Error::WindowCreate(format!( | ||
| "{}x{} at {}x scale is too large", |
| len as usize, | ||
| libc::PROT_READ, | ||
| libc::MAP_PRIVATE, | ||
| fd, | ||
| fd.as_raw_fd(), |
A keycode arrives as a compositor-supplied u32 and was offset into xkb's numbering with a plain add, so a large one panicked in debug builds and wrapped to a low, valid-looking keycode in release ones. It is skipped now, as an unknown button state already is. The keymap mapping was taken on trust. mmap does not check the length the compositor sends against the file behind the fd, so a short fd mapped cleanly and only faulted once xkbcommon read it, and a keymap with no terminator sent the parser's scan past the end of the mapping. Both are refused before parsing: the fd has to be at least as long as the advertised keymap, and the last byte the protocol counts has to be the terminator.
SurfaceSize::new refuses a non-positive dimension as well as a byte count that will not fit the wire, so a 0x0 window was reported as too large. Both public API and Scale reach it, and the two cases want telling apart.
There was a problem hiding this comment.
🔵 Needs a closer look
Configure transactions can be mismatched, and unsupported keymaps can leave stale keyboard state active.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/os/posix/wayland.rs:350
- The configure serial is stored independently from the preceding
xdg_toplevel::Configurestate. If dispatch readstoplevel(B)aftersurface(A)but beforesurface(B),update()applies size B and the next frame acknowledges serial A, mixing two configure transactions. Keep a pending toplevel size and snapshot it together with the serial whenxdg_surface::Configurearrives, then consume that pair atomically.
// Only the most recent configure needs acknowledging.
if let xdg_surface::Event::Configure { serial } = event {
state.xdg_config = Some(serial);
src/os/posix/wayland.rs:970
- An explicit
KeymapFormat::NoKeymapreaches this error branch, but only logging leaves any previous XKB keymap/state active. Subsequent raw keycodes are then interpreted using a stale layout even though the compositor has withdrawn that format. HandleNoKeymapseparately by disabling/clearing keyboard translation (and releasing tracked keys), or close the window if unsupported.
Err(e) => eprintln!("Failed to load the compositor keymap: {:?}", e),
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
An xdg_toplevel::Configure size was published the moment it arrived, independently of the xdg_surface::Configure serial that closes the sequence. A dispatch round ending between the two left the new size paired with the previous transaction's serial, so the next frame committed one configure's dimensions while acknowledging another's. The size is held pending until the serial arrives and the two are published together. A keymap the backend cannot use was logged and otherwise ignored, leaving the previous keymap and state live. A compositor withdrawing its layout -- sending no_keymap after a valid one -- therefore had subsequent keycodes translated through a layout it had already taken back. Translation now stops instead. Keys held at that point are released, since with translation stopped nothing would ever report them up.
Moves the Wayland backend from the unmaintained 0.29 line to wayland-client 0.31 / wayland-protocols 0.32 / wayland-cursor 0.31.
The 0.29 event model (
Main<T>+quick_assign) no longer exists; events now arrive throughDispatchimpls on one state type, which queues keyboard and pointer events forWindow::updateto drain. The xkbcommon path is unchanged — level-0 keysym derivation, modifier tracking, held-key release bookkeeping andadd_charall carried over, andkey_level_testsstill covers them.wayland-clientneeds thesystemfeature:raw-window-handlehas to hand out a realwl_surface*/wl_display*, which only the libwayland backend can produce.dlopenstill selects runtime loading vs linking, so the default build links neither libwayland nor libxkbcommon, as before.Bugs fixed by the rewrite:
wl_buffer::Releaseand never cleared, and a resize dropped the new flag, so from frame 3 on a buffer was reused whether or not the compositor had released it. It is now the buffer's dispatch user data, cleared on attach. With every buffer held and the pool at its cap the frame is dropped rather than written into a buffer being scanned out.sync_channel(1024)was filled and drained on the same thread, so an event burst could block dispatch with nothing draining it. Events queue in aVecinstead.OwnedFdand is closed; a keymap change unrefs the previous keymap and state.update()closes the window instead of panicking.Sizes and input coming from the compositor are validated rather than trusted:
SurfaceSizetype carries a checked width, height and ARGB byte count, andnew()is its only constructor, so holding one is the proof the siz 8000 e was checked and nothing downstream revalidates. Window creation, the configure path and the shm pool all take one. This replaces a first attempt at the check that could itself overflow:i32::MAX * i32::MAX * 4exceedsi64::MAX, so the guard meant to reject an oversized window panicked on it.scale_buffer'si32multiply unvalidated. They are now refused if unusable, and a configure may zero either axis on its own to leave that dimension to the client — a(0, 600)configure applies to the height instead of being dropped as a pair.wl_pointerButtonStatewas read as a release and could clear a button still being held. It is ignored now, matching the key and axis handlers.WouldBlockflush error was only logged, so a broken pipe left the window open despite the read and dispatch paths closing it.u32and was offset into xkb's numbering with a plain add, so a large one panicked in debug builds and wrapped to a low, valid-looking keycode in release ones.mmapdoes not check the length the compositor sends against the file behind the fd, so a short fd mapped cleanly and only faulted once xkbcommon read it, and a keymap with no terminator sent the parser's scan past the end of the mapping. Both are refused before parsing.xdg_toplevel::Configuresize was published independently of thexdg_surface::Configureserial that closes the sequence, so a dispatch round ending between the two left a frame committing one configure's dimensions while acknowledging another's. The size is held pending until the serial arrives and the two are published together.Drops nix 0.24, memoffset, wayland-commons and xml-rs; no duplicate crate versions remain.
Verified on Hyprland 0.56.2: examples run clean, keys and modifiers inject correctly via
wtype, andget_sizetracks the compositor's configures across a resize. 33 unit + 34 doctests pass.cargo fmt --checkis clean andcargo clippy --all-targetsreports exactly the same set of pre-existing warnings as master, across the wayland, wayland+x11, x11 and default feature sets.Closes #318
Closes #408
Closes #409
Closes #410
Closes #375