From aafc73a4164250c64a6f25c89d5e1d915bfcd2b7 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 22:57:12 +0200 Subject: [PATCH 1/9] Clipboard copied once into a region the compositor made; no client handle is ever used The compositor took MSG_CLIPBOARD_SET_SHM's region from the client: any handle the client chose went to shm_map, and a pipe there is WrongType, which the kernel answers by ending the caller (exit 139). And it validated the region as UTF-8 in place and then copied it, while the client could still write it. Now the compositor never receives a handle from a client. A copy past the inline limit is MSG_COPY_BEGIN{len}, answered with MSG_COPY_REGION and a region the compositor made; the client writes it and sends MSG_COPY_COMMIT; the compositor reads every byte once through AtomicU8, validates that copy with String::from_utf8, drops the region and closes the connection. A copy never committed is dropped at HANDSHAKE_TIMEOUT by name. Message 10 is retired and refused by name. The inline clipboard goes through the same validation instead of from_utf8_lossy. The compositor denies clippy::undocumented_unsafe_blocks. The key read is a byte buffer decoded as KeyEvent through IpcPayload instead of a byte view of an event array; the cursor upload writes the mapping's slice instead of a raw pointer; the one unsafe block left in render.rs carries its SAFETY clause. compositor_hostile_clipboard is the guest test: a pipe on the retired message, a region rewritten while it is read, a region rewritten after its commit, and a copy never committed. compositor_client_death's two region cases move to the new protocol. issues/isolation/a-received-handle-has-no-knowable-type.md loses the two bullets that said a hostile client reaches only a closed instance; blockd is one it reaches. Filed: the compositor ignores unknown message types, a window's buffer is read while its client writes it, and no gate runs clippy over the compositor. Co-Authored-By: Claude Opus 5.5 --- issues/build/no-gate-lints-the-compositor.md | 21 ++ .../a-received-handle-has-no-knowable-type.md | 14 +- ...ffer-is-read-while-its-client-writes-it.md | 18 ++ ...itor-ignores-a-message-it-does-not-know.md | 21 ++ .../src/bin/compositor_client_death.rs | 37 +-- .../src/bin/compositor_hostile_clipboard.rs | 280 ++++++++++++++++++ tests/toyos.rs | 108 ++++++- userland/compositor/src/client.rs | 41 ++- userland/compositor/src/main.rs | 2 + userland/compositor/src/render.rs | 20 +- userland/compositor/src/session.rs | 193 ++++++++---- userland/toyos-window/src/lib.rs | 113 +++++-- 12 files changed, 716 insertions(+), 152 deletions(-) create mode 100644 issues/build/no-gate-lints-the-compositor.md create mode 100644 issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md create mode 100644 issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md create mode 100644 tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs diff --git a/issues/build/no-gate-lints-the-compositor.md b/issues/build/no-gate-lints-the-compositor.md new file mode 100644 index 00000000000..19d68a87aa4 --- /dev/null +++ b/issues/build/no-gate-lints-the-compositor.md @@ -0,0 +1,21 @@ +--- +status: open +kind: tooling +opened: 2026-09-27 +--- + +# No gate runs clippy over the compositor + +`userland/compositor/src/main.rs` denies `clippy::undocumented_unsafe_blocks`, +and nothing in `src/clippy.rs` or `.github/workflows/` runs clippy over +`userland/`, so the attribute holds only when someone runs it by hand. The +`toyos` toolchain has no clippy, but the compositor checks with the host's: + + cd userland && cargo +stable clippy -p compositor --no-deps \ + --target aarch64-apple-darwin -- -A clippy::all + +exits 0 and reds on an unsafe block with no `SAFETY:` comment. Without +`-A clippy::all` it reds on one default lint in `Session::tick_taskbar`. + +**Exit**: a shape in `src/clippy.rs` runs that command for the compositor on +every host architecture the gate runs on, and fails on its findings. diff --git a/issues/isolation/a-received-handle-has-no-knowable-type.md b/issues/isolation/a-received-handle-has-no-knowable-type.md index 9e4dd02c6b9..f26562face8 100644 --- a/issues/isolation/a-received-handle-has-no-knowable-type.md +++ b/issues/isolation/a-received-handle-has-no-knowable-type.md @@ -31,6 +31,8 @@ typed can be ended by whoever sent it.** The sender needs nothing but - An audio client receives two handles from soundd and calls `SYS_SHM_MAP` on the first (`toyos/src/audio.rs`). A hostile *server* ends every client. - A window client receives its buffer the same way (`userland/toyos-window/src/lib.rs`). +- blockd maps the region a client sends with its open (`Region::adopt` in + `userland/blockd/src/region.rs`), so a *client* holding its connector ends it. Nothing in the tree is hostile today, so nothing fails. The property the architecture claims — that a process cannot be harmed by what it was not given — @@ -57,18 +59,6 @@ change, and a new syscall is the owner's to approve. Judged while clearing PR #22's blockers, with the reasoning written down because the next reader will ask why a class this wide was left open. -- **The one instance a hostile *client* can reach is closed.** `/system/bin/init`'s - launcher takes `extra` connectors from anybody holding a `launcher` connector - and hands them to `SYS_NAMESPACE_BUILD`, and that call answers - `InvalidArgument` for a wrong type rather than ending the caller. It is the - one handle argument in the ABI that routinely crosses a trust boundary, - `kernel/CLAUDE.md` says so where the policy is stated, and `launcher_refusals` - gates it. -- **Every other instance needs a hostile *server*** — soundd sending an audio - client its region, the compositor sending a window its buffer. A client whose - server is hostile has already lost: that server chooses what the client sees, - when it is answered, and whether it is answered at all. Ending it with a - `WrongType` is not a new capability. - **The fix is an ABI shape change and the ABI was the owner's to approve.** It widens `SYS_HANDLE_RECV`'s answer from `n` to `n` pairs, which is a syscall the owner approved changing shape after the fact. diff --git a/issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md b/issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md new file mode 100644 index 00000000000..44c0178e2fa --- /dev/null +++ b/issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md @@ -0,0 +1,18 @@ +--- +status: open +kind: defect +opened: 2026-09-27 +--- + +# A window's buffer is read while its client writes it + +Each window has one region, which the client draws into and the compositor +blits from (`render::draw_window` in `userland/compositor/src/render.rs`). The +compositor reads it as a `&[u8]` while the client may be writing it: a data +race in Rust's model, and on the panel a frame whose pixels come from two of +the client's frames. Nothing bounds the read outside the mapping, so the +compositor is not at risk; the soundness claim and the frame are. + +**Exit**: the compositor makes two buffers per window, a client hands one over +with its present and gets it back on release, and the compositor never reads a +buffer the client holds. diff --git a/issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md b/issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md new file mode 100644 index 00000000000..a2653f2c6cf --- /dev/null +++ b/issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md @@ -0,0 +1,21 @@ +--- +status: open +kind: defect +opened: 2026-09-27 +--- + +# The compositor ignores a message type it does not know + +`Session::dispatch` (`userland/compositor/src/session.rs`) ends its match with +`_ => {}`: a frame of a type no protocol here defines is read, framed, and +dropped without a word, on a window's connection and on a fresh one alike. The +one retired type, `window::MSG_RETIRED_CLIPBOARD_SET_SHM`, is refused by name; +every other unknown type is accepted and silently discarded. + +`compositor_stall`'s streaming case depends on it: its window sends +`UNKNOWN_MSG` on every pass as load with nothing to draw +(`tests/toyos-rust-tests/src/bin/compositor_stall.rs`). A refusal there would +drop the window after its first frame and leave the case passing with no load. + +**Exit**: an unknown type drops its client with `DropReason::OutOfProtocol`, +and the stall's stream is a type the compositor serves without drawing. diff --git a/tests/toyos-rust-tests/src/bin/compositor_client_death.rs b/tests/toyos-rust-tests/src/bin/compositor_client_death.rs index 04eeffb4038..206b659d98f 100644 --- a/tests/toyos-rust-tests/src/bin/compositor_client_death.rs +++ b/tests/toyos-rust-tests/src/bin/compositor_client_death.rs @@ -30,7 +30,6 @@ use std::process::{exit, Command, Stdio}; use toyos::endow; use toyos::AsHandle; -use toyos::shm::SharedMemory; use toyos::{ipc, Connection}; use toyos_abi::syscall::{self, SyscallError}; use toyos_abi::RawHandle; @@ -110,19 +109,20 @@ fn run() { write_handle(doubled.handle(), &create_frame(), "a second create"); probe("a second create on a live window"); - // A clipboard frame with no region sent ahead of it. The receive is not a - // poll — a short batch is a peer that sent its frame first — so this must - // cost the client its connection and nothing else. - clipboard_shm(None, 64, "a clipboard frame with no region"); - probe("a clipboard frame with no region"); + // A commit with no copy begun: there is no region for it to name, so this + // must cost the client its connection and nothing else. + let commit = endow::service("compositor").expect("a connection to commit on"); + ipc::signal(commit.as_handle(), window::MSG_COPY_COMMIT).expect("send the commit"); + probe("a commit with no copy begun"); - // A region really sent, with a length no region can satisfy. The length - // decides how much of the region is read as clipboard text, so it is the - // compositor's to bound rather than the client's to choose. - let region = SharedMemory::create(4096).expect("a region of our own"); - let shared = region.share().expect("a second handle to it"); - clipboard_shm(Some(shared), u32::MAX, "a clipboard longer than any region"); - probe("a clipboard longer than any region"); + // A copy no region is made for. The length decides how large a region the + // compositor makes, so it is the compositor's to bound rather than the + // client's to choose. + let begin = endow::service("compositor").expect("a connection to begin on"); + begin + .send(window::MSG_COPY_BEGIN, &window::ClipboardShmMsg { len: u32::MAX }) + .expect("send the begin"); + probe("a copy longer than any clipboard"); // An inline clipboard one byte past what any client may inline. The // compositor keeps that one byte, so the frame is refusable here instead of @@ -234,17 +234,6 @@ fn create_frame() -> Vec { frame } -fn clipboard_shm(region: Option, len: u32, what: &str) { - let conn = endow::service("compositor") - .unwrap_or_else(|e| fail(&format!("[{what}] the compositor is not serving: {e:?}"))); - let msg = window::ClipboardShmMsg { len }; - let sent = match region { - Some(h) => conn.send_with_handles(&[h], window::MSG_CLIPBOARD_SET_SHM, &msg), - None => conn.send(window::MSG_CLIPBOARD_SET_SHM, &msg), - }; - sent.unwrap_or_else(|e| fail(&format!("[{what}] could not send: {e:?}"))); -} - /// Every write here fits in the pipe it goes into, so a blocking `write` can /// only be the compositor's problem, never this binary's. fn write_handle(handle: toyos_abi::RawHandle, bytes: &[u8], what: &str) { diff --git a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs new file mode 100644 index 00000000000..369081bf330 --- /dev/null +++ b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs @@ -0,0 +1,280 @@ +//! Two hostile clipboards, and a compositor that must outlive both and keep +//! only text it validated. +//! +//! Needs a live compositor and a host that types GUI+V, which the shared boot +//! does not have — it is in `RUST_SKIP` and `metal_sim_hostile_clipboard` runs +//! it on the metal-sim profile. +//! +//! 1. **A wrong-typed handle.** A pipe end rides the retired region message. +//! The kernel ends whoever maps a pipe as shared memory, so the compositor +//! must refuse the client without ever receiving the handle — and the pipe's +//! writer, queued on the refused connection, must go back to the kernel +//! unused. +//! 2. **A region rewritten while it is read.** The client commits a copy of +//! `A`s and keeps rewriting the region between `A`s and bytes that are not +//! UTF-8. The compositor may keep the text or refuse it, and the paste that +//! follows has to be one of the two texts it could have validated. +//! 3. **A region rewritten after its commit.** Once the compositor has closed +//! the connection the region is the client's own again, and a paste has to +//! be what was committed, not what the region holds now. +//! 4. **A copy never committed.** The client holds its region and says nothing; +//! the compositor has to drop it by name rather than hold the region. +//! +//! Each case ends with a probe the compositor answers from its dispatch, under +//! a deadline. The host asserts what this side cannot see: no handle fault and +//! no compositor exit in the kernel's records, and the refusals named. + +use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; +use std::sync::Arc; +use std::thread; +use std::time::{Duration, Instant}; + +use toyos::endow; +use toyos::poller::{Poller, READABLE}; +use toyos::shm::SharedMemory; +use toyos::{AsHandle, Connection}; +use toyos_abi::syscall::{self, SyscallError}; +use toyos_abi::RawHandle; +use window::{Event, Window}; + +// The wire as this client speaks it, spelled here rather than imported: the +// negative control builds this binary against a `window` that predates all +// four. +const RETIRED_CLIPBOARD_SET_SHM: u32 = 10; +const COPY_BEGIN: u32 = 13; +const COPY_COMMIT: u32 = 14; +const COPY_REGION: u32 = 13; +/// The longest copy the compositor makes a region for. +const COPY_LEN: usize = 2 * 1024 * 1024; + +/// The line the host answers with GUI+V. Printed again while no paste has +/// come, so an injection lost on the way costs time and not the verdict. +const PASTE_MARKER: &str = "===HOSTILE_CLIPBOARD_PASTE==="; +const REMARK: Duration = Duration::from_secs(2); + +/// A liveness ceiling on every wait here: it costs nothing when the answer +/// comes, and bounds a compositor that is gone or parked. +const CEILING: Duration = Duration::from_secs(30); + +const BEFORE: &str = "hostile clipboard: the text before"; + +fn main() { + // First, so it has the focus: GUI+V pastes into the focused window. + let mut target = Window::create_with_title(160, 120, "paste") + .unwrap_or_else(|e| fail("the paste target", &format!("no window: {e}"))); + target.present(); + window::clipboard_set(BEFORE) + .unwrap_or_else(|e| fail("the clipboard to start from", &e.to_string())); + probe("the clipboard to start from"); + + wrong_typed_handle(); + probe("a wrong-typed handle"); + + rewritten_while_read(); + probe("a region rewritten while it is read"); + let what = "the paste after a rewrite during the read"; + let first = paste(&mut target, what, None); + let all_a = first.len() == COPY_LEN && first.iter().all(|&b| b == b'A'); + if first != BEFORE.as_bytes() && !all_a { + fail(what, &describe(&first, b'A')); + } + println!( + "hostile clipboard: the rewritten copy was {}", + if all_a { "kept whole" } else { "refused" } + ); + + rewritten_after_commit(); + probe("a region rewritten after its commit"); + let what = "the paste after a rewrite past the commit"; + let second = paste(&mut target, what, Some(&first)); + if second.len() != COPY_LEN || second.iter().any(|&b| b != b'C') { + fail(what, &describe(&second, b'C')); + } + + let what = "a copy never committed"; + let (conn, _region) = begin_copy(what); + await_hangup(conn.as_handle(), what, "the compositor giving up on the commit"); + probe(what); + + println!("hostile clipboard: 4 cases survived, compositor still serving"); +} + +/// A pipe end where the retired message carried a region. +fn wrong_typed_handle() { + let what = "a wrong-typed handle"; + let ends = syscall::pipe().unwrap_or_else(|e| fail(what, &format!("no pipe: {e:?}"))); + let conn = connect(what); + conn.send_with_handles( + &[ends.write], + RETIRED_CLIPBOARD_SET_SHM, + &window::ClipboardShmMsg { len: 64 }, + ) + .unwrap_or_else(|e| fail(what, &format!("could not send: {e:?}"))); + await_hangup(conn.as_handle(), what, "the compositor's refusal"); + // The send moved the only writer, so the reader hangs up exactly when the + // queue holding it is gone — and not while anything holds it. + await_hangup(ends.read, what, "the writer's return to the kernel"); + syscall::close(ends.read); +} + +/// Commit `A`s, then rewrite the region while the compositor copies it. +fn rewritten_while_read() { + let what = "a region rewritten while it is read"; + let (conn, region) = begin_copy(what); + for byte in bytes(®ion) { + byte.store(b'A', Ordering::Relaxed); + } + let stop = Arc::new(AtomicBool::new(false)); + let rewriter = { + let stop = Arc::clone(&stop); + thread::spawn(move || { + let mut fill = 0xFF; + while !stop.load(Ordering::Relaxed) { + for byte in bytes(®ion) { + byte.store(fill, Ordering::Relaxed); + } + fill = if fill == b'A' { 0xFF } else { b'A' }; + } + }) + }; + conn.signal(COPY_COMMIT).unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); + await_hangup(conn.as_handle(), what, "the compositor closing the copy"); + stop.store(true, Ordering::Relaxed); + rewriter.join().unwrap_or_else(|_| fail(what, "the rewriter panicked")); +} + +/// Commit `C`s, wait for the compositor to be done with them, then overwrite +/// the region. +fn rewritten_after_commit() { + let what = "a region rewritten after its commit"; + let (conn, region) = begin_copy(what); + for byte in bytes(®ion) { + byte.store(b'C', Ordering::Relaxed); + } + conn.signal(COPY_COMMIT).unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); + await_hangup(conn.as_handle(), what, "the compositor closing the copy"); + for byte in bytes(®ion) { + byte.store(0xFF, Ordering::Relaxed); + } +} + +/// A connection holding the region the compositor made for a whole copy. +fn begin_copy(what: &str) -> (Connection, SharedMemory) { + let conn = connect(what); + conn.send(COPY_BEGIN, &window::ClipboardShmMsg { len: COPY_LEN as u32 }) + .unwrap_or_else(|e| fail(what, &format!("could not begin: {e:?}"))); + await_readable(conn.as_handle(), what, "the compositor's region"); + let header = conn.recv_header().unwrap_or_else(|e| fail(what, &format!("no answer: {e:?}"))); + if header.msg_type != COPY_REGION { + fail(what, &format!("the compositor answered with message type {}", header.msg_type)); + } + let info: window::ClipboardShmMsg = conn + .recv_payload(&header) + .unwrap_or_else(|e| fail(what, &format!("no length: {e:?}"))); + if info.len as usize != COPY_LEN { + fail(what, &format!("a region of {} bytes for a copy of {COPY_LEN}", info.len)); + } + let [region] = + conn.recv_handles_exact::<1>().unwrap_or_else(|| fail(what, "the answer had no region")); + let region = SharedMemory::adopt(region, COPY_LEN) + .unwrap_or_else(|e| fail(what, &format!("the region would not map: {e:?}"))); + (conn, region) +} + +/// The region as the only type that may alias memory another process reads. +fn bytes(region: &SharedMemory) -> &[AtomicU8] { + // SAFETY: the mapping is `region.len()` bytes and `region` outlives the + // borrow; the compositor reads it concurrently, which an atomic permits. + unsafe { std::slice::from_raw_parts(region.as_ptr() as *const AtomicU8, region.len()) } +} + +/// Ask the host for GUI+V and return what the target is pasted, skipping any +/// paste equal to `stale` — an earlier marker's second injection. +fn paste(target: &mut Window, what: &str, stale: Option<&[u8]>) -> Vec { + let deadline = Instant::now() + CEILING; + let mut mark = Instant::now(); + loop { + let now = Instant::now(); + if now >= deadline { + fail(what, &format!("no paste in {} s of asking", CEILING.as_secs())); + } + if now >= mark { + println!("{PASTE_MARKER}"); + mark = now + REMARK; + } + let wait = mark.min(deadline).saturating_duration_since(now); + match target.poll_event(wait.as_nanos() as u64) { + Some(Event::ClipboardPaste(text)) if Some(text.as_slice()) != stale => return text, + Some(Event::Close) => fail(what, "the paste target's window was closed"), + _ => {} + } + } +} + +/// A paste that is neither validated text, summarised — never printed whole. +fn describe(text: &[u8], fill: u8) -> String { + let stray = text.iter().position(|&b| b != fill); + format!( + "the paste was {} bytes, UTF-8: {}, first byte that is not {:?} at {stray:?} — the \ + compositor kept bytes it had not validated", + text.len(), + std::str::from_utf8(text).is_ok(), + fill as char + ) +} + +fn connect(what: &str) -> Connection { + endow::service("compositor") + .unwrap_or_else(|e| fail(what, &format!("the compositor is not serving: {e:?}"))) +} + +/// Wait until `handle` is readable, or fail at the ceiling. +fn await_readable(handle: RawHandle, what: &str, awaited: &str) { + let poller = Poller::new(1); + poller.watch_raw(handle, READABLE, 0); + let mut ready = false; + poller.wait(1, CEILING.as_nanos() as u64, |_| ready = true); + if !ready { + fail(what, &format!("{awaited} did not come in {} s", CEILING.as_secs())); + } +} + +/// Wait for the peer of `handle` to hang up, failing on anything it sends. +fn await_hangup(handle: RawHandle, what: &str, awaited: &str) { + let deadline = Instant::now() + CEILING; + loop { + let mut byte = [0u8; 1]; + match syscall::read_nonblock(handle, &mut byte) { + Ok(0) => return, + Ok(_) => fail(what, &format!("the peer answered where {awaited} was due")), + Err(SyscallError::WouldBlock) => {} + Err(e) => fail(what, &format!("waiting for {awaited}: {e:?}")), + } + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + fail(what, &format!("{awaited} did not come in {} s", CEILING.as_secs())); + } + let poller = Poller::new(1); + poller.watch_raw(handle, READABLE, 0); + poller.wait(1, left.as_nanos() as u64, |_| {}); + } +} + +/// Ask the compositor something it always answers, under the ceiling. +fn probe(what: &str) { + let conn = connect(what); + conn.signal(window::MSG_GET_RESOLUTION) + .unwrap_or_else(|e| fail(what, &format!("could not ask for the resolution: {e:?}"))); + await_readable(conn.as_handle(), what, "the compositor's answer to a probe"); + let header = conn + .recv_header() + .unwrap_or_else(|e| fail(what, &format!("the probe went unanswered: {e:?}"))); + if header.msg_type != window::MSG_RESOLUTION_CHANGED { + fail(what, &format!("the probe was answered with message type {}", header.msg_type)); + } +} + +fn fail(what: &str, msg: &str) -> ! { + eprintln!("hostile clipboard: [{what}] {msg}"); + std::process::exit(1); +} diff --git a/tests/toyos.rs b/tests/toyos.rs index f2ed0e90973..498bd6f91d3 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -350,6 +350,9 @@ const RUST_SKIP: &[&str] = &[ // Same again, and it also needs a host injecting pointer packets: // `metal_sim_window_drag` runs it. "window_drag", + // Same again, and it also needs a host typing GUI+V: + // `metal_sim_hostile_clipboard` runs it. + "compositor_hostile_clipboard", // Needs a compositor, a terminal and a shell: `desktop_window_child` // launches it from that shell. "window_child", @@ -752,6 +755,9 @@ const MACHINE_TESTS: &[(&str, Sched, Tier)] = &[ // slow on purpose. Its own boot too: it leaves the pointer somewhere else // and the window in a different place than it found them. ("metal_sim_window_drag", Sched::Serial, Tier::Nightly), + // A client's clipboard, hostile two ways; no clock in any verdict. Its own + // boot: the compositor it abuses has to be one nothing else has touched. + ("metal_sim_hostile_clipboard", Sched::Parallel, Tier::Fast), // A host-measured drain rate with an 8 s ceiling on a 3.3 s expectation. // Not gate A, but the same instrument: what it measures is how fast a // client's audio leaves the machine. @@ -1711,6 +1717,7 @@ const CARRIES: &[(&str, &[&str])] = &[ ("metal_sim_compositor_stall", METAL_SIM_CLIENTS), ("metal_sim_client_death", METAL_SIM_CLIENTS), ("metal_sim_window_drag", &["test_rs_window_drag"]), + ("metal_sim_hostile_clipboard", &["test_rs_compositor_hostile_clipboard"]), ("desktop_window_child", &["test_rs_window_child"]), ("toolkit_window_wake", &["test_rs_window_wake"]), ("toolkit_winit_loop", &["test_rs_winit_loop"]), @@ -7724,6 +7731,91 @@ fn metal_sim_window_drag(rust_bins: &[(String, Vec)]) -> Result<(), String> Ok(()) } +/// A client's clipboard sent two hostile ways: a pipe where a region went, and +/// a region rewritten while the compositor reads it. +/// +/// The guest runs the cases and asks for a paste after each copy; this half +/// types GUI+V at every ask and asserts what the guest cannot see. **The +/// kernel's record is the independent half**: a compositor that maps the pipe +/// is ended by the kernel with a handle fault, whatever the compositor believed +/// it was doing, so no such record may appear — and each refusal the guest +/// caused must be named by the compositor. +fn metal_sim_hostile_clipboard(rust_bins: &[(String, Vec)]) -> Result<(), String> { + /// The guest's `PASTE_MARKER`. + const PASTE: &str = "===HOSTILE_CLIPBOARD_PASTE==="; + let bins: Vec<(String, Vec)> = rust_bins + .iter() + .filter(|(name, _)| name == "compositor_hostile_clipboard") + .cloned() + .collect(); + if bins.is_empty() { + return Err("the compositor_hostile_clipboard client was not built".to_string()); + } + let config = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/metalcase"); + let options = + BootOptions { profile: qemu::Profile::Metal, qmp: true, ..Default::default() }; + metal_sim_argv_check(&qemu::profile_argv(&options))?; + let mut qemu = QemuInstance::boot_with_options(&config, &[], &bins, options); + + let result = qemu.run_test_paced( + "test_rs_compositor_hostile_clipboard", + Duration::from_secs(240), + |socket, line| { + if line.contains(PASTE) { + let socket = socket.expect("this boot was made with QMP"); + qemu::QmpInput::open(socket).keys(&[ + ("meta_l", true), + ("v", true), + ("v", false), + ("meta_l", false), + ]); + } + }, + ); + let text = format!("{}\n{}", result.stdout, result.serial); + + for death in ["handle fault:", "exit: compositor"] { + if let Some(line) = text.lines().find(|l| l.contains(death)) { + return Err(format!("the kernel ended the compositor: {}\n{text}", line.trim())); + } + } + if result.error.is_some() || result.exit_code != Some(0) { + let why = match &result.error { + Some(err) => err.to_string(), + None => String::from("it finished and its exit code is the finding"), + }; + return Err(format!( + "compositor_hostile_clipboard exited {:?}: {why}\n{text}", + result.exit_code + )); + } + if !text.contains("hostile clipboard: 4 cases survived") { + return Err(format!("the guest did not report its four cases survived:\n{text}")); + } + if !text.contains("it began a copy and never committed it") { + return Err(format!( + "the client that held its region and never committed was not dropped by name:\n{text}" + )); + } + const REFUSED: &str = "it sent a frame this protocol cannot describe"; + if !text.lines().any(|l| l.contains("compositor: dropping client") && l.contains(REFUSED)) { + return Err(format!( + "the client that sent a pipe where a region went was not refused by name:\n{text}" + )); + } + if text.contains("hostile clipboard: the rewritten copy was refused") + && !text.contains("compositor: refusing a clipboard from client") + { + return Err(format!( + "the rewritten copy never reached the clipboard and the compositor never said \ + why:\n{text}" + )); + } + serial::Serial::named("boot console", result.serial.as_str()).must_be_clean()?; + eprintln!(" [metal-sim] a pipe refused unused, both rewritten regions read once"); + Ok(()) +} + /// How far above its content the host reaches for a window's title bar. /// /// Not the compositor's title-bar height — this is a probe, and what it needs @@ -11408,9 +11500,9 @@ fn metal_sim_client_death(boot: &mut Boot) -> Result<(), String> { )); } - // The one case whose verdict is a line rather than survival: a payload - // past what any client may inline is refused by name, because storing the - // prefix a frame reader keeps is the silent half of the same event. + // A payload past what any client may inline is refused by name, because + // storing the prefix a frame reader keeps is the silent half of the same + // event. const OVERSIZE: &str = "compositor: refusing an inline payload past"; if !result.stdout.contains(OVERSIZE) { return Err(format!( @@ -11419,6 +11511,15 @@ fn metal_sim_client_death(boot: &mut Boot) -> Result<(), String> { result.stdout )); } + // A copy's length sizes the region the compositor makes, so a length past + // the clipboard is refused before any region exists. + const LONG_COPY: &str = "compositor: refusing a copy of 4294967295 bytes"; + if !result.stdout.contains(LONG_COPY) { + return Err(format!( + "a copy longer than any clipboard was not refused by name:\n{}", + result.stdout + )); + } // Still painting once every case is behind it, on a capture that starts // empty — so this counts frames produced *after* the last case. The @@ -15386,6 +15487,7 @@ fn run_machine_test( Ok(()) } "metal_sim_window_drag" => metal_sim_window_drag(rust_bins), + "metal_sim_hostile_clipboard" => metal_sim_hostile_clipboard(rust_bins), "metal_sim_pointer_churn" => { // The owner froze his desktop twice by plugging a mouse in and // pulling it out again, and the second freeze landed on the fourth diff --git a/userland/compositor/src/client.rs b/userland/compositor/src/client.rs index b2326912b5c..758b24b3214 100644 --- a/userland/compositor/src/client.rs +++ b/userland/compositor/src/client.rs @@ -7,6 +7,11 @@ //! it the same decision by not reading. Here a peer that stops halfway through //! a frame costs a buffer and a deadline, and one that will not take a message //! costs itself. +//! +//! **The compositor never receives a handle from a client.** A received handle's +//! kind is unknowable, and using one of the wrong kind ends the process that +//! used it — so every region a client writes is one the compositor made, and a +//! handle a client sends stays queued until its connection closes. use std::time::{Duration, Instant}; @@ -41,17 +46,6 @@ pub const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(2); /// declare up to `ipc::MAX_FRAME_LEN` and the excess is discarded unread. pub const MAX_KEPT_PAYLOAD: usize = window::MAX_INLINE_PAYLOAD + 1; -/// The largest clipboard the compositor will hold for a client. -/// -/// Two things meet here. `MSG_CLIPBOARD_SET_SHM` carries a length the client -/// chooses and the compositor reads that many bytes out of a region the client -/// sent, so an unbounded length is a read past the mapping — and the kernel -/// rounds every shared region up to one 2 MiB page -/// (`object::shm::SharedMemObject::create`), which makes a page the largest -/// length that cannot leave the smallest region anybody can send. It is also policy: a clipboard is text somebody selected, -/// and a megabyte of it is already generous. -pub const MAX_CLIPBOARD_BYTES: usize = 2 * 1024 * 1024; - /// One client's inbound framing. pub type ClientRx = ipc::FrameRx; @@ -74,20 +68,30 @@ pub type Win = Window; /// A whole client message, off the connection and in memory. /// -/// `conn` is `Some` only for the first frame on a freshly accepted connection: -/// `MSG_CREATE_WINDOW` keeps it, and every other message type answers on it -/// and lets it close. +/// `conn` is `Some` only for a frame off a connection that is not a window: +/// `MSG_CREATE_WINDOW` keeps it, `MSG_COPY_BEGIN` puts it back with the region +/// it answered with, and every other message type answers on it and lets it +/// close. pub struct ClientFrame { pub handle: RawHandle, pub msg_type: u32, payload: [u8; MAX_KEPT_PAYLOAD], payload_len: usize, pub conn: Option, + /// The connection's [`PendingConn::copy`]. + pub copy: Option, } impl ClientFrame { pub fn new(handle: RawHandle, msg_type: u32) -> Self { - Self { handle, msg_type, payload: [0; MAX_KEPT_PAYLOAD], payload_len: 0, conn: None } + Self { + handle, + msg_type, + payload: [0; MAX_KEPT_PAYLOAD], + payload_len: 0, + conn: None, + copy: None, + } } pub fn set_payload(&mut self, bytes: &[u8]) { @@ -109,6 +113,10 @@ pub struct PendingConn { pub conn: Connection, pub rx: ClientRx, pub since: Instant, + /// The region a `MSG_COPY_BEGIN` was answered with. A connection holding + /// one may send `MSG_COPY_COMMIT` and nothing else, within + /// [`HANDSHAKE_TIMEOUT`] of `since`. + pub copy: Option, } /// Why a client is going. @@ -133,6 +141,8 @@ pub enum DropReason { Gone, /// Accepted, and never completed a first frame. HandshakeTimeout, + /// Given a region to copy into, and never committed it. + CopyTimeout, } impl DropReason { @@ -142,6 +152,7 @@ impl DropReason { Self::NotReading => "its pipe will not take another message and it is not reading", Self::Gone => "its connection is gone", Self::HandshakeTimeout => "it never finished its first message", + Self::CopyTimeout => "it began a copy and never committed it", } } } diff --git a/userland/compositor/src/main.rs b/userland/compositor/src/main.rs index de29f51c1dd..c53c669a911 100644 --- a/userland/compositor/src/main.rs +++ b/userland/compositor/src/main.rs @@ -14,6 +14,8 @@ //! What is left in this file is the policy the other three read: the numbers //! that are decisions rather than derivations. +#![deny(clippy::undocumented_unsafe_blocks)] + mod client; mod render; mod session; diff --git a/userland/compositor/src/render.rs b/userland/compositor/src/render.rs index ef0696c175a..07277d1e941 100644 --- a/userland/compositor/src/render.rs +++ b/userland/compositor/src/render.rs @@ -179,6 +179,9 @@ fn draw_window( } if let Some(b) = content_blit(win, clip) { + // SAFETY: the mapping is `shm.len()` bytes and `win` holds it past this + // borrow; its client writes it concurrently, which can tear pixels but + // never reach outside the mapping. let buffer = unsafe { std::slice::from_raw_parts(win.client.shm.as_ptr(), win.client.shm.len()) }; @@ -333,27 +336,22 @@ pub fn scale_wallpaper( /// Render the cursor sprite (RGBA) into a 64x64 BGRA hardware cursor buffer. pub fn upload_cursor( fb: &toyos::FramebufferDev, - cursor_buf: *mut u8, + cursor_buf: &mut [u8], sprite: &sprite::Sprite, hw_cursor: bool, ) { let data = sprite.data(); let w = sprite.width(); let h = sprite.height(); - unsafe { - core::ptr::write_bytes(cursor_buf, 0, 64 * 64 * 4); - } + cursor_buf[..64 * 64 * 4].fill(0); for y in 0..h.min(64) { for x in 0..w.min(64) { let si = (y * w + x) * 4; let di = (y * 64 + x) * 4; - unsafe { - let dst = cursor_buf.add(di); - *dst = data[si + 2]; - *dst.add(1) = data[si + 1]; - *dst.add(2) = data[si]; - *dst.add(3) = data[si + 3]; - } + cursor_buf[di] = data[si + 2]; + cursor_buf[di + 1] = data[si + 1]; + cursor_buf[di + 2] = data[si]; + cursor_buf[di + 3] = data[si + 3]; } } if hw_cursor { diff --git a/userland/compositor/src/session.rs b/userland/compositor/src/session.rs index fff0fa135e3..ee23c363ce5 100644 --- a/userland/compositor/src/session.rs +++ b/userland/compositor/src/session.rs @@ -8,6 +8,7 @@ //! to the panel. use std::process::Command; +use std::sync::atomic::{AtomicU8, Ordering}; use std::time::{Duration, Instant}; use toyos::endow; @@ -28,7 +29,7 @@ use window::Screen; use crate::client::{ announce, deliver, deliver_signal, deliver_with_handles, mark_dead, note_closed, note_opened, Client, ClientFrame, ClientRx, Dead, DropReason, PendingConn, Win, HANDSHAKE_TIMEOUT, - MAX_CLIPBOARD_BYTES, MAX_KEPT_PAYLOAD, MAX_PENDING_CONNS, + MAX_KEPT_PAYLOAD, MAX_PENDING_CONNS, }; use crate::render::{self, Assets, BackBuffer, SystemStats, TitleBarIcons}; use crate::stats::{FrameStats, FrameTotals}; @@ -96,9 +97,8 @@ pub struct Session { screen: Screen, back: BackBuffer, hw_cursor: bool, - /// Held because `cursor_buf` points into it. - _cursor_shm: SharedMemory, - cursor_buf: *mut u8, + /// The hardware cursor's 64x64 BGRA image. + cursor_shm: SharedMemory, cursors: Cursors, current_cursor: CursorStyle, @@ -172,9 +172,8 @@ impl Session { let back = BackBuffer::new(screen.width(), screen.height(), screen.pixel_format_raw()); let hw_cursor = fb_info.flags & FLAG_HARDWARE_CURSOR != 0; - let cursor_shm = SharedMemory::adopt(fb_info.cursor, 64 * 64 * 4) + let mut cursor_shm = SharedMemory::adopt(fb_info.cursor, 64 * 64 * 4) .expect("the cursor buffer the framebuffer claim just handed over"); - let cursor_buf = cursor_shm.as_ptr(); let cursors = Cursors { default: read_sprite("/system/share/icons/cursor-bold.svg", CURSOR_PX, [255, 255, 255]), resize: read_sprite( @@ -184,7 +183,7 @@ impl Session { ), crosshair: read_sprite("/system/share/icons/crosshair-simple-bold.svg", CURSOR_PX, [0, 0, 0]), }; - render::upload_cursor(&fb_dev, cursor_buf, &cursors.default, hw_cursor); + render::upload_cursor(&fb_dev, cursor_shm.as_mut_slice(), &cursors.default, hw_cursor); let font_data = std::fs::read("/system/share/fonts/JetBrainsMono-Regular-8x16.font") .expect("failed to read font"); @@ -257,8 +256,7 @@ impl Session { screen, back, hw_cursor, - _cursor_shm: cursor_shm, - cursor_buf, + cursor_shm, cursors, current_cursor: CursorStyle::Default, font, @@ -335,11 +333,11 @@ impl Session { // client's traffic. let now = Instant::now(); for p in self.pending.iter().filter(|p| now.duration_since(p.since) >= HANDSHAKE_TIMEOUT) { - eprintln!( - "compositor: dropping client {} — {}", - p.conn.as_handle().0, - DropReason::HandshakeTimeout.why() - ); + let reason = match p.copy { + Some(_) => DropReason::CopyTimeout, + None => DropReason::HandshakeTimeout, + }; + eprintln!("compositor: dropping client {} — {}", p.conn.as_handle().0, reason.why()); } self.pending.retain(|p| now.duration_since(p.since) < HANDSHAKE_TIMEOUT); @@ -373,19 +371,18 @@ impl Session { } fn keys(&mut self) { - let mut events = [window::KeyEvent::EMPTY; 8]; - let buf = unsafe { - std::slice::from_raw_parts_mut( - events.as_mut_ptr() as *mut u8, - std::mem::size_of_val(&events), - ) - }; + const EVENT: usize = std::mem::size_of::(); + let mut buf = [0u8; 8 * EVENT]; // Never read blocking here. The kernel wakes only when a report queued // an event, so readiness and `has_data` agree — but a blocking read on // an empty queue parks the compositor until the next real key, and one // spurious wake anywhere would freeze it. - let n = self.kb.read_nonblock(buf).unwrap_or(0); - for event in &events[..n / std::mem::size_of::()] { + let n = self.kb.read_nonblock(&mut buf).unwrap_or(0); + let (events, torn) = buf[..n].as_chunks::(); + assert!(torn.is_empty(), "compositor: the keyboard read {n} bytes, not whole events"); + for raw in events { + let event = &ipc::decode_payload::(raw) + .expect("a chunk is exactly one event long"); let focused = self.stack.focused(); let action = key_action((*event).into(), focused.map(|i| self.stack[i].mode), self.launcher_open); @@ -466,7 +463,7 @@ impl Session { self.current_cursor = wanted; render::upload_cursor( &self.fb_dev, - self.cursor_buf, + self.cursor_shm.as_mut_slice(), self.cursors.get(wanted), self.hw_cursor, ); @@ -655,7 +652,12 @@ impl Session { } Ok(conn) => { self.poller.watch(&conn, READABLE, conn.as_handle().0 as u64); - self.pending.push(PendingConn { conn, rx: ClientRx::new(), since: Instant::now() }); + self.pending.push(PendingConn { + conn, + rx: ClientRx::new(), + since: Instant::now(), + copy: None, + }); } } } @@ -688,14 +690,16 @@ impl Session { frame.set_payload(self.pending[i].rx.payload(payload_len)); // A connection is identified by its first frame and by // nothing else. `MSG_CREATE_WINDOW` promotes it to a - // window; anything else is a one-shot request, answered and - // closed — which is what an `endow::service` caller like - // `window::clipboard_set` expects. + // window; `MSG_COPY_BEGIN` puts it back to wait for its + // commit; anything else is a one-shot request, answered and + // closed. // // One promotion per pass keeps `i` meaningful across the // `remove`; the rest are re-armed below and served next // pass. - frame.conn = Some(self.pending.remove(i).conn); + let p = self.pending.remove(i); + frame.conn = Some(p.conn); + frame.copy = p.copy; out.push(frame); break; } @@ -739,8 +743,14 @@ impl Session { mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); continue; } + if frame.copy.is_some() && frame.msg_type != window::MSG_COPY_COMMIT { + mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); + continue; + } match frame.msg_type { window::MSG_CREATE_WINDOW => self.create_window(frame), + window::MSG_COPY_BEGIN => self.copy_begin(frame), + window::MSG_COPY_COMMIT => self.copy_commit(frame), window::MSG_PRESENT => { let Ok(rect) = ipc::decode_payload::(frame.payload()) else { mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); @@ -761,8 +771,9 @@ impl Session { self.damage_all(); } } - window::MSG_CLIPBOARD_SET => { - self.clipboard = String::from_utf8_lossy(frame.payload()).into_owned(); + window::MSG_CLIPBOARD_SET => self.set_clipboard(handle, frame.payload().to_vec()), + window::MSG_RETIRED_CLIPBOARD_SET_SHM => { + mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); } window::MSG_LAYOUT_CHANGED => { // The compositor is the root of the surface tree and @@ -776,38 +787,6 @@ impl Session { deliver_signal(&mut self.dead, win, window::MSG_LAYOUT_CHANGED); } } - window::MSG_CLIPBOARD_SET_SHM => { - let Ok(info) = - ipc::decode_payload::(frame.payload()) - else { - mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); - continue; - }; - // The length is the client's claim about how much of the - // region it sent is text, and past the region that is a - // read of somebody else's memory rather than a clipboard. - // The region itself is no longer a claim: it is a handle - // the client moved, so there is nothing left to disbelieve - // about which memory this is. - if info.len as usize > MAX_CLIPBOARD_BYTES { - eprintln!( - "compositor: refusing {} bytes of clipboard from client {}, max \ - {MAX_CLIPBOARD_BYTES}", - info.len, - handle.0 - ); - continue; - } - let Some([buffer]) = ipc::recv_handles_exact::<1>(handle) else { - mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); - continue; - }; - let Ok(shm) = SharedMemory::adopt(buffer, info.len as usize) else { - mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); - continue; - }; - self.clipboard = String::from_utf8_lossy(shm.as_slice()).into_owned(); - } window::MSG_SET_CURSOR => { let Ok(style) = ipc::decode_payload::(frame.payload()) else { mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); @@ -962,6 +941,81 @@ impl Session { self.damage_all(); } + /// `MSG_COPY_BEGIN`: make the region the client's text goes into, send it, + /// and hold the connection for its commit. + fn copy_begin(&mut self, frame: ClientFrame) { + let handle = frame.handle; + let info = ipc::decode_payload::(frame.payload()); + let (Ok(info), Some(conn)) = (info, frame.conn) else { + mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); + return; + }; + let len = info.len as usize; + if !window::copy_fits(len) { + eprintln!( + "compositor: refusing a copy of {len} bytes from client {}, max {}", + handle.0, + window::MAX_CLIPBOARD_BYTES + ); + return; + } + let region = match SharedMemory::create(len) { + Ok(region) => region, + Err(e) => { + eprintln!( + "compositor: client {} gets no copy — no memory for {len} bytes ({e:?})", + handle.0 + ); + return; + } + }; + let theirs = match region.share() { + Ok(theirs) => theirs, + Err(e) => { + eprintln!( + "compositor: client {} gets no copy — its region cannot be shared ({e:?})", + handle.0 + ); + return; + } + }; + if let Err(e) = conn.try_send_with_handles(&[theirs], window::MSG_COPY_REGION, &info) { + mark_dead(&mut self.dead, handle, e.into()); + return; + } + self.pending.push(PendingConn { + conn, + rx: ClientRx::new(), + since: Instant::now(), + copy: Some(region), + }); + } + + /// `MSG_COPY_COMMIT`: take the text out of the region once, and let the + /// connection close. + fn copy_commit(&mut self, frame: ClientFrame) { + let handle = frame.handle; + let bare = frame.payload().is_empty(); + let (Some(region), true) = (frame.copy, bare) else { + mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); + return; + }; + self.set_clipboard(handle, copy_out(®ion)); + } + + /// The clipboard, from bytes that are the compositor's own: validated + /// only once no client can change them. + fn set_clipboard(&mut self, from: RawHandle, bytes: Vec) { + match String::from_utf8(bytes) { + Ok(text) => self.clipboard = text, + Err(e) => eprintln!( + "compositor: refusing a clipboard from client {} — it is not UTF-8 ({})", + from.0, + e.utf8_error() + ), + } + } + fn answer_resolution(&mut self, handle: RawHandle) { let reply = window::ResolutionInfo { width: self.fb_info.width, height: self.fb_info.height }; @@ -1323,6 +1377,19 @@ fn desk_of(screen: &Screen, font: &font::Font, apps: usize) -> Desk { } } +/// Every byte of `region`, each read once. +/// +/// The client still maps the region and may be writing it, so this copy is +/// the only read of it and the only thing validated. +fn copy_out(region: &SharedMemory) -> Vec { + // SAFETY: the mapping is `region.len()` bytes and `region` outlives the + // borrow; an atomic is the one type that may alias memory another process + // writes. + let bytes = + unsafe { std::slice::from_raw_parts(region.as_ptr() as *const AtomicU8, region.len()) }; + bytes.iter().map(|b| b.load(Ordering::Relaxed)).collect() +} + fn read_sprite(path: &str, size: u32, color: [u8; 3]) -> sprite::Sprite { let svg = std::fs::read(path).unwrap_or_else(|e| panic!("failed to read {path}: {e}")); sprite::Sprite::from_svg_colored(&svg, size, color) diff --git a/userland/toyos-window/src/lib.rs b/userland/toyos-window/src/lib.rs index 6350e5853c6..177b1134b0e 100644 --- a/userland/toyos-window/src/lib.rs +++ b/userland/toyos-window/src/lib.rs @@ -61,10 +61,35 @@ pub const MSG_RESOLUTION_CHANGED: u32 = 8; /// another window has no move except to serve it or to drop the connection. pub const MSG_WINDOW_REFUSED: u32 = 9; -// Shared-memory clipboard (for payloads > 116 bytes) -pub const MSG_CLIPBOARD_SET_SHM: u32 = 10; +/// Client → compositor, retired: it carried a region the client chose, and the +/// compositor never uses a handle a client sent. A client sending it is dropped +/// by name. +pub const MSG_RETIRED_CLIPBOARD_SET_SHM: u32 = 10; +/// Compositor → client: a clipboard past [`MAX_INLINE_PAYLOAD`], in a region +/// the compositor made. Payload is a [`ClipboardShmMsg`]. pub const MSG_CLIPBOARD_PASTE_SHM: u32 = 11; +/// Client → compositor, as a connection's first frame: a clipboard of +/// [`ClipboardShmMsg::len`] bytes, which [`copy_fits`]. Answered with +/// [`MSG_COPY_REGION`]. +pub const MSG_COPY_BEGIN: u32 = 13; +/// Client → compositor, bare, and the only frame a connection answered with +/// [`MSG_COPY_REGION`] may send next: the text is in the region. The compositor +/// copies it once and closes the connection. +pub const MSG_COPY_COMMIT: u32 = 14; +/// Compositor → client: a region of [`ClipboardShmMsg::len`] bytes the +/// compositor made, for the text of the copy [`MSG_COPY_BEGIN`] announced. +pub const MSG_COPY_REGION: u32 = 13; + +/// The longest clipboard. Policy: a clipboard is text somebody selected, and +/// this is one 2 MiB page, the smallest region the kernel makes. +pub const MAX_CLIPBOARD_BYTES: usize = 2 * 1024 * 1024; + +/// Whether a clipboard of `len` bytes is one the compositor makes a region for. +pub fn copy_fits(len: usize) -> bool { + (1..=MAX_CLIPBOARD_BYTES).contains(&len) +} + /// Either direction: [`surface::LAYOUT_CONFIG`] changed, re-read it. /// /// The compositor translates nothing, so it does not act on this — it is the @@ -163,8 +188,8 @@ toyos::ipc_payload! { pub h: u32, } - /// How much of the region that travels with this message is text. The - /// region itself is one transferred handle, sent ahead of the frame. + /// How many bytes of clipboard text a copy region holds. Where a region + /// travels with the message, it is one handle sent ahead of the frame. pub struct ClipboardShmMsg { pub len: u32, } @@ -353,31 +378,71 @@ pub fn load_layout(translator: &mut Translator) { } } +/// Why [`clipboard_set`] put nothing on the clipboard. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CopyError { + /// This program was given no `compositor` connector. + NotEndowed, + /// The compositor exited, or the connection died mid-copy. + CompositorGone, + /// The compositor answered, and not with anything this exchange allows. + Protocol(u32), + /// Longer than [`MAX_CLIPBOARD_BYTES`]. + TooLong(usize), +} + +impl std::fmt::Display for CopyError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::NotEndowed => write!(f, "this program was given no compositor"), + Self::CompositorGone => write!(f, "the compositor is gone"), + Self::Protocol(msg_type) => { + write!(f, "the compositor answered with message type {msg_type}") + } + Self::TooLong(len) => { + write!(f, "{len} bytes is past the clipboard's {MAX_CLIPBOARD_BYTES}") + } + } + } +} + +impl std::error::Error for CopyError {} + +impl From for CopyError { + fn from(e: EndowError) -> Self { + match e { + EndowError::NotEndowed => Self::NotEndowed, + EndowError::ServerGone | EndowError::Refused(_) => Self::CompositorGone, + } + } +} + /// Put `text` on the system clipboard, over a connection of its own. /// -/// Fallible where it used to `expect`: a program the manifest gives no -/// compositor is a program that cannot copy, which is an answer and not a -/// reason to take the caller down. -pub fn clipboard_set(text: &str) -> Result<(), CreateError> { - use std::sync::Mutex; - static CLIPBOARD_SHM: Mutex> = Mutex::new(None); - - let conn = endow::service("compositor")?; +/// Past [`MAX_INLINE_PAYLOAD`] the text goes into a region the compositor +/// made for it ([`MSG_COPY_BEGIN`]). +pub fn clipboard_set(text: &str) -> Result<(), CopyError> { let bytes = text.as_bytes(); + let gone = |_: ipc::IpcError| CopyError::CompositorGone; if bytes.len() <= MAX_INLINE_PAYLOAD { - let _ = conn.send_bytes(MSG_CLIPBOARD_SET, bytes); - } else if let Ok(mut shm) = SharedMemory::create(bytes.len()) { - shm.as_mut_slice()[..bytes.len()].copy_from_slice(bytes); - if let Ok(handle) = shm.share() { - let _ = conn.send_with_handles( - &[handle], - MSG_CLIPBOARD_SET_SHM, - &ClipboardShmMsg { len: bytes.len() as u32 }, - ); - } - *CLIPBOARD_SHM.lock().unwrap() = Some(shm); + let conn = endow::service("compositor")?; + return conn.send_bytes(MSG_CLIPBOARD_SET, bytes).map_err(gone); } - Ok(()) + if !copy_fits(bytes.len()) { + return Err(CopyError::TooLong(bytes.len())); + } + let conn = endow::service("compositor")?; + conn.send(MSG_COPY_BEGIN, &ClipboardShmMsg { len: bytes.len() as u32 }).map_err(gone)?; + let header = conn.recv_header().map_err(gone)?; + if header.msg_type != MSG_COPY_REGION { + return Err(CopyError::Protocol(header.msg_type)); + } + let _: ClipboardShmMsg = conn.recv_payload(&header).map_err(gone)?; + let [region] = conn.recv_handles_exact::<1>().ok_or(CopyError::CompositorGone)?; + let mut region = + SharedMemory::adopt(region, bytes.len()).map_err(|_| CopyError::CompositorGone)?; + region.as_mut_slice().copy_from_slice(bytes); + conn.signal(MSG_COPY_COMMIT).map_err(gone) } pub struct Window { From 28db806bca3800679d4b2e1b98225b7f498beea6 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 23:50:19 +0200 Subject: [PATCH 2/9] Copy-once is a type; the compositor forbids unsafe code; a refused move closes its handle - copy_begin moves the region's handle with handle_send on its own and closes it when the move is refused, then sends MSG_COPY_REGION bare. A client that begins a copy and closes before the answer used to cost the compositor one handle slot and one region per try: the kernel restores a refused batch at its own numbers, and nothing closed it. A handle is never closed after a successful move. No guest can trigger this deterministically, since it is the client's close racing the send. The split is the fix. The same leak in deliver_with_handles (window creation, paste, resize) is filed. - CopyRegion (client.rs) wraps the compositor's copy region. Its one method, take(self), reads every byte once through SharedMemory::as_atomic. Pending connections and frames carry Option, and copy_out is gone. Reading the region in place or reading it a second time no longer compiles. - The copy state is decided in one match on (frame.copy, msg_type): a connection holding a region may send its commit and nothing else, and a commit comes only on such a connection. Deleting the refusal arm makes the match non-exhaustive. - MSG_COPY_BEGIN's payload must be exactly one ClipboardShmMsg. Trailing bytes drop the client as out of protocol. - MSG_RETIRED_CLIPBOARD_SET_SHM drops its client with its own reason, DropReason::Retired, so the host can tell that refusal from every other out-of-protocol drop. - The compositor is #![forbid(unsafe_code)]. The window blit reads SharedMemory::as_slice, and the copy reads SharedMemory::as_atomic, added in toyos/src/shm.rs. The clippy deny and its issue are deleted. - toyos-window: CopyError keeps only what is its own, TooLong and Compositor(CreateError). CreateError reports CompositorGone only for a closed port. It reports ConnectRefused for a kernel refusal, BrokenOff for a connection that failed mid-exchange (the compositor exited, or refused by closing), Protocol for an answer that came without its region, and Unmappable for a region that would not map. - terminal and editor report a refused copy instead of discarding it, and an editor cut whose copy was refused keeps its text. - compositor_hostile_clipboard: the rewritten-while-read case is replaced by a copy of 0xFF bytes, whose paste must be the clipboard from before and whose refusal the host requires by name. The after-commit case overwrites with text, so a read at paste would be pasted. New cases: a second MSG_COPY_BEGIN on a copy connection, and a begin with trailing bytes. Each must end in a hangup. - Filed: issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md. netd is added to a-received-handle-has-no-knowable-type.md. Co-Authored-By: Claude Opus 5.5 --- issues/build/no-gate-lints-the-compositor.md | 21 --- .../a-received-handle-has-no-knowable-type.md | 3 + ...e-move-leaves-the-compositor-holding-it.md | 36 +++++ .../src/bin/compositor_hostile_clipboard.rs | 131 ++++++++---------- tests/toyos.rs | 26 ++-- toyos/src/shm.rs | 9 ++ userland/compositor/src/client.rs | 27 +++- userland/compositor/src/main.rs | 2 +- userland/compositor/src/render.rs | 7 +- userland/compositor/src/session.rs | 77 +++++----- userland/editor/src/main.rs | 13 +- userland/terminal/src/main.rs | 10 +- userland/toyos-window/src/lib.rs | 115 ++++++++------- 13 files changed, 263 insertions(+), 214 deletions(-) delete mode 100644 issues/build/no-gate-lints-the-compositor.md create mode 100644 issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md diff --git a/issues/build/no-gate-lints-the-compositor.md b/issues/build/no-gate-lints-the-compositor.md deleted file mode 100644 index 19d68a87aa4..00000000000 --- a/issues/build/no-gate-lints-the-compositor.md +++ /dev/null @@ -1,21 +0,0 @@ ---- -status: open -kind: tooling -opened: 2026-09-27 ---- - -# No gate runs clippy over the compositor - -`userland/compositor/src/main.rs` denies `clippy::undocumented_unsafe_blocks`, -and nothing in `src/clippy.rs` or `.github/workflows/` runs clippy over -`userland/`, so the attribute holds only when someone runs it by hand. The -`toyos` toolchain has no clippy, but the compositor checks with the host's: - - cd userland && cargo +stable clippy -p compositor --no-deps \ - --target aarch64-apple-darwin -- -A clippy::all - -exits 0 and reds on an unsafe block with no `SAFETY:` comment. Without -`-A clippy::all` it reds on one default lint in `Session::tick_taskbar`. - -**Exit**: a shape in `src/clippy.rs` runs that command for the compositor on -every host architecture the gate runs on, and fails on its findings. diff --git a/issues/isolation/a-received-handle-has-no-knowable-type.md b/issues/isolation/a-received-handle-has-no-knowable-type.md index f26562face8..ebbb2f4c887 100644 --- a/issues/isolation/a-received-handle-has-no-knowable-type.md +++ b/issues/isolation/a-received-handle-has-no-knowable-type.md @@ -33,6 +33,9 @@ typed can be ended by whoever sent it.** The sender needs nothing but - A window client receives its buffer the same way (`userland/toyos-window/src/lib.rs`). - blockd maps the region a client sends with its open (`Region::adopt` in `userland/blockd/src/region.rs`), so a *client* holding its connector ends it. +- netd adopts as pipes the two handles a client sends with a piped socket + (`DataPipes::take`) and the one with a piped bind (`handle_tcp_bind_piped`), + both in `userland/netd/src/main.rs`: a *client* ends it the same way. Nothing in the tree is hostile today, so nothing fails. The property the architecture claims — that a process cannot be harmed by what it was not given — diff --git a/issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md b/issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md new file mode 100644 index 00000000000..c0ac07c6d09 --- /dev/null +++ b/issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md @@ -0,0 +1,36 @@ +--- +status: open +kind: defect +opened: 2026-09-27 +--- + +# A refused handle move leaves the compositor holding what it meant to send + +`deliver_with_handles` (`userland/compositor/src/client.rs`) sends through +`Connection::try_send_with_handles`, which calls `syscall::handle_send` and +then the frame. When `handle_send` itself is refused, the kernel restores every +handle at its own number (`sys_handle_send` in `kernel/src/syscall/ipc.rs`). +The compositor drops the client, but it still holds the handle and never +closes it. The function's doc says the handles are moved whether or not the +frame lands, and that is false for this refusal. + +Each refusal keeps one handle slot and one region. Three sites are affected: + +- `create_window`'s `MSG_WINDOW_CREATED`: a client that closes before the + answer arrives. +- `paste`'s `MSG_CLIPBOARD_PASTE_SHM`. +- `rebuffer`'s `MSG_WINDOW_RESIZED`, reached by any app through + `MSG_SET_RESOLUTION`, which reallocates every window's buffer. A window that + never takes its handles fills its `MAX_QUEUED_BATCHES` queue, and the next + move is refused. + +A client can repeat this, and nothing bounds it before the handle table or +memory runs out. + +Owner: the compositor's client delivery, `deliver_with_handles` in +`userland/compositor/src/client.rs`. + +**Exit**: `deliver_with_handles` calls `syscall::handle_send` on its own and +closes the handles if that is refused, then sends the frame. It never closes a +handle after a successful move. `copy_begin` in +`userland/compositor/src/session.rs` already has this shape. diff --git a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs index 369081bf330..28fa234b8b5 100644 --- a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs +++ b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs @@ -1,6 +1,3 @@ -//! Two hostile clipboards, and a compositor that must outlive both and keep -//! only text it validated. -//! //! Needs a live compositor and a host that types GUI+V, which the shared boot //! does not have — it is in `RUST_SKIP` and `metal_sim_hostile_clipboard` runs //! it on the metal-sim profile. @@ -10,23 +7,23 @@ //! must refuse the client without ever receiving the handle — and the pipe's //! writer, queued on the refused connection, must go back to the kernel //! unused. -//! 2. **A region rewritten while it is read.** The client commits a copy of -//! `A`s and keeps rewriting the region between `A`s and bytes that are not -//! UTF-8. The compositor may keep the text or refuse it, and the paste that -//! follows has to be one of the two texts it could have validated. +//! 2. **A copy that is not UTF-8.** The client commits a region of `0xFF`, and +//! a paste has to be the clipboard from before it. //! 3. **A region rewritten after its commit.** Once the compositor has closed //! the connection the region is the client's own again, and a paste has to //! be what was committed, not what the region holds now. //! 4. **A copy never committed.** The client holds its region and says nothing; -//! the compositor has to drop it by name rather than hold the region. +//! the compositor has to drop it by name. +//! 5. **A second begin.** A connection holding a region may send its commit and +//! nothing else, so a second `MSG_COPY_BEGIN` on it is refused rather than +//! answered with another region. +//! 6. **A begin with bytes past its length.** Refused rather than answered. //! //! Each case ends with a probe the compositor answers from its dispatch, under //! a deadline. The host asserts what this side cannot see: no handle fault and //! no compositor exit in the kernel's records, and the refusals named. -use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; -use std::sync::Arc; -use std::thread; +use std::sync::atomic::{AtomicU8, Ordering}; use std::time::{Duration, Instant}; use toyos::endow; @@ -70,22 +67,21 @@ fn main() { wrong_typed_handle(); probe("a wrong-typed handle"); - rewritten_while_read(); - probe("a region rewritten while it is read"); - let what = "the paste after a rewrite during the read"; + let what = "a copy that is not UTF-8"; + commit_filled(what, 0xFF); + probe(what); let first = paste(&mut target, what, None); - let all_a = first.len() == COPY_LEN && first.iter().all(|&b| b == b'A'); - if first != BEFORE.as_bytes() && !all_a { - fail(what, &describe(&first, b'A')); + if first != BEFORE.as_bytes() { + let len = first.len(); + fail(what, &format!("the paste was {len} bytes, not the clipboard from before")); } - println!( - "hostile clipboard: the rewritten copy was {}", - if all_a { "kept whole" } else { "refused" } - ); - rewritten_after_commit(); - probe("a region rewritten after its commit"); - let what = "the paste after a rewrite past the commit"; + let what = "a region rewritten after its commit"; + let region = commit_filled(what, b'C'); + // Text too, so a read of the region at the paste is pasted rather than + // refused, and differs from the stale paste `paste` skips. + fill(®ion, b'D'); + probe(what); let second = paste(&mut target, what, Some(&first)); if second.len() != COPY_LEN || second.iter().any(|&b| b != b'C') { fail(what, &describe(&second, b'C')); @@ -96,7 +92,23 @@ fn main() { await_hangup(conn.as_handle(), what, "the compositor giving up on the commit"); probe(what); - println!("hostile clipboard: 4 cases survived, compositor still serving"); + let what = "a second begin on a copy"; + let (conn, _region) = begin_copy(what); + conn.send(COPY_BEGIN, &window::ClipboardShmMsg { len: COPY_LEN as u32 }) + .unwrap_or_else(|e| fail(what, &format!("could not begin again: {e:?}"))); + await_hangup(conn.as_handle(), what, "the compositor's refusal"); + probe(what); + + let what = "a begin with bytes past its length"; + let conn = connect(what); + let mut begin = (COPY_LEN as u32).to_ne_bytes().to_vec(); + begin.extend_from_slice(&[0; 4]); + conn.send_bytes(COPY_BEGIN, &begin) + .unwrap_or_else(|e| fail(what, &format!("could not begin: {e:?}"))); + await_hangup(conn.as_handle(), what, "the compositor's refusal"); + probe(what); + + println!("hostile clipboard: every case survived, compositor still serving"); } /// A pipe end where the retired message carried a region. @@ -117,45 +129,14 @@ fn wrong_typed_handle() { syscall::close(ends.read); } -/// Commit `A`s, then rewrite the region while the compositor copies it. -fn rewritten_while_read() { - let what = "a region rewritten while it is read"; +/// Commit a whole copy of `byte`, and wait for the compositor to be done with +/// it. +fn commit_filled(what: &str, byte: u8) -> SharedMemory { let (conn, region) = begin_copy(what); - for byte in bytes(®ion) { - byte.store(b'A', Ordering::Relaxed); - } - let stop = Arc::new(AtomicBool::new(false)); - let rewriter = { - let stop = Arc::clone(&stop); - thread::spawn(move || { - let mut fill = 0xFF; - while !stop.load(Ordering::Relaxed) { - for byte in bytes(®ion) { - byte.store(fill, Ordering::Relaxed); - } - fill = if fill == b'A' { 0xFF } else { b'A' }; - } - }) - }; + fill(®ion, byte); conn.signal(COPY_COMMIT).unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); await_hangup(conn.as_handle(), what, "the compositor closing the copy"); - stop.store(true, Ordering::Relaxed); - rewriter.join().unwrap_or_else(|_| fail(what, "the rewriter panicked")); -} - -/// Commit `C`s, wait for the compositor to be done with them, then overwrite -/// the region. -fn rewritten_after_commit() { - let what = "a region rewritten after its commit"; - let (conn, region) = begin_copy(what); - for byte in bytes(®ion) { - byte.store(b'C', Ordering::Relaxed); - } - conn.signal(COPY_COMMIT).unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); - await_hangup(conn.as_handle(), what, "the compositor closing the copy"); - for byte in bytes(®ion) { - byte.store(0xFF, Ordering::Relaxed); - } + region } /// A connection holding the region the compositor made for a whole copy. @@ -165,14 +146,15 @@ fn begin_copy(what: &str) -> (Connection, SharedMemory) { .unwrap_or_else(|e| fail(what, &format!("could not begin: {e:?}"))); await_readable(conn.as_handle(), what, "the compositor's region"); let header = conn.recv_header().unwrap_or_else(|e| fail(what, &format!("no answer: {e:?}"))); - if header.msg_type != COPY_REGION { - fail(what, &format!("the compositor answered with message type {}", header.msg_type)); - } - let info: window::ClipboardShmMsg = conn - .recv_payload(&header) - .unwrap_or_else(|e| fail(what, &format!("no length: {e:?}"))); - if info.len as usize != COPY_LEN { - fail(what, &format!("a region of {} bytes for a copy of {COPY_LEN}", info.len)); + if header.msg_type != COPY_REGION || header.len() != 0 { + fail( + what, + &format!( + "the compositor answered with message type {} and {} bytes", + header.msg_type, + header.len() + ), + ); } let [region] = conn.recv_handles_exact::<1>().unwrap_or_else(|| fail(what, "the answer had no region")); @@ -181,6 +163,12 @@ fn begin_copy(what: &str) -> (Connection, SharedMemory) { (conn, region) } +fn fill(region: &SharedMemory, byte: u8) { + for b in bytes(region) { + b.store(byte, Ordering::Relaxed); + } +} + /// The region as the only type that may alias memory another process reads. fn bytes(region: &SharedMemory) -> &[AtomicU8] { // SAFETY: the mapping is `region.len()` bytes and `region` outlives the @@ -211,12 +199,11 @@ fn paste(target: &mut Window, what: &str, stale: Option<&[u8]>) -> Vec { } } -/// A paste that is neither validated text, summarised — never printed whole. +/// A paste, summarised — never printed whole. fn describe(text: &[u8], fill: u8) -> String { let stray = text.iter().position(|&b| b != fill); format!( - "the paste was {} bytes, UTF-8: {}, first byte that is not {:?} at {stray:?} — the \ - compositor kept bytes it had not validated", + "the paste was {} bytes, UTF-8: {}, first byte that is not {:?} at {stray:?}", text.len(), std::str::from_utf8(text).is_ok(), fill as char diff --git a/tests/toyos.rs b/tests/toyos.rs index 498bd6f91d3..22e67a35aa0 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -755,7 +755,7 @@ const MACHINE_TESTS: &[(&str, Sched, Tier)] = &[ // slow on purpose. Its own boot too: it leaves the pointer somewhere else // and the window in a different place than it found them. ("metal_sim_window_drag", Sched::Serial, Tier::Nightly), - // A client's clipboard, hostile two ways; no clock in any verdict. Its own + // A client's clipboard; no clock in any verdict. Its own // boot: the compositor it abuses has to be one nothing else has touched. ("metal_sim_hostile_clipboard", Sched::Parallel, Tier::Fast), // A host-measured drain rate with an 8 s ceiling on a 3.3 s expectation. @@ -7731,9 +7731,6 @@ fn metal_sim_window_drag(rust_bins: &[(String, Vec)]) -> Result<(), String> Ok(()) } -/// A client's clipboard sent two hostile ways: a pipe where a region went, and -/// a region rewritten while the compositor reads it. -/// /// The guest runs the cases and asks for a paste after each copy; this half /// types GUI+V at every ask and asserts what the guest cannot see. **The /// kernel's record is the independent half**: a compositor that maps the pipe @@ -7789,30 +7786,27 @@ fn metal_sim_hostile_clipboard(rust_bins: &[(String, Vec)]) -> Result<(), St result.exit_code )); } - if !text.contains("hostile clipboard: 4 cases survived") { - return Err(format!("the guest did not report its four cases survived:\n{text}")); + if !text.contains("hostile clipboard: every case survived") { + return Err(format!("the guest did not report that every case survived:\n{text}")); } if !text.contains("it began a copy and never committed it") { return Err(format!( "the client that held its region and never committed was not dropped by name:\n{text}" )); } - const REFUSED: &str = "it sent a frame this protocol cannot describe"; - if !text.lines().any(|l| l.contains("compositor: dropping client") && l.contains(REFUSED)) { + // The compositor's `DropReason::Retired`, which no other case produces. + const RETIRED: &str = "it sent the retired clipboard region"; + if !text.lines().any(|l| l.contains("compositor: dropping client") && l.contains(RETIRED)) { return Err(format!( "the client that sent a pipe where a region went was not refused by name:\n{text}" )); } - if text.contains("hostile clipboard: the rewritten copy was refused") - && !text.contains("compositor: refusing a clipboard from client") - { - return Err(format!( - "the rewritten copy never reached the clipboard and the compositor never said \ - why:\n{text}" - )); + const NOT_UTF8: [&str; 2] = ["compositor: refusing a clipboard from client", "it is not UTF-8"]; + if !text.lines().any(|l| NOT_UTF8.iter().all(|s| l.contains(s))) { + return Err(format!("the copy that is not UTF-8 was not refused by name:\n{text}")); } serial::Serial::named("boot console", result.serial.as_str()).must_be_clean()?; - eprintln!(" [metal-sim] a pipe refused unused, both rewritten regions read once"); + eprintln!(" [metal-sim] a pipe refused unused"); Ok(()) } diff --git a/toyos/src/shm.rs b/toyos/src/shm.rs index 03ad9bec45a..f81eec0e62a 100644 --- a/toyos/src/shm.rs +++ b/toyos/src/shm.rs @@ -9,6 +9,8 @@ //! The mapping goes away with the last handle, so dropping this is all the //! cleanup there is. +use core::sync::atomic::AtomicU8; + use toyos_abi::syscall::{self, SyscallError}; use crate::{AsHandle, OwnedHandle, RawHandle}; @@ -72,6 +74,13 @@ impl SharedMemory { pub fn as_mut_slice(&mut self) -> &mut [u8] { unsafe { core::slice::from_raw_parts_mut(self.ptr, self.size) } } + + /// The region as the one type that may alias memory a peer writes. + pub fn as_atomic(&self) -> &[AtomicU8] { + // SAFETY: the mapping is `size` bytes and lives as long as `self`, and + // `AtomicU8` has `u8`'s size and alignment. + unsafe { core::slice::from_raw_parts(self.ptr as *const AtomicU8, self.size) } + } } impl AsHandle for SharedMemory { diff --git a/userland/compositor/src/client.rs b/userland/compositor/src/client.rs index 758b24b3214..e4b8c8591e5 100644 --- a/userland/compositor/src/client.rs +++ b/userland/compositor/src/client.rs @@ -13,6 +13,7 @@ //! used it — so every region a client writes is one the compositor made, and a //! handle a client sends stays queued until its connection closes. +use std::sync::atomic::Ordering; use std::time::{Duration, Instant}; use toyos::shm::SharedMemory; @@ -79,7 +80,7 @@ pub struct ClientFrame { payload_len: usize, pub conn: Option, /// The connection's [`PendingConn::copy`]. - pub copy: Option, + pub copy: Option, } impl ClientFrame { @@ -116,7 +117,25 @@ pub struct PendingConn { /// The region a `MSG_COPY_BEGIN` was answered with. A connection holding /// one may send `MSG_COPY_COMMIT` and nothing else, within /// [`HANDSHAKE_TIMEOUT`] of `since`. - pub copy: Option, + pub copy: Option, +} + +/// A region made for one client's copy, which that client maps and may still +/// be writing. +/// +/// **Read once, and only by being taken.** Its one method consumes it, so +/// nothing here can read the region twice or validate it in place. +pub struct CopyRegion(SharedMemory); + +impl CopyRegion { + pub fn new(region: SharedMemory) -> Self { + Self(region) + } + + /// Every byte, each read once, into memory no client can write. + pub fn take(self) -> Vec { + self.0.as_atomic().iter().map(|b| b.load(Ordering::Relaxed)).collect() + } } /// Why a client is going. @@ -134,6 +153,9 @@ pub enum DropReason { /// A frame no protocol here can produce. The next message boundary is /// unlocatable, so there is nothing to resynchronise to. OutOfProtocol, + /// `window::MSG_RETIRED_CLIPBOARD_SET_SHM`, whose handle this compositor + /// never takes. + Retired, /// Its pipe would not take a whole frame — an entire pipe of messages it /// has not read. NotReading, @@ -149,6 +171,7 @@ impl DropReason { pub fn why(self) -> &'static str { match self { Self::OutOfProtocol => "it sent a frame this protocol cannot describe", + Self::Retired => "it sent the retired clipboard region, whose handle is never taken", Self::NotReading => "its pipe will not take another message and it is not reading", Self::Gone => "its connection is gone", Self::HandshakeTimeout => "it never finished its first message", diff --git a/userland/compositor/src/main.rs b/userland/compositor/src/main.rs index c53c669a911..e0c199c0078 100644 --- a/userland/compositor/src/main.rs +++ b/userland/compositor/src/main.rs @@ -14,7 +14,7 @@ //! What is left in this file is the policy the other three read: the numbers //! that are decisions rather than derivations. -#![deny(clippy::undocumented_unsafe_blocks)] +#![forbid(unsafe_code)] mod client; mod render; diff --git a/userland/compositor/src/render.rs b/userland/compositor/src/render.rs index 07277d1e941..468977a1067 100644 --- a/userland/compositor/src/render.rs +++ b/userland/compositor/src/render.rs @@ -179,12 +179,7 @@ fn draw_window( } if let Some(b) = content_blit(win, clip) { - // SAFETY: the mapping is `shm.len()` bytes and `win` holds it past this - // borrow; its client writes it concurrently, which can tear pixels but - // never reach outside the mapping. - let buffer = unsafe { - std::slice::from_raw_parts(win.client.shm.as_ptr(), win.client.shm.len()) - }; + let buffer = win.client.shm.as_slice(); let offset = (b.src_y as usize * win.buf_w as usize + b.src_x as usize) * 4; surface.blit( b.dst.x0 as usize, diff --git a/userland/compositor/src/session.rs b/userland/compositor/src/session.rs index ee23c363ce5..5d0d6e6886c 100644 --- a/userland/compositor/src/session.rs +++ b/userland/compositor/src/session.rs @@ -8,7 +8,6 @@ //! to the panel. use std::process::Command; -use std::sync::atomic::{AtomicU8, Ordering}; use std::time::{Duration, Instant}; use toyos::endow; @@ -17,7 +16,7 @@ use toyos::poller::{Poller, READABLE}; use toyos::port::Acceptor; use toyos::shm::SharedMemory; use toyos::{ipc, system, AsHandle, FramebufferDev, Keyboard, Mouse}; -use toyos_abi::syscall::DeviceType; +use toyos_abi::syscall::{self, DeviceType}; use toyos_abi::RawHandle; use toyos_desktop::{ cursor_from_abs, cursor_style, fold_mouse, hit_test, key_action, set_mode, tab_action, Chrome, @@ -28,8 +27,8 @@ use window::Screen; use crate::client::{ announce, deliver, deliver_signal, deliver_with_handles, mark_dead, note_closed, note_opened, - Client, ClientFrame, ClientRx, Dead, DropReason, PendingConn, Win, HANDSHAKE_TIMEOUT, - MAX_KEPT_PAYLOAD, MAX_PENDING_CONNS, + Client, ClientFrame, ClientRx, CopyRegion, Dead, DropReason, PendingConn, Win, + HANDSHAKE_TIMEOUT, MAX_KEPT_PAYLOAD, MAX_PENDING_CONNS, }; use crate::render::{self, Assets, BackBuffer, SystemStats, TitleBarIcons}; use crate::stats::{FrameStats, FrameTotals}; @@ -381,16 +380,16 @@ impl Session { let (events, torn) = buf[..n].as_chunks::(); assert!(torn.is_empty(), "compositor: the keyboard read {n} bytes, not whole events"); for raw in events { - let event = &ipc::decode_payload::(raw) + let event = ipc::decode_payload::(raw) .expect("a chunk is exactly one event long"); let focused = self.stack.focused(); let action = - key_action((*event).into(), focused.map(|i| self.stack[i].mode), self.launcher_open); + key_action(event.into(), focused.map(|i| self.stack[i].mode), self.launcher_open); match action { KeyAction::Ignore => {} KeyAction::Forward => { if let Some(i) = focused { - deliver(&mut self.dead, &self.stack[i], window::MSG_KEY_INPUT, event); + deliver(&mut self.dead, &self.stack[i], window::MSG_KEY_INPUT, &event); } } KeyAction::CloseLauncher => { @@ -730,7 +729,7 @@ impl Session { } fn dispatch(&mut self, frames: Vec) { - for frame in frames { + for mut frame in frames { let handle = frame.handle; // A payload filling the kept buffer declared more than any client // may inline, so what arrived is a prefix of what was sent. @@ -743,14 +742,22 @@ impl Session { mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); continue; } - if frame.copy.is_some() && frame.msg_type != window::MSG_COPY_COMMIT { - mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); - continue; + // A connection holding a region may send its commit and nothing + // else, and a commit is only ever of a region held. + match (frame.copy.take(), frame.msg_type) { + (Some(region), window::MSG_COPY_COMMIT) => { + self.copy_commit(frame, region); + continue; + } + (Some(_), _) | (None, window::MSG_COPY_COMMIT) => { + mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); + continue; + } + (None, _) => {} } match frame.msg_type { window::MSG_CREATE_WINDOW => self.create_window(frame), window::MSG_COPY_BEGIN => self.copy_begin(frame), - window::MSG_COPY_COMMIT => self.copy_commit(frame), window::MSG_PRESENT => { let Ok(rect) = ipc::decode_payload::(frame.payload()) else { mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); @@ -773,7 +780,7 @@ impl Session { } window::MSG_CLIPBOARD_SET => self.set_clipboard(handle, frame.payload().to_vec()), window::MSG_RETIRED_CLIPBOARD_SET_SHM => { - mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); + mark_dead(&mut self.dead, handle, DropReason::Retired); } window::MSG_LAYOUT_CHANGED => { // The compositor is the root of the surface tree and @@ -945,8 +952,10 @@ impl Session { /// and hold the connection for its commit. fn copy_begin(&mut self, frame: ClientFrame) { let handle = frame.handle; + // Exactly one `ClipboardShmMsg`: bytes past it are not this protocol. + let exact = frame.payload().len() == std::mem::size_of::(); let info = ipc::decode_payload::(frame.payload()); - let (Ok(info), Some(conn)) = (info, frame.conn) else { + let (Ok(info), Some(conn), true) = (info, frame.conn, exact) else { mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); return; }; @@ -979,7 +988,14 @@ impl Session { return; } }; - if let Err(e) = conn.try_send_with_handles(&[theirs], window::MSG_COPY_REGION, &info) { + // Moved on its own, so that a refused move leaves `theirs` here to + // close: once moved, its number is no longer this process's to close. + if let Err(e) = syscall::handle_send(conn.as_handle(), &[theirs]) { + syscall::close(theirs); + mark_dead(&mut self.dead, handle, ipc::TrySendError::Syscall(e).into()); + return; + } + if let Err(e) = conn.try_signal(window::MSG_COPY_REGION) { mark_dead(&mut self.dead, handle, e.into()); return; } @@ -987,20 +1003,18 @@ impl Session { conn, rx: ClientRx::new(), since: Instant::now(), - copy: Some(region), + copy: Some(CopyRegion::new(region)), }); } - /// `MSG_COPY_COMMIT`: take the text out of the region once, and let the - /// connection close. - fn copy_commit(&mut self, frame: ClientFrame) { - let handle = frame.handle; - let bare = frame.payload().is_empty(); - let (Some(region), true) = (frame.copy, bare) else { - mark_dead(&mut self.dead, handle, DropReason::OutOfProtocol); + /// `MSG_COPY_COMMIT` on a connection holding `region`: take the text out + /// once, and let the connection close. + fn copy_commit(&mut self, frame: ClientFrame, region: CopyRegion) { + if !frame.payload().is_empty() { + mark_dead(&mut self.dead, frame.handle, DropReason::OutOfProtocol); return; - }; - self.set_clipboard(handle, copy_out(®ion)); + } + self.set_clipboard(frame.handle, region.take()); } /// The clipboard, from bytes that are the compositor's own: validated @@ -1377,19 +1391,6 @@ fn desk_of(screen: &Screen, font: &font::Font, apps: usize) -> Desk { } } -/// Every byte of `region`, each read once. -/// -/// The client still maps the region and may be writing it, so this copy is -/// the only read of it and the only thing validated. -fn copy_out(region: &SharedMemory) -> Vec { - // SAFETY: the mapping is `region.len()` bytes and `region` outlives the - // borrow; an atomic is the one type that may alias memory another process - // writes. - let bytes = - unsafe { std::slice::from_raw_parts(region.as_ptr() as *const AtomicU8, region.len()) }; - bytes.iter().map(|b| b.load(Ordering::Relaxed)).collect() -} - fn read_sprite(path: &str, size: u32, color: [u8; 3]) -> sprite::Sprite { let svg = std::fs::read(path).unwrap_or_else(|e| panic!("failed to read {path}: {e}")); sprite::Sprite::from_svg_colored(&svg, size, color) diff --git a/userland/editor/src/main.rs b/userland/editor/src/main.rs index 34293a33460..d35e80d2dfd 100644 --- a/userland/editor/src/main.rs +++ b/userland/editor/src/main.rs @@ -1610,13 +1610,20 @@ fn handle_key(editor: &mut Editor, key: &KeyPress, fb: &mut Framebuffer) { } Some('c') => { if let Some(text) = editor.selected_text() { - window::clipboard_set(&text).ok(); + if let Err(e) = window::clipboard_set(&text) { + eprintln!("editor: nothing was copied — {e}"); + } } } Some('x') => { if let Some(text) = editor.selected_text() { - window::clipboard_set(&text).ok(); - editor.delete_selection(); + // A cut whose copy was refused keeps its text. + match window::clipboard_set(&text) { + Ok(()) => { + editor.delete_selection(); + } + Err(e) => eprintln!("editor: nothing was cut — {e}"), + } } } Some('v') => {} // Paste handled via ClipboardPaste event diff --git a/userland/terminal/src/main.rs b/userland/terminal/src/main.rs index 793456fbd16..49cdc2f5f0e 100644 --- a/userland/terminal/src/main.rs +++ b/userland/terminal/src/main.rs @@ -38,6 +38,12 @@ fn present(console: &Console, window: &Window) { } } +fn copy(text: &str) { + if let Err(e) = window::clipboard_set(text) { + eprintln!("terminal: nothing was copied — {e}"); + } +} + fn main() { // **This terminal's surface is a port it makes, not a name it registers.** // One per instance: the connector goes into the namespace of the shell it @@ -148,7 +154,7 @@ fn main() { window::Event::KeyInput(key) if key.gui() && key.keycode == 0x06 => { // Cmd+C: copy selection to clipboard if let Some(text) = console.get_selection() { - window::clipboard_set(&text).ok(); + copy(&text); } } window::Event::KeyInput(key) => { @@ -175,7 +181,7 @@ fn main() { } window::MOUSE_RELEASE if ev.changed == 1 => { if let Some(text) = console.mouse_up(col, row) { - window::clipboard_set(&text).ok(); + copy(&text); } present(&console, &window); } diff --git a/userland/toyos-window/src/lib.rs b/userland/toyos-window/src/lib.rs index 177b1134b0e..465c4f5e52f 100644 --- a/userland/toyos-window/src/lib.rs +++ b/userland/toyos-window/src/lib.rs @@ -6,6 +6,7 @@ pub use framebuffer::{Color, Framebuffer, Screen, Traffic}; pub use wait::{Waiter, Waker, Woke}; /// What [`Window::handle`] answers with, and what a [`Waiter`] waits on. pub use toyos_abi::RawHandle; +use toyos_abi::syscall::SyscallError; use toyos::ipc; use toyos::AsHandle; @@ -69,20 +70,19 @@ pub const MSG_RETIRED_CLIPBOARD_SET_SHM: u32 = 10; /// the compositor made. Payload is a [`ClipboardShmMsg`]. pub const MSG_CLIPBOARD_PASTE_SHM: u32 = 11; -/// Client → compositor, as a connection's first frame: a clipboard of -/// [`ClipboardShmMsg::len`] bytes, which [`copy_fits`]. Answered with -/// [`MSG_COPY_REGION`]. +/// Client → compositor, as a connection's first frame whose payload is exactly +/// a [`ClipboardShmMsg`]: a clipboard of that many bytes, which [`copy_fits`]. +/// Answered with [`MSG_COPY_REGION`]. pub const MSG_COPY_BEGIN: u32 = 13; /// Client → compositor, bare, and the only frame a connection answered with /// [`MSG_COPY_REGION`] may send next: the text is in the region. The compositor /// copies it once and closes the connection. pub const MSG_COPY_COMMIT: u32 = 14; -/// Compositor → client: a region of [`ClipboardShmMsg::len`] bytes the -/// compositor made, for the text of the copy [`MSG_COPY_BEGIN`] announced. +/// Compositor → client, bare: a region the compositor made, as long as the copy +/// [`MSG_COPY_BEGIN`] announced, sent ahead of the frame. pub const MSG_COPY_REGION: u32 = 13; -/// The longest clipboard. Policy: a clipboard is text somebody selected, and -/// this is one 2 MiB page, the smallest region the kernel makes. +/// The longest clipboard. Policy: a clipboard is text somebody selected. pub const MAX_CLIPBOARD_BYTES: usize = 2 * 1024 * 1024; /// Whether a clipboard of `len` bytes is one the compositor makes a region for. @@ -114,17 +114,26 @@ toyos::ipc_payload! { } } -/// Why creating a window failed. +/// Why a request to the compositor failed: a window, or the exchange under a +/// [`CopyError`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CreateError { /// This program was given no `compositor` connector. A statement about /// what the manifest says this program holds, not about the machine — and /// never about timing: every port exists before any server runs. NotEndowed, - /// The compositor exited, or the connection died mid-request. + /// The compositor exited: its port is closed for good. CompositorGone, + /// The kernel refused the connection — a full queue, a full table. + ConnectRefused(SyscallError), + /// The exchange broke off: the compositor closed the connection, having + /// exited or refused (its log says which), or sent a frame shorter than + /// its type. + BrokenOff, /// The compositor answered, and not with anything this exchange allows. Protocol(u32), + /// The region the compositor sent would not map here. + Unmappable(SyscallError), /// The compositor is already holding as many windows as it can afford. AtCapacity, /// The requested size is bigger than the screen it would be drawn on. @@ -140,6 +149,11 @@ impl std::fmt::Display for CreateError { match self { Self::NotEndowed => write!(f, "this program was given no compositor"), Self::CompositorGone => write!(f, "the compositor is gone"), + Self::ConnectRefused(e) => { + write!(f, "the kernel refused a connection to the compositor ({e:?})") + } + Self::BrokenOff => write!(f, "the compositor broke off the exchange"), + Self::Unmappable(e) => write!(f, "the compositor's region would not map ({e:?})"), Self::AtCapacity => write!(f, "the compositor is at its window limit"), Self::TooLarge => write!(f, "the window is larger than the screen"), Self::NoMemory => write!(f, "there is no memory for a window that size"), @@ -157,11 +171,18 @@ impl From for CreateError { fn from(e: EndowError) -> Self { match e { EndowError::NotEndowed => Self::NotEndowed, - EndowError::ServerGone | EndowError::Refused(_) => Self::CompositorGone, + EndowError::ServerGone => Self::CompositorGone, + EndowError::Refused(e) => Self::ConnectRefused(e), } } } +impl From for CreateError { + fn from(_: ipc::IpcError) -> Self { + Self::BrokenOff + } +} + impl CreateError { fn from_wire(reason: u32) -> Self { match reason { @@ -381,39 +402,28 @@ pub fn load_layout(translator: &mut Translator) { /// Why [`clipboard_set`] put nothing on the clipboard. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CopyError { - /// This program was given no `compositor` connector. - NotEndowed, - /// The compositor exited, or the connection died mid-copy. - CompositorGone, - /// The compositor answered, and not with anything this exchange allows. - Protocol(u32), /// Longer than [`MAX_CLIPBOARD_BYTES`]. TooLong(usize), + /// The exchange with the compositor failed. + Compositor(CreateError), } impl std::fmt::Display for CopyError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::NotEndowed => write!(f, "this program was given no compositor"), - Self::CompositorGone => write!(f, "the compositor is gone"), - Self::Protocol(msg_type) => { - write!(f, "the compositor answered with message type {msg_type}") - } Self::TooLong(len) => { write!(f, "{len} bytes is past the clipboard's {MAX_CLIPBOARD_BYTES}") } + Self::Compositor(e) => e.fmt(f), } } } impl std::error::Error for CopyError {} -impl From for CopyError { - fn from(e: EndowError) -> Self { - match e { - EndowError::NotEndowed => Self::NotEndowed, - EndowError::ServerGone | EndowError::Refused(_) => Self::CompositorGone, - } +impl From for CopyError { + fn from(e: CreateError) -> Self { + Self::Compositor(e) } } @@ -423,26 +433,28 @@ impl From for CopyError { /// made for it ([`MSG_COPY_BEGIN`]). pub fn clipboard_set(text: &str) -> Result<(), CopyError> { let bytes = text.as_bytes(); - let gone = |_: ipc::IpcError| CopyError::CompositorGone; - if bytes.len() <= MAX_INLINE_PAYLOAD { - let conn = endow::service("compositor")?; - return conn.send_bytes(MSG_CLIPBOARD_SET, bytes).map_err(gone); - } - if !copy_fits(bytes.len()) { + if bytes.len() > MAX_INLINE_PAYLOAD && !copy_fits(bytes.len()) { return Err(CopyError::TooLong(bytes.len())); } + Ok(copy(bytes)?) +} + +/// [`clipboard_set`]'s exchange, for text the clipboard holds. +fn copy(bytes: &[u8]) -> Result<(), CreateError> { let conn = endow::service("compositor")?; - conn.send(MSG_COPY_BEGIN, &ClipboardShmMsg { len: bytes.len() as u32 }).map_err(gone)?; - let header = conn.recv_header().map_err(gone)?; - if header.msg_type != MSG_COPY_REGION { - return Err(CopyError::Protocol(header.msg_type)); - } - let _: ClipboardShmMsg = conn.recv_payload(&header).map_err(gone)?; - let [region] = conn.recv_handles_exact::<1>().ok_or(CopyError::CompositorGone)?; - let mut region = - SharedMemory::adopt(region, bytes.len()).map_err(|_| CopyError::CompositorGone)?; + if bytes.len() <= MAX_INLINE_PAYLOAD { + return Ok(conn.send_bytes(MSG_CLIPBOARD_SET, bytes)?); + } + conn.send(MSG_COPY_BEGIN, &ClipboardShmMsg { len: bytes.len() as u32 })?; + let header = conn.recv_header()?; + if header.msg_type != MSG_COPY_REGION || header.len() != 0 { + return Err(CreateError::Protocol(header.msg_type)); + } + let [region] = + conn.recv_handles_exact::<1>().ok_or(CreateError::Protocol(MSG_COPY_REGION))?; + let mut region = SharedMemory::adopt(region, bytes.len()).map_err(CreateError::Unmappable)?; region.as_mut_slice().copy_from_slice(bytes); - conn.signal(MSG_COPY_COMMIT).map_err(gone) + Ok(conn.signal(MSG_COPY_COMMIT)?) } pub struct Window { @@ -497,32 +509,29 @@ impl Window { let len = bytes.len().min(30); req.title[..len].copy_from_slice(&bytes[..len]); req.title_len = len as u8; - conn.send(MSG_CREATE_WINDOW, &req).map_err(|_| CreateError::CompositorGone)?; + conn.send(MSG_CREATE_WINDOW, &req)?; // Header first, then the payload the message type calls for: the two // answers carry different structs, and a payload shorter than the type // it was asked for is a refusal from `recv_payload`, not a window. - let header = conn.recv_header().map_err(|_| CreateError::CompositorGone)?; + let header = conn.recv_header()?; match header.msg_type { MSG_WINDOW_CREATED => {} MSG_WINDOW_REFUSED => { - let refused: WindowRefused = conn - .recv_payload(&header) - .map_err(|_| CreateError::CompositorGone)?; + let refused: WindowRefused = conn.recv_payload(&header)?; return Err(CreateError::from_wire(refused.reason)); } other => return Err(CreateError::Protocol(other)), } - let info: WindowInfo = conn - .recv_payload(&header) - .map_err(|_| CreateError::CompositorGone)?; + let info: WindowInfo = conn.recv_payload(&header)?; let buf_size = info.stride as usize * info.height as usize * 4; // The buffer crossed ahead of the frame. A compositor that announced a // window and sent nothing with it is not serving this client, whatever // else it is doing. - let [buffer] = conn.recv_handles_exact::<1>().ok_or(CreateError::CompositorGone)?; - let shm = SharedMemory::adopt(buffer, buf_size).map_err(|_| CreateError::CompositorGone)?; + let [buffer] = + conn.recv_handles_exact::<1>().ok_or(CreateError::Protocol(MSG_WINDOW_CREATED))?; + let shm = SharedMemory::adopt(buffer, buf_size).map_err(CreateError::Unmappable)?; let poller = Poller::new(1); Ok(Self { From c6613b4022504c86dd29f0a7ae7a9db28fb78eec Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 01:17:25 +0200 Subject: [PATCH 3/9] A commit is refused on a window and with a payload; a client's error keeps the kernel's word The compositor: - A refused region move drops the client as `DropReason::Gone`, written directly rather than built from a send error. - `deliver_with_handles` loses the sentence saying its handles move whether or not the frame lands, which is false for a refused `handle_send`. `toyos-window`'s `CreateError`: - `ConnectRefused` and `Unmappable` become one `Kernel(SyscallError)`. Nothing distinguished them but the call that was refused, and an `IpcError::Syscall` now lands there too instead of being discarded as `BrokenOff`. - An answer without its handle is `NoHandle`, not `Protocol` of the type that was right. Its doc names both causes, since `recv_handles_exact` also answers `None` when this process has no room for the handle. - `IpcError::TooLarge` is unreachable: every frame this crate sends is within `MAX_FRAME_LEN`, and a const assertion holds `MAX_INLINE_PAYLOAD` to it. - `Protocol`'s message no longer says the type was wrong, because a payload on `MSG_COPY_REGION` is the right type in the wrong shape. Tests: - `compositor_hostile_clipboard` gains two cases. A commit with a payload over a region of `E`s must leave the clipboard as it was. A commit on a window's connection must lose the window its connection. - `window_refusal` gains readers for `BrokenOff` (the stand-in closes unanswered), `NoHandle` (`MSG_WINDOW_CREATED` with no buffer), and `Kernel` (a port queue filled until the kernel refuses the connection). Filed: - `issues/isolation/sharedmemory-slices-rest-on-a-size-nobody-checks.md` - `issues/design-debt/decode-payload-accepts-bytes-past-its-type.md` - `issues/design-debt/a-client-waits-on-the-compositors-answer-with-no-bound.md` Co-Authored-By: Claude Opus 5.5 --- ...on-the-compositors-answer-with-no-bound.md | 17 ++++++ ...ode-payload-accepts-bytes-past-its-type.md | 19 +++++++ ...e-move-leaves-the-compositor-holding-it.md | 3 +- ...ory-slices-rest-on-a-size-nobody-checks.md | 29 ++++++++++ .../src/bin/compositor_hostile_clipboard.rs | 47 +++++++++++++++ .../src/bin/window_refusal.rs | 57 +++++++++++++++---- userland/compositor/src/client.rs | 4 -- userland/compositor/src/session.rs | 4 +- userland/toyos-window/src/lib.rs | 46 ++++++++------- 9 files changed, 187 insertions(+), 39 deletions(-) create mode 100644 issues/design-debt/a-client-waits-on-the-compositors-answer-with-no-bound.md create mode 100644 issues/design-debt/decode-payload-accepts-bytes-past-its-type.md create mode 100644 issues/isolation/sharedmemory-slices-rest-on-a-size-nobody-checks.md diff --git a/issues/design-debt/a-client-waits-on-the-compositors-answer-with-no-bound.md b/issues/design-debt/a-client-waits-on-the-compositors-answer-with-no-bound.md new file mode 100644 index 00000000000..559be9f6410 --- /dev/null +++ b/issues/design-debt/a-client-waits-on-the-compositors-answer-with-no-bound.md @@ -0,0 +1,17 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# A client waits on the compositor's answer with no bound + +`window::clipboard_set` (`userland/toyos-window/src/lib.rs`) blocks in +`recv_header` for `MSG_COPY_REGION` on a copy past `MAX_INLINE_PAYLOAD`, and +`Window::create` blocks the same way for `MSG_WINDOW_CREATED`. Neither wait has +a deadline, so a compositor that is alive and not answering wedges every +terminal or editor that copies, and every program that asks for a window. + +Owner: `toyos-window`. + +**Exit**: each wait has a bound, and a missed one is a `CreateError` of its own. diff --git a/issues/design-debt/decode-payload-accepts-bytes-past-its-type.md b/issues/design-debt/decode-payload-accepts-bytes-past-its-type.md new file mode 100644 index 00000000000..ff918aabc7d --- /dev/null +++ b/issues/design-debt/decode-payload-accepts-bytes-past-its-type.md @@ -0,0 +1,19 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# `ipc::decode_payload` accepts bytes past the type it decodes + +`toyos::ipc::decode_payload` refuses a payload shorter than its `T` and ignores +whatever follows one. Each caller decides for itself whether trailing bytes are +out of protocol, and the compositor's client frames +(`userland/compositor/src/session.rs`) disagree: `copy_begin` refuses them with +a length check of its own, and `MSG_PRESENT`'s `Rect`, `MSG_SET_CURSOR`'s +style, `ResolutionRequest` and `CreateWindowRequest` accept them. + +Owner: `toyos::ipc`. + +**Exit**: one trailing-bytes rule in `toyos::ipc` that every `decode_payload` +caller gets, and `copy_begin`'s own check deleted. diff --git a/issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md b/issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md index c0ac07c6d09..441297e51f3 100644 --- a/issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md +++ b/issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md @@ -11,8 +11,7 @@ opened: 2026-09-27 then the frame. When `handle_send` itself is refused, the kernel restores every handle at its own number (`sys_handle_send` in `kernel/src/syscall/ipc.rs`). The compositor drops the client, but it still holds the handle and never -closes it. The function's doc says the handles are moved whether or not the -frame lands, and that is false for this refusal. +closes it. Each refusal keeps one handle slot and one region. Three sites are affected: diff --git a/issues/isolation/sharedmemory-slices-rest-on-a-size-nobody-checks.md b/issues/isolation/sharedmemory-slices-rest-on-a-size-nobody-checks.md new file mode 100644 index 00000000000..25ca04aaca3 --- /dev/null +++ b/issues/isolation/sharedmemory-slices-rest-on-a-size-nobody-checks.md @@ -0,0 +1,29 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# `SharedMemory`'s safe slices rest on a size nobody checks + +`SharedMemory::as_slice`, `as_mut_slice` and `as_atomic` (`toyos/src/shm.rs`) +are safe functions that make a slice of `size` bytes over the mapping. The type +enforces neither premise that makes this sound: + +- `adopt` is safe and takes `size` from its caller without checking it against + the region the kernel mapped. A server that adopts a peer's region at the + peer's declared length reads and writes past it in safe code. +- `shm_map` is idempotent, so `share()` followed by `adopt` gives two + `SharedMemory` values over one mapping in one process. `as_mut_slice` on one + then aliases `as_slice` or `as_atomic` on the other. + +`#![forbid(unsafe_code)]` on the compositor (`userland/compositor/src/main.rs`) +leans on both. The compositor meets them, since it adopts only regions it +created and the kernel's framebuffer and cursor, but the compiler does not know +that. + +Owner: `toyos::shm`. + +**Exit**: `adopt` is an `unsafe fn` whose contract is both premises, or it +checks `size` against the kernel's region and refuses a second mapping of one +region. diff --git a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs index 28fa234b8b5..a5c5fb03fc1 100644 --- a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs +++ b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs @@ -18,6 +18,10 @@ //! nothing else, so a second `MSG_COPY_BEGIN` on it is refused rather than //! answered with another region. //! 6. **A begin with bytes past its length.** Refused rather than answered. +//! 7. **A commit with a payload.** Refused, so a paste has to be the clipboard +//! from before it and not the region's text. +//! 8. **A commit on a window.** A commit names a region held, so a window +//! sending one loses its connection. //! //! Each case ends with a probe the compositor answers from its dispatch, under //! a deadline. The host asserts what this side cannot see: no handle fault and @@ -27,6 +31,7 @@ use std::sync::atomic::{AtomicU8, Ordering}; use std::time::{Duration, Instant}; use toyos::endow; +use toyos::ipc; use toyos::poller::{Poller, READABLE}; use toyos::shm::SharedMemory; use toyos::{AsHandle, Connection}; @@ -54,6 +59,7 @@ const REMARK: Duration = Duration::from_secs(2); const CEILING: Duration = Duration::from_secs(30); const BEFORE: &str = "hostile clipboard: the text before"; +const AFTER: &str = "hostile clipboard: the text after"; fn main() { // First, so it has the focus: GUI+V pastes into the focused window. @@ -108,9 +114,50 @@ fn main() { await_hangup(conn.as_handle(), what, "the compositor's refusal"); probe(what); + let what = "a commit with a payload"; + set_inline(what, AFTER); + let (conn, region) = begin_copy(what); + fill(®ion, b'E'); + conn.send_bytes(COPY_COMMIT, &[0; 4]) + .unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); + await_hangup(conn.as_handle(), what, "the compositor's refusal"); + probe(what); + let third = paste(&mut target, what, Some(&second)); + if third != AFTER.as_bytes() { + let len = third.len(); + fail(what, &format!("the paste was {len} bytes, not the clipboard from before")); + } + + // Last: the new window takes the focus the pastes went to. + let what = "a commit on a window"; + let mut committing = Window::create_with_title(64, 64, "commit") + .unwrap_or_else(|e| fail(what, &format!("no window: {e}"))); + ipc::signal(committing.handle(), COPY_COMMIT) + .unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); + let deadline = Instant::now() + CEILING; + loop { + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + fail(what, &format!("the compositor kept the window {} s", CEILING.as_secs())); + } + if let Some(Event::Close) = committing.poll_event(left.as_nanos() as u64) { + break; + } + } + probe(what); + println!("hostile clipboard: every case survived, compositor still serving"); } +/// Put `text` on the clipboard inline, and wait for the compositor to be done +/// with it. +fn set_inline(what: &str, text: &str) { + let conn = connect(what); + conn.send_bytes(window::MSG_CLIPBOARD_SET, text.as_bytes()) + .unwrap_or_else(|e| fail(what, &format!("could not set the clipboard: {e:?}"))); + await_hangup(conn.as_handle(), what, "the compositor closing the clipboard"); +} + /// A pipe end where the retired message carried a region. fn wrong_typed_handle() { let what = "a wrong-typed handle"; diff --git a/tests/toyos-rust-tests/src/bin/window_refusal.rs b/tests/toyos-rust-tests/src/bin/window_refusal.rs index 28bbaa3a021..1a495a29bb7 100644 --- a/tests/toyos-rust-tests/src/bin/window_refusal.rs +++ b/tests/toyos-rust-tests/src/bin/window_refusal.rs @@ -12,8 +12,8 @@ //! to take now. Instead it creates a port, builds a namespace mapping //! `"compositor"` to that port's connector, and spawns a child holding it: the //! child's `Window::create` reaches this process and nothing else, no other -//! process can see the service, and the same four answers are reachable from -//! one binary. That is the pattern every hostile-server test uses from here. +//! process can see the service. That is the pattern every hostile-server test +//! uses from here. //! //! Roles: no argument is the server; `client` is the child that asks for a //! window and decodes what comes back. @@ -21,6 +21,7 @@ use std::os::toyos::process::CommandExt; use std::process::{Command, Stdio}; +use toyos::endow::{self, EndowError}; use toyos::port::Acceptor; use toyos::AsHandle; use toyos::{ipc, namespace, port}; @@ -29,16 +30,29 @@ use window::{CreateError, Window}; const SELF_PATH: &str = "/system/bin/test_rs_window_refusal"; -/// The reply, and the `CreateError` the client must turn it into. `None` is -/// the "not an answer to this request at all" case. -const CASES: &[(Option, CreateError)] = &[ - (Some(window::REFUSED_AT_CAPACITY), CreateError::AtCapacity), - (Some(window::REFUSED_TOO_LARGE), CreateError::TooLarge), +/// What the stand-in compositor does with one `MSG_CREATE_WINDOW`. +#[derive(Clone, Copy, Debug)] +enum Reply { + Refuse(u32), + /// A frame that answers nothing this request asked. + Unrelated, + /// No answer: the connection closes. + Close, + /// `MSG_WINDOW_CREATED` with no buffer sent ahead of it. + NoBuffer, +} + +/// The reply, and the `CreateError` the client must turn it into. +const CASES: &[(Reply, CreateError)] = &[ + (Reply::Refuse(window::REFUSED_AT_CAPACITY), CreateError::AtCapacity), + (Reply::Refuse(window::REFUSED_TOO_LARGE), CreateError::TooLarge), // A reason from a newer compositor than this client. It must arrive as a // refusal carrying the raw value, not as a protocol error and not as a // window. - (Some(4242), CreateError::Refused(4242)), - (None, CreateError::Protocol(window::MSG_FRAME)), + (Reply::Refuse(4242), CreateError::Refused(4242)), + (Reply::Unrelated, CreateError::Protocol(window::MSG_FRAME)), + (Reply::Close, CreateError::BrokenOff), + (Reply::NoBuffer, CreateError::NoHandle), ]; fn main() { @@ -77,7 +91,7 @@ fn server() { /// Answer one `MSG_CREATE_WINDOW`, then drop the connection — which is what the /// compositor does after a refusal, and the reason the reply has to still be /// readable once the writer is gone. -fn serve_one(acceptor: &Acceptor, reply: Option) { +fn serve_one(acceptor: &Acceptor, reply: Reply) { let accepted = acceptor.accept().expect("accept a client"); let handle = accepted.as_handle(); let header = ipc::recv_header(handle).expect("request header"); @@ -85,13 +99,19 @@ fn serve_one(acceptor: &Acceptor, reply: Option) { let _req: window::CreateWindowRequest = ipc::recv_payload(handle, &header).expect("request payload"); match reply { - Some(reason) => { + Reply::Refuse(reason) => { ipc::send(handle, window::MSG_WINDOW_REFUSED, &window::WindowRefused { reason }) .expect("send the refusal"); } - None => { + Reply::Unrelated => { ipc::signal(handle, window::MSG_FRAME).expect("send a reply that answers nothing"); } + Reply::Close => {} + Reply::NoBuffer => { + let info = window::WindowInfo { width: 100, height: 100, stride: 100, pixel_format: 0 }; + ipc::send(handle, window::MSG_WINDOW_CREATED, &info) + .expect("send a window with no buffer"); + } } } @@ -107,4 +127,17 @@ fn client() { // compositor drops the connection the moment it has answered. assert!(!got.to_string().is_empty(), "{got:?} has no message"); } + + // A port whose queue is full, which the stand-in never drains: the kernel + // refuses the connection itself. + let mut queued = Vec::new(); + let refused = loop { + match endow::service("compositor") { + Ok(conn) => queued.push(conn), + Err(EndowError::Refused(e)) => break e, + Err(other) => panic!("filling the queue ended in {other:?}"), + } + }; + let got = Window::create(100, 100).err(); + assert_eq!(got, Some(CreateError::Kernel(refused)), "a full queue decoded wrongly"); } diff --git a/userland/compositor/src/client.rs b/userland/compositor/src/client.rs index e4b8c8591e5..b2504f30f7b 100644 --- a/userland/compositor/src/client.rs +++ b/userland/compositor/src/client.rs @@ -249,10 +249,6 @@ pub fn deliver(dead: &mut Vec, win: &Win, msg_type: u3 } /// [`deliver`] for a message whose payload names buffers that travel with it. -/// -/// The handles are moved whether or not the frame lands, so the caller has -/// already given them up — and a client dropped here drops the queue holding -/// them, which is what releases the region. pub fn deliver_with_handles( dead: &mut Vec, win: &Win, diff --git a/userland/compositor/src/session.rs b/userland/compositor/src/session.rs index 5d0d6e6886c..bc64d560d9d 100644 --- a/userland/compositor/src/session.rs +++ b/userland/compositor/src/session.rs @@ -990,9 +990,9 @@ impl Session { }; // Moved on its own, so that a refused move leaves `theirs` here to // close: once moved, its number is no longer this process's to close. - if let Err(e) = syscall::handle_send(conn.as_handle(), &[theirs]) { + if syscall::handle_send(conn.as_handle(), &[theirs]).is_err() { syscall::close(theirs); - mark_dead(&mut self.dead, handle, ipc::TrySendError::Syscall(e).into()); + mark_dead(&mut self.dead, handle, DropReason::Gone); return; } if let Err(e) = conn.try_signal(window::MSG_COPY_REGION) { diff --git a/userland/toyos-window/src/lib.rs b/userland/toyos-window/src/lib.rs index 465c4f5e52f..393343c75d3 100644 --- a/userland/toyos-window/src/lib.rs +++ b/userland/toyos-window/src/lib.rs @@ -124,16 +124,18 @@ pub enum CreateError { NotEndowed, /// The compositor exited: its port is closed for good. CompositorGone, - /// The kernel refused the connection — a full queue, a full table. - ConnectRefused(SyscallError), + /// The kernel refused a call of the exchange: the connection, a frame, or + /// the map of the region the compositor sent. + Kernel(SyscallError), /// The exchange broke off: the compositor closed the connection, having - /// exited or refused (its log says which), or sent a frame shorter than - /// its type. + /// exited or refused (its log says which), or sent a frame this client + /// cannot read. BrokenOff, /// The compositor answered, and not with anything this exchange allows. Protocol(u32), - /// The region the compositor sent would not map here. - Unmappable(SyscallError), + /// The answer came without the one handle it carries: the compositor sent + /// none, or this process had no room to take it. + NoHandle, /// The compositor is already holding as many windows as it can afford. AtCapacity, /// The requested size is bigger than the screen it would be drawn on. @@ -149,17 +151,17 @@ impl std::fmt::Display for CreateError { match self { Self::NotEndowed => write!(f, "this program was given no compositor"), Self::CompositorGone => write!(f, "the compositor is gone"), - Self::ConnectRefused(e) => { - write!(f, "the kernel refused a connection to the compositor ({e:?})") + Self::Kernel(e) => { + write!(f, "the kernel refused the exchange with the compositor ({e:?})") } Self::BrokenOff => write!(f, "the compositor broke off the exchange"), - Self::Unmappable(e) => write!(f, "the compositor's region would not map ({e:?})"), + Self::NoHandle => write!(f, "the compositor's answer arrived without its handle"), Self::AtCapacity => write!(f, "the compositor is at its window limit"), Self::TooLarge => write!(f, "the window is larger than the screen"), Self::NoMemory => write!(f, "there is no memory for a window that size"), Self::Refused(reason) => write!(f, "the compositor refused (reason {reason})"), Self::Protocol(msg_type) => { - write!(f, "the compositor answered with message type {msg_type}") + write!(f, "the compositor's answer (type {msg_type}) is not one this exchange allows") } } } @@ -172,17 +174,25 @@ impl From for CreateError { match e { EndowError::NotEndowed => Self::NotEndowed, EndowError::ServerGone => Self::CompositorGone, - EndowError::Refused(e) => Self::ConnectRefused(e), + EndowError::Refused(e) => Self::Kernel(e), } } } impl From for CreateError { - fn from(_: ipc::IpcError) -> Self { - Self::BrokenOff + fn from(e: ipc::IpcError) -> Self { + match e { + ipc::IpcError::Disconnected | ipc::IpcError::Malformed => Self::BrokenOff, + ipc::IpcError::Syscall(e) => Self::Kernel(e), + ipc::IpcError::TooLarge => { + unreachable!("every frame this crate sends is within ipc::MAX_FRAME_LEN") + } + } } } +const _: () = assert!(MAX_INLINE_PAYLOAD <= ipc::MAX_FRAME_LEN as usize); + impl CreateError { fn from_wire(reason: u32) -> Self { match reason { @@ -450,9 +460,8 @@ fn copy(bytes: &[u8]) -> Result<(), CreateError> { if header.msg_type != MSG_COPY_REGION || header.len() != 0 { return Err(CreateError::Protocol(header.msg_type)); } - let [region] = - conn.recv_handles_exact::<1>().ok_or(CreateError::Protocol(MSG_COPY_REGION))?; - let mut region = SharedMemory::adopt(region, bytes.len()).map_err(CreateError::Unmappable)?; + let [region] = conn.recv_handles_exact::<1>().ok_or(CreateError::NoHandle)?; + let mut region = SharedMemory::adopt(region, bytes.len()).map_err(CreateError::Kernel)?; region.as_mut_slice().copy_from_slice(bytes); Ok(conn.signal(MSG_COPY_COMMIT)?) } @@ -529,9 +538,8 @@ impl Window { // The buffer crossed ahead of the frame. A compositor that announced a // window and sent nothing with it is not serving this client, whatever // else it is doing. - let [buffer] = - conn.recv_handles_exact::<1>().ok_or(CreateError::Protocol(MSG_WINDOW_CREATED))?; - let shm = SharedMemory::adopt(buffer, buf_size).map_err(CreateError::Unmappable)?; + let [buffer] = conn.recv_handles_exact::<1>().ok_or(CreateError::NoHandle)?; + let shm = SharedMemory::adopt(buffer, buf_size).map_err(CreateError::Kernel)?; let poller = Poller::new(1); Ok(Self { From 2a5c94c4c6ed4b30cb2c65f4966f5701a829aea1 Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 07:38:29 +0200 Subject: [PATCH 4/9] Guest tests: no wait carries a clock; each blocks on its event and names it compositor_hostile_clipboard loses its 30 s ceiling, its 2 s paste re-mark and all three Instant deadlines. A hang-up is a blocking read that answers 0, the region and the probe are blocking header reads, a paste and a window's close are the window's blocking recv_event. The paste marker is printed once, so each GUI+V the host types is one paste and no stale-paste skip is needed. compositor_client_death's probe loses its 500 x 10 ms sleep-poll, and the window-closed-from-the-inside case its eight 2 s polls: it waits for Close with no timeout, then asks once more, which a latched window answers None at once and an unlatched one answers Close at once. Every wait prints what it waits for first, so an event that never comes is the harness's ceiling with the missing event on the guest's last line. window_refusal had no clock; both its sides now name their waits too. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- .../src/bin/compositor_client_death.rs | 103 +++++++---------- .../src/bin/compositor_hostile_clipboard.rs | 109 ++++++------------ .../src/bin/window_refusal.rs | 7 ++ 3 files changed, 85 insertions(+), 134 deletions(-) diff --git a/tests/toyos-rust-tests/src/bin/compositor_client_death.rs b/tests/toyos-rust-tests/src/bin/compositor_client_death.rs index 206b659d98f..16b0c0bc6ac 100644 --- a/tests/toyos-rust-tests/src/bin/compositor_client_death.rs +++ b/tests/toyos-rust-tests/src/bin/compositor_client_death.rs @@ -20,9 +20,13 @@ //! each one used to take a process with it. //! //! Each case leaves its damage standing and then asks the compositor a -//! question **with a deadline**, exactly as `compositor_stall` does — the host -//! asserts the other half, that the desktop is still painting and that every -//! client dropped on the way was named with its pid. +//! question it answers from its dispatch — the host asserts the other half, +//! that the desktop is still painting and that every client dropped on the way +//! was named with its pid. +//! +//! No wait here has a clock: each blocks on its event, so one that never comes +//! is the harness's ceiling. Before each wait on the compositor this process +//! prints what it waits for, the line such an event leaves last. use std::io::{BufRead, BufReader}; use std::os::toyos::process::CommandExt; @@ -31,7 +35,7 @@ use std::process::{exit, Command, Stdio}; use toyos::endow; use toyos::AsHandle; use toyos::{ipc, Connection}; -use toyos_abi::syscall::{self, SyscallError}; +use toyos_abi::syscall; use toyos_abi::RawHandle; use window::Window; @@ -44,21 +48,8 @@ const RELAY_SOCKET: RawHandle = RawHandle(3); /// reaped. Nothing is ever read off it but the hang-up. const RELAY_GO: RawHandle = RawHandle(4); -/// `MSG_GET_RESOLUTION` is answered from the compositor's dispatch, so a reply -/// proves the event loop reached the end of a pass rather than merely that the -/// process still exists. -const PROBE_POLLS: u32 = 500; -const PROBE_POLL_NS: u64 = 10_000_000; - -/// How many events a closed window is asked for. -/// -/// Two would do — `Close`, then `None` — and this is a handful more so the -/// failure prints a stream rather than a single wrong answer. Each poll past -/// the close costs nothing: the handle is ready, so none of them waits. -const POLLS_AFTER_CLOSE: usize = 8; -/// Long enough that a compositor still on its way to closing the connection is -/// waited for rather than raced. -const POLL_TIMEOUT_NS: u64 = 2_000_000_000; +/// `timeout_nanos` for a wait with no clock (`syscall::inbox_submit`). +const FOREVER: u64 = u64::MAX; fn main() { match std::env::args().nth(1).as_deref() { @@ -105,6 +96,7 @@ fn run() { // A window is a connection promoted by its first frame, so a second // `MSG_CREATE_WINDOW` on one arrives with nothing to promote. The // compositor read that as its own bug. + waiting("a second create on a live window", "its window"); let doubled = Window::create(64, 64).expect("a window to send a second create on"); write_handle(doubled.handle(), &create_frame(), "a second create"); probe("a second create on a live window"); @@ -139,41 +131,32 @@ fn run() { // `MSG_DESTROY_WINDOW` makes the compositor drop the connection, after // which the handle is permanently read-ready at EOF — so a `poll_event` that // did not latch answered `Close` for as long as anybody kept asking, and a - // client draining until `None` never got out. Two calls decide it. + // client draining until `None` never got out. Two calls decide it, and + // the second waits for nothing: a latched window answers `None` at once, + // and an unlatched one reads the end of the stream at once. + let what = "a window closed from the inside"; + waiting(what, "its window"); let mut ending = Window::create(64, 64).expect("a window to close from the inside"); ipc::signal(ending.handle(), window::MSG_DESTROY_WINDOW) .expect("ask the compositor to destroy this window"); - // Named rather than kept, because `Event` is not `Debug` and a failure - // here has to print the whole sequence it saw. - let mut seen: Vec<&'static str> = Vec::new(); - for _ in 0..POLLS_AFTER_CLOSE { - let name = match ending.poll_event(POLL_TIMEOUT_NS) { - None => "none", - Some(window::Event::Close) => "close", + waiting(what, "the window's Close"); + loop { + match ending.poll_event(FOREVER) { + Some(window::Event::Close) => break, // A frame the compositor had already sent can arrive first. It is // not what this case is about, and skipping it is not a weakening: // what follows still has to be close and then nothing. - Some(_) => "other", - }; - seen.push(name); - if name == "none" { - break; + Some(_) => {} + None => fail(&format!("[{what}] the connection went and the window never said so")), } } - let sequence = seen.join(","); - let Some(closed_at) = seen.iter().position(|n| *n == "close") else { - fail(&format!( - "[a window closed from the inside] the connection went and the window never said \ - so: {sequence}" - )); - }; - if seen.get(closed_at + 1) != Some(&"none") { + if ending.poll_event(FOREVER).is_some() { fail(&format!( - "[a window closed from the inside] the poll after Close answered again: {sequence} \ - — a client that drains until None cannot leave" + "[{what}] the poll after Close answered again — a client that drains until None \ + cannot leave" )); } - probe("a window closed from the inside"); + probe(what); println!("compositor client death: 6 deaths survived, compositor still serving"); } @@ -246,32 +229,28 @@ fn write_handle(handle: toyos_abi::RawHandle, bytes: &[u8], what: &str) { } } -/// Ask the compositor something it always answers, and give it a deadline. +/// Ask the compositor something it always answers from its dispatch, so an +/// answer proves the event loop reached the end of a pass rather than merely +/// that the process still exists. fn probe(what: &str) { let conn: Connection = endow::service("compositor") .unwrap_or_else(|e| fail(&format!("[{what}] the compositor is not serving: {e:?}"))); if let Err(e) = ipc::signal(conn.as_handle(), window::MSG_GET_RESOLUTION) { fail(&format!("[{what}] could not ask the compositor for its resolution: {e:?}")); } - let mut buf = [0u8; 16]; - let mut got = 0; - for _ in 0..PROBE_POLLS { - match conn.read_nonblock(&mut buf[got..]) { - Ok(0) => fail(&format!("[{what}] the compositor closed the probe unanswered")), - Ok(n) => { - got += n; - if got == buf.len() { - return; - } - } - Err(SyscallError::WouldBlock) => syscall::nanosleep(PROBE_POLL_NS), - Err(e) => fail(&format!("[{what}] the probe could not be read: {e:?}")), - } + waiting(what, "the compositor's answer to a probe"); + let header = conn + .recv_header() + .unwrap_or_else(|e| fail(&format!("[{what}] the probe went unanswered: {e:?}"))); + if header.msg_type != window::MSG_RESOLUTION_CHANGED { + fail(&format!("[{what}] the probe was answered with message type {}", header.msg_type)); } - fail(&format!( - "[{what}] the compositor did not answer in {} ms — it is gone or its loop is parked", - PROBE_POLLS as u64 * PROBE_POLL_NS / 1_000_000, - )); +} + +/// Said before each wait on the compositor: the line a missing event leaves +/// last. +fn waiting(what: &str, awaited: &str) { + println!("compositor client death: [{what}] waiting for {awaited}"); } fn fail(msg: &str) -> ! { diff --git a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs index a5c5fb03fc1..8c819c6a9df 100644 --- a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs +++ b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs @@ -23,19 +23,21 @@ //! 8. **A commit on a window.** A commit names a region held, so a window //! sending one loses its connection. //! -//! Each case ends with a probe the compositor answers from its dispatch, under -//! a deadline. The host asserts what this side cannot see: no handle fault and -//! no compositor exit in the kernel's records, and the refusals named. +//! Each case ends with a probe the compositor answers from its dispatch. The +//! host asserts what this side cannot see: no handle fault and no compositor +//! exit in the kernel's records, and the refusals named. +//! +//! No wait here has a clock. Each blocks on its event and first prints what it +//! waits for, so an event that never comes is the harness's ceiling with its +//! name on the guest's last line. use std::sync::atomic::{AtomicU8, Ordering}; -use std::time::{Duration, Instant}; use toyos::endow; use toyos::ipc; -use toyos::poller::{Poller, READABLE}; use toyos::shm::SharedMemory; use toyos::{AsHandle, Connection}; -use toyos_abi::syscall::{self, SyscallError}; +use toyos_abi::syscall; use toyos_abi::RawHandle; use window::{Event, Window}; @@ -49,23 +51,19 @@ const COPY_REGION: u32 = 13; /// The longest copy the compositor makes a region for. const COPY_LEN: usize = 2 * 1024 * 1024; -/// The line the host answers with GUI+V. Printed again while no paste has -/// come, so an injection lost on the way costs time and not the verdict. +/// The line the host answers with GUI+V, once. const PASTE_MARKER: &str = "===HOSTILE_CLIPBOARD_PASTE==="; -const REMARK: Duration = Duration::from_secs(2); - -/// A liveness ceiling on every wait here: it costs nothing when the answer -/// comes, and bounds a compositor that is gone or parked. -const CEILING: Duration = Duration::from_secs(30); const BEFORE: &str = "hostile clipboard: the text before"; const AFTER: &str = "hostile clipboard: the text after"; fn main() { // First, so it has the focus: GUI+V pastes into the focused window. + waiting("the paste target", "its window"); let mut target = Window::create_with_title(160, 120, "paste") .unwrap_or_else(|e| fail("the paste target", &format!("no window: {e}"))); target.present(); + waiting("the clipboard to start from", "the compositor taking it"); window::clipboard_set(BEFORE) .unwrap_or_else(|e| fail("the clipboard to start from", &e.to_string())); probe("the clipboard to start from"); @@ -76,7 +74,7 @@ fn main() { let what = "a copy that is not UTF-8"; commit_filled(what, 0xFF); probe(what); - let first = paste(&mut target, what, None); + let first = paste(&mut target, what); if first != BEFORE.as_bytes() { let len = first.len(); fail(what, &format!("the paste was {len} bytes, not the clipboard from before")); @@ -85,10 +83,10 @@ fn main() { let what = "a region rewritten after its commit"; let region = commit_filled(what, b'C'); // Text too, so a read of the region at the paste is pasted rather than - // refused, and differs from the stale paste `paste` skips. + // refused. fill(®ion, b'D'); probe(what); - let second = paste(&mut target, what, Some(&first)); + let second = paste(&mut target, what); if second.len() != COPY_LEN || second.iter().any(|&b| b != b'C') { fail(what, &describe(&second, b'C')); } @@ -122,7 +120,7 @@ fn main() { .unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); await_hangup(conn.as_handle(), what, "the compositor's refusal"); probe(what); - let third = paste(&mut target, what, Some(&second)); + let third = paste(&mut target, what); if third != AFTER.as_bytes() { let len = third.len(); fail(what, &format!("the paste was {len} bytes, not the clipboard from before")); @@ -130,20 +128,13 @@ fn main() { // Last: the new window takes the focus the pastes went to. let what = "a commit on a window"; + waiting(what, "its window"); let mut committing = Window::create_with_title(64, 64, "commit") .unwrap_or_else(|e| fail(what, &format!("no window: {e}"))); ipc::signal(committing.handle(), COPY_COMMIT) .unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); - let deadline = Instant::now() + CEILING; - loop { - let left = deadline.saturating_duration_since(Instant::now()); - if left.is_zero() { - fail(what, &format!("the compositor kept the window {} s", CEILING.as_secs())); - } - if let Some(Event::Close) = committing.poll_event(left.as_nanos() as u64) { - break; - } - } + waiting(what, "the compositor closing the window"); + while !matches!(committing.recv_event(), Event::Close) {} probe(what); println!("hostile clipboard: every case survived, compositor still serving"); @@ -191,7 +182,7 @@ fn begin_copy(what: &str) -> (Connection, SharedMemory) { let conn = connect(what); conn.send(COPY_BEGIN, &window::ClipboardShmMsg { len: COPY_LEN as u32 }) .unwrap_or_else(|e| fail(what, &format!("could not begin: {e:?}"))); - await_readable(conn.as_handle(), what, "the compositor's region"); + waiting(what, "the compositor's region"); let header = conn.recv_header().unwrap_or_else(|e| fail(what, &format!("no answer: {e:?}"))); if header.msg_type != COPY_REGION || header.len() != 0 { fail( @@ -223,24 +214,14 @@ fn bytes(region: &SharedMemory) -> &[AtomicU8] { unsafe { std::slice::from_raw_parts(region.as_ptr() as *const AtomicU8, region.len()) } } -/// Ask the host for GUI+V and return what the target is pasted, skipping any -/// paste equal to `stale` — an earlier marker's second injection. -fn paste(target: &mut Window, what: &str, stale: Option<&[u8]>) -> Vec { - let deadline = Instant::now() + CEILING; - let mut mark = Instant::now(); +/// Ask the host for GUI+V and return what the target is pasted. +fn paste(target: &mut Window, what: &str) -> Vec { + waiting(what, "the paste"); + println!("{PASTE_MARKER}"); loop { - let now = Instant::now(); - if now >= deadline { - fail(what, &format!("no paste in {} s of asking", CEILING.as_secs())); - } - if now >= mark { - println!("{PASTE_MARKER}"); - mark = now + REMARK; - } - let wait = mark.min(deadline).saturating_duration_since(now); - match target.poll_event(wait.as_nanos() as u64) { - Some(Event::ClipboardPaste(text)) if Some(text.as_slice()) != stale => return text, - Some(Event::Close) => fail(what, "the paste target's window was closed"), + match target.recv_event() { + Event::ClipboardPaste(text) => return text, + Event::Close => fail(what, "the paste target's window was closed"), _ => {} } } @@ -262,44 +243,28 @@ fn connect(what: &str) -> Connection { .unwrap_or_else(|e| fail(what, &format!("the compositor is not serving: {e:?}"))) } -/// Wait until `handle` is readable, or fail at the ceiling. -fn await_readable(handle: RawHandle, what: &str, awaited: &str) { - let poller = Poller::new(1); - poller.watch_raw(handle, READABLE, 0); - let mut ready = false; - poller.wait(1, CEILING.as_nanos() as u64, |_| ready = true); - if !ready { - fail(what, &format!("{awaited} did not come in {} s", CEILING.as_secs())); - } +/// Said before every blocking wait: the line a missing event leaves last. +fn waiting(what: &str, awaited: &str) { + println!("hostile clipboard: [{what}] waiting for {awaited}"); } /// Wait for the peer of `handle` to hang up, failing on anything it sends. fn await_hangup(handle: RawHandle, what: &str, awaited: &str) { - let deadline = Instant::now() + CEILING; - loop { - let mut byte = [0u8; 1]; - match syscall::read_nonblock(handle, &mut byte) { - Ok(0) => return, - Ok(_) => fail(what, &format!("the peer answered where {awaited} was due")), - Err(SyscallError::WouldBlock) => {} - Err(e) => fail(what, &format!("waiting for {awaited}: {e:?}")), - } - let left = deadline.saturating_duration_since(Instant::now()); - if left.is_zero() { - fail(what, &format!("{awaited} did not come in {} s", CEILING.as_secs())); - } - let poller = Poller::new(1); - poller.watch_raw(handle, READABLE, 0); - poller.wait(1, left.as_nanos() as u64, |_| {}); + waiting(what, awaited); + let mut byte = [0u8; 1]; + match syscall::read(handle, &mut byte) { + Ok(0) => {} + Ok(_) => fail(what, &format!("the peer answered where {awaited} was due")), + Err(e) => fail(what, &format!("waiting for {awaited}: {e:?}")), } } -/// Ask the compositor something it always answers, under the ceiling. +/// Ask the compositor something it always answers. fn probe(what: &str) { let conn = connect(what); conn.signal(window::MSG_GET_RESOLUTION) .unwrap_or_else(|e| fail(what, &format!("could not ask for the resolution: {e:?}"))); - await_readable(conn.as_handle(), what, "the compositor's answer to a probe"); + waiting(what, "the compositor's answer to a probe"); let header = conn .recv_header() .unwrap_or_else(|e| fail(what, &format!("the probe went unanswered: {e:?}"))); diff --git a/tests/toyos-rust-tests/src/bin/window_refusal.rs b/tests/toyos-rust-tests/src/bin/window_refusal.rs index 1a495a29bb7..33ecf5e55ba 100644 --- a/tests/toyos-rust-tests/src/bin/window_refusal.rs +++ b/tests/toyos-rust-tests/src/bin/window_refusal.rs @@ -17,6 +17,10 @@ //! //! Roles: no argument is the server; `client` is the child that asks for a //! window and decodes what comes back. +//! +//! No wait here has a clock: each side prints what it waits for and blocks on +//! it, so an answer that never comes is the harness's ceiling with its name on +//! the last line. use std::os::toyos::process::CommandExt; use std::process::{Command, Stdio}; @@ -80,9 +84,11 @@ fn server() { .expect("window_refusal: spawn the client"); for (reply, _) in CASES { + println!("window refusal: [{reply:?}] waiting for the client's request"); serve_one(&acceptor, *reply); } + println!("window refusal: waiting for the client to exit"); let status = child.wait().expect("window_refusal: reap the client"); assert_eq!(status.code(), Some(0), "the client did not survive the refusals"); println!("{} refusal outcomes decoded, none panicked the client", CASES.len()); @@ -117,6 +123,7 @@ fn serve_one(acceptor: &Acceptor, reply: Reply) { fn client() { for (reply, expected) in CASES { + println!("window refusal: [{reply:?}] waiting for the answer"); let outcome = Window::create(100, 100); let got = match outcome { Ok(_) => panic!("reply {reply:?} produced a window"), From a69ee8e68fd9b88378949e011b1a79520e2ec2fd Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 07:40:20 +0200 Subject: [PATCH 5/9] window_refusal: the client makes every request before it judges any The stand-in serves one request per case. A client that panicked at a wrong answer left the stand-in blocked in accept on the next case, so a decoding red was a harness ceiling instead of the client's assertion. Now every answer is collected first and judged after, so a wrong one reds as the client's panic and the server's reap. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- tests/toyos-rust-tests/src/bin/window_refusal.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/toyos-rust-tests/src/bin/window_refusal.rs b/tests/toyos-rust-tests/src/bin/window_refusal.rs index 33ecf5e55ba..59695620142 100644 --- a/tests/toyos-rust-tests/src/bin/window_refusal.rs +++ b/tests/toyos-rust-tests/src/bin/window_refusal.rs @@ -122,9 +122,14 @@ fn serve_one(acceptor: &Acceptor, reply: Reply) { } fn client() { - for (reply, expected) in CASES { + // Every request is made before any is judged: the stand-in serves one per + // case, and a client that stopped early would leave it waiting on the next. + let mut outcomes = Vec::new(); + for (reply, _) in CASES { println!("window refusal: [{reply:?}] waiting for the answer"); - let outcome = Window::create(100, 100); + outcomes.push(Window::create(100, 100)); + } + for ((reply, expected), outcome) in CASES.iter().zip(outcomes) { let got = match outcome { Ok(_) => panic!("reply {reply:?} produced a window"), Err(e) => e, From 10265bbb326553a905facdc96778ba73812e1865 Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 10:36:22 +0200 Subject: [PATCH 6/9] Case 8 hangs up on a probe rather than waiting for Close, so a compositor that answers the commit instead of dropping the window reds instead of passing: a fresh MSG_GET_RESOLUTION on the same connection is answered only if the connection survives, and await_hangup tells that apart from a drop. File the compositor's own mapping of a committed region outliving the client's handle, with its owner and exit, and give the two prior new issues an owner line in the form the others use. Replace the atomic cast the test duplicated with SharedMemory::as_atomic, and give the second poll_event its waiting line. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- ...ffer-is-read-while-its-client-writes-it.md | 6 +++-- ...itor-ignores-a-message-it-does-not-know.md | 2 ++ ...-mapping-for-as-long-as-the-client-does.md | 24 +++++++++++++++++++ .../src/bin/compositor_client_death.rs | 5 +--- .../src/bin/compositor_hostile_clipboard.rs | 22 +++++------------ tests/toyos.rs | 5 +--- 6 files changed, 38 insertions(+), 26 deletions(-) create mode 100644 issues/isolation/the-compositor-keeps-a-committed-regions-mapping-for-as-long-as-the-client-does.md diff --git a/issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md b/issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md index 44c0178e2fa..d12454c2b12 100644 --- a/issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md +++ b/issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md @@ -10,8 +10,10 @@ Each window has one region, which the client draws into and the compositor blits from (`render::draw_window` in `userland/compositor/src/render.rs`). The compositor reads it as a `&[u8]` while the client may be writing it: a data race in Rust's model, and on the panel a frame whose pixels come from two of -the client's frames. Nothing bounds the read outside the mapping, so the -compositor is not at risk; the soundness claim and the frame are. +the client's frames. + +Owner: the compositor's window blit, `render::draw_window` in +`userland/compositor/src/render.rs`. **Exit**: the compositor makes two buffers per window, a client hands one over with its present and gets it back on release, and the compositor never reads a diff --git a/issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md b/issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md index a2653f2c6cf..4e02e63b686 100644 --- a/issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md +++ b/issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md @@ -17,5 +17,7 @@ every other unknown type is accepted and silently discarded. (`tests/toyos-rust-tests/src/bin/compositor_stall.rs`). A refusal there would drop the window after its first frame and leave the case passing with no load. +Owner: `Session::dispatch` in `userland/compositor/src/session.rs`. + **Exit**: an unknown type drops its client with `DropReason::OutOfProtocol`, and the stall's stream is a type the compositor serves without drawing. diff --git a/issues/isolation/the-compositor-keeps-a-committed-regions-mapping-for-as-long-as-the-client-does.md b/issues/isolation/the-compositor-keeps-a-committed-regions-mapping-for-as-long-as-the-client-does.md new file mode 100644 index 00000000000..b63546278f1 --- /dev/null +++ b/issues/isolation/the-compositor-keeps-a-committed-regions-mapping-for-as-long-as-the-client-does.md @@ -0,0 +1,24 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# The compositor keeps a committed region's mapping for as long as the client does + +`CopyRegion::take` (`userland/compositor/src/client.rs`) drops the +compositor's own `SharedMemory`, closing its handle to the region. That does +not unmap it: `SharedMemObject::on_zero_handles` (`kernel/src/object/shm.rs`) +tears down every process's mapping together, only once every handle to the +object is gone anywhere — `unmap_from` exists to drop one process's own +mapping on its own, but nothing outside `kernel/src/inbox/mod.rs` calls it. A +client that keeps the handle its copy answered with keeps the compositor's own +2 MiB mapping alive too, for as long as it likes, and a client that copies +repeatedly and keeps every handle leaves one such mapping per copy — bounded +only by its own handle table, never by the compositor's need for the memory. + +Owner: `kernel::object::shm`'s handle-driven mapping teardown. + +**Exit**: closing a process's last handle to a shared-memory object unmaps +that process's own view at once, through `unmap_from`, independent of whether +another process still holds a handle to the same object. diff --git a/tests/toyos-rust-tests/src/bin/compositor_client_death.rs b/tests/toyos-rust-tests/src/bin/compositor_client_death.rs index 16b0c0bc6ac..018b96be915 100644 --- a/tests/toyos-rust-tests/src/bin/compositor_client_death.rs +++ b/tests/toyos-rust-tests/src/bin/compositor_client_death.rs @@ -23,10 +23,6 @@ //! question it answers from its dispatch — the host asserts the other half, //! that the desktop is still painting and that every client dropped on the way //! was named with its pid. -//! -//! No wait here has a clock: each blocks on its event, so one that never comes -//! is the harness's ceiling. Before each wait on the compositor this process -//! prints what it waits for, the line such an event leaves last. use std::io::{BufRead, BufReader}; use std::os::toyos::process::CommandExt; @@ -150,6 +146,7 @@ fn run() { None => fail(&format!("[{what}] the connection went and the window never said so")), } } + waiting(what, "the latched poll answering None"); if ending.poll_event(FOREVER).is_some() { fail(&format!( "[{what}] the poll after Close answered again — a client that drains until None \ diff --git a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs index 8c819c6a9df..2f8f453dcba 100644 --- a/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs +++ b/tests/toyos-rust-tests/src/bin/compositor_hostile_clipboard.rs @@ -26,12 +26,8 @@ //! Each case ends with a probe the compositor answers from its dispatch. The //! host asserts what this side cannot see: no handle fault and no compositor //! exit in the kernel's records, and the refusals named. -//! -//! No wait here has a clock. Each blocks on its event and first prints what it -//! waits for, so an event that never comes is the harness's ceiling with its -//! name on the guest's last line. -use std::sync::atomic::{AtomicU8, Ordering}; +use std::sync::atomic::Ordering; use toyos::endow; use toyos::ipc; @@ -129,12 +125,13 @@ fn main() { // Last: the new window takes the focus the pastes went to. let what = "a commit on a window"; waiting(what, "its window"); - let mut committing = Window::create_with_title(64, 64, "commit") + let committing = Window::create_with_title(64, 64, "commit") .unwrap_or_else(|e| fail(what, &format!("no window: {e}"))); ipc::signal(committing.handle(), COPY_COMMIT) .unwrap_or_else(|e| fail(what, &format!("no commit: {e:?}"))); - waiting(what, "the compositor closing the window"); - while !matches!(committing.recv_event(), Event::Close) {} + ipc::signal(committing.handle(), window::MSG_GET_RESOLUTION) + .unwrap_or_else(|e| fail(what, &format!("no probe: {e:?}"))); + await_hangup(committing.handle(), what, "the compositor closing the window"); probe(what); println!("hostile clipboard: every case survived, compositor still serving"); @@ -202,18 +199,11 @@ fn begin_copy(what: &str) -> (Connection, SharedMemory) { } fn fill(region: &SharedMemory, byte: u8) { - for b in bytes(region) { + for b in region.as_atomic() { b.store(byte, Ordering::Relaxed); } } -/// The region as the only type that may alias memory another process reads. -fn bytes(region: &SharedMemory) -> &[AtomicU8] { - // SAFETY: the mapping is `region.len()` bytes and `region` outlives the - // borrow; the compositor reads it concurrently, which an atomic permits. - unsafe { std::slice::from_raw_parts(region.as_ptr() as *const AtomicU8, region.len()) } -} - /// Ask the host for GUI+V and return what the target is pasted. fn paste(target: &mut Window, what: &str) -> Vec { waiting(what, "the paste"); diff --git a/tests/toyos.rs b/tests/toyos.rs index 03961b84616..bf46f78dd39 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -710,7 +710,7 @@ const MACHINE_TESTS: &[(&str, Sched, Tier)] = &[ // slow on purpose. Its own boot too: it leaves the pointer somewhere else // and the window in a different place than it found them. ("metal_sim_window_drag", Sched::Serial, Tier::Nightly), - // A client's clipboard; no clock in any verdict. Its own + // A client's clipboard. Its own // boot: the compositor it abuses has to be one nothing else has touched. ("metal_sim_hostile_clipboard", Sched::Parallel, Tier::Fast), // A host-measured drain rate with an 8 s ceiling on a 3.3 s expectation. @@ -11314,9 +11314,6 @@ fn metal_sim_client_death(boot: &mut Boot) -> Result<(), String> { )); } - // A payload past what any client may inline is refused by name, because - // storing the prefix a frame reader keeps is the silent half of the same - // event. const OVERSIZE: &str = "compositor: refusing an inline payload past"; if !result.stdout.contains(OVERSIZE) { return Err(format!( From 2591a1d422bde6a5d25a96ee3238c0c2b9302deb Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 10:57:12 +0200 Subject: [PATCH 7/9] File a flaky buildlock test found while running this round's host gates cargo test --lib reds once on buildlock::tests::a_key_being_built_is_waited_for_and_another_key_is_not under a loaded host and passes alone straight after; unrelated to this branch's compositor work. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- ...another-key-is-not-reds-under-host-load.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md diff --git a/issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md b/issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md new file mode 100644 index 00000000000..21c65937d46 --- /dev/null +++ b/issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md @@ -0,0 +1,20 @@ +--- +status: open +kind: tooling +opened: 2026-09-28 +--- + +# `a_key_being_built_is_waited_for_and_another_key_is_not` reds under host load + +`src/buildlock.rs`'s last assertion, `keyed_idle(&root, Keyed::Sysroot, +"k1").is_some()` after the `using` guard drops, failed once under `cargo test +--lib` on a host running several other agents' builds concurrently +(`wt/toyos-desk1` at `e039fe6b`). Run alone straight after, the same test +passed in 0.59 s. The test spawns real child processes and times state +transitions against wall-clock sleeps and deadlines (`appeared`, +`keyed_idle`'s own polling), so host contention can move an event past a +window the test assumed was empty. + +**Exit**: reproduce under synthetic host load (parallel `cargo build`s pinned +to the same cores) to find which wait the contention defeats, then widen that +wait or replace the polled read with the event itself. From 70156f1bce86e1fa1937550df2736e0f9d3ee005 Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 11:11:05 +0200 Subject: [PATCH 8/9] File the bootstrap build-cache defect blocking this branch's build-only gate cargo run -- --build-only and cargo test --test toyos-build -- --list both fail after this branch's merge moved rust/'s pin, reproducing with ./x alone outside this repository's wrapper; a hand-run cargo build with the same command line and env succeeds every time, so the fault is in the stale build-dir x drives, not in the crates or this branch's sources. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- ...go-build-cant-find-its-crates-through-x.md | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md diff --git a/issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md b/issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md new file mode 100644 index 00000000000..ca07a913add --- /dev/null +++ b/issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md @@ -0,0 +1,41 @@ +--- +status: open +kind: tooling +opened: 2026-09-28 +--- + +# Bootstrap's own `cargo build`, run through `x`, can't find its crates + +`cargo run -- --build-only` and `cargo test --test toyos-build -- --list` +both panic at `src/toolchain.rs:650` ("std did not compile") on a worktree +that just moved its `rust/` pin: `./x build library --stage 0 --config +/bootstrap.toml ...` fails compiling `src/bootstrap` itself with +`error[E0463]: can't find crate for` `serde`, `clap`, `xz2`, `sha2`, `object`, +`ignore`, `clap_complete`, `cc`, `termcolor`, `build_helper` and others — +33 errors, `Compiling bootstrap v0.0.0` printed with no dependency compiled +before it, and `Build completed unsuccessfully in 0:00:00`. + +**Reproduced with `./x` alone**, bypassing `src/toolchain.rs` entirely, so +the cause is in `rust/`'s own bootstrap or its build-dir, not in this +repository's wrapper. Every one of the missing crates, at the exact version +`src/bootstrap/Cargo.lock` names, is already present under +`~/.cargo/registry/src/`. + +**The same `cargo build` command `x` logs as having failed** — copied +verbatim, including `RUSTC`, `RUSTC_BOOTSTRAP=1`, `RUSTFLAGS=-Zallow-features=` +and `CARGO_TARGET_DIR` set to the exact same +`/bootstrap` — compiles cleanly when run directly, both against +that same target directory and against a fresh one, every time it was tried +by hand. Matching `x`'s environment as closely as `ps eww` could show +(`RUSTUP_TOOLCHAIN`, `CARGO`, a fully cleared environment) did not reproduce +the failure outside `x`; only going through `x` itself does, on every one of +eight tries across roughly forty minutes of otherwise-varying host load — +including once with almost no other build running on the machine — so this +is not the host-load flakiness `buildlock`'s test suite already knows about. + +**Exit**: reproduce standalone (`./x build library --stage 0 --config +/bootstrap.toml ...` on a `local-rebuild = true` config after +`rust/`'s pin moves), and trace what `x`'s own `cargo build` invocation for +`src/bootstrap` does differently from the identical command line run by +hand — most likely a stale `.fingerprint` entry under `/bootstrap` +that a hand-run `cargo build` invalidates correctly and `x`'s does not. From 988c381bc5d9b4e188ba8959d197c60a70742d2b Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 15:48:29 +0200 Subject: [PATCH 9/9] Land round: drop the orchestrator's own-environment issue, own the flaky buildlock test, and cut a narrating comment The bootstrap build-cache failure was this worktree's environment, not the tree's, per the orchestrator's 17 successful `cargo test --test toyos-build` runs at this head; the file is deleted rather than kept as a false claim. The buildlock flake is the orchestrator's to disable, not this branch's, so it gets an owner line and loses the prescribed fix it has no standing to choose. `tests/toyos.rs`'s clipboard test comment loses the line that only restates the test's name. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- ...another-key-is-not-reds-under-host-load.md | 8 +++- ...go-build-cant-find-its-crates-through-x.md | 41 ------------------- tests/toyos.rs | 4 +- 3 files changed, 8 insertions(+), 45 deletions(-) delete mode 100644 issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md diff --git a/issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md b/issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md index 21c65937d46..4874c8968a0 100644 --- a/issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md +++ b/issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md @@ -16,5 +16,9 @@ transitions against wall-clock sleeps and deadlines (`appeared`, window the test assumed was empty. **Exit**: reproduce under synthetic host load (parallel `cargo build`s pinned -to the same cores) to find which wait the contention defeats, then widen that -wait or replace the polled read with the event itself. +to the same cores) to find which wait the contention defeats, then replace +the polled read with the event itself. + +## Owner + +Held by the orchestrator. diff --git a/issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md b/issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md deleted file mode 100644 index ca07a913add..00000000000 --- a/issues/build/bootstraps-own-cargo-build-cant-find-its-crates-through-x.md +++ /dev/null @@ -1,41 +0,0 @@ ---- -status: open -kind: tooling -opened: 2026-09-28 ---- - -# Bootstrap's own `cargo build`, run through `x`, can't find its crates - -`cargo run -- --build-only` and `cargo test --test toyos-build -- --list` -both panic at `src/toolchain.rs:650` ("std did not compile") on a worktree -that just moved its `rust/` pin: `./x build library --stage 0 --config -/bootstrap.toml ...` fails compiling `src/bootstrap` itself with -`error[E0463]: can't find crate for` `serde`, `clap`, `xz2`, `sha2`, `object`, -`ignore`, `clap_complete`, `cc`, `termcolor`, `build_helper` and others — -33 errors, `Compiling bootstrap v0.0.0` printed with no dependency compiled -before it, and `Build completed unsuccessfully in 0:00:00`. - -**Reproduced with `./x` alone**, bypassing `src/toolchain.rs` entirely, so -the cause is in `rust/`'s own bootstrap or its build-dir, not in this -repository's wrapper. Every one of the missing crates, at the exact version -`src/bootstrap/Cargo.lock` names, is already present under -`~/.cargo/registry/src/`. - -**The same `cargo build` command `x` logs as having failed** — copied -verbatim, including `RUSTC`, `RUSTC_BOOTSTRAP=1`, `RUSTFLAGS=-Zallow-features=` -and `CARGO_TARGET_DIR` set to the exact same -`/bootstrap` — compiles cleanly when run directly, both against -that same target directory and against a fresh one, every time it was tried -by hand. Matching `x`'s environment as closely as `ps eww` could show -(`RUSTUP_TOOLCHAIN`, `CARGO`, a fully cleared environment) did not reproduce -the failure outside `x`; only going through `x` itself does, on every one of -eight tries across roughly forty minutes of otherwise-varying host load — -including once with almost no other build running on the machine — so this -is not the host-load flakiness `buildlock`'s test suite already knows about. - -**Exit**: reproduce standalone (`./x build library --stage 0 --config -/bootstrap.toml ...` on a `local-rebuild = true` config after -`rust/`'s pin moves), and trace what `x`'s own `cargo build` invocation for -`src/bootstrap` does differently from the identical command line run by -hand — most likely a stale `.fingerprint` entry under `/bootstrap` -that a hand-run `cargo build` invalidates correctly and `x`'s does not. diff --git a/tests/toyos.rs b/tests/toyos.rs index 9fd8b69513b..29f1ca34a8d 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -706,8 +706,8 @@ const MACHINE_TESTS: &[(&str, Sched, Tier)] = &[ // slow on purpose. Its own boot too: it leaves the pointer somewhere else // and the window in a different place than it found them. ("metal_sim_window_drag", Sched::Serial, Tier::Nightly), - // A client's clipboard. Its own - // boot: the compositor it abuses has to be one nothing else has touched. + // Its own boot: the compositor it abuses has to be one nothing else has + // touched. ("metal_sim_hostile_clipboard", Sched::Parallel, Tier::Fast), // A host-measured drain rate with an 8 s ceiling on a 3.3 s expectation. // Not gate A, but the same instrument: what it measures is how fast a