diff --git a/compositor/Cargo.toml b/compositor/Cargo.toml index 8080daa0..9970548d 100755 --- a/compositor/Cargo.toml +++ b/compositor/Cargo.toml @@ -1,6 +1,4 @@ -# Kryptik compositor layer (docs/roadmap.md, Compositor and GUI isolation). -# A workspace apart from compartments/kryptikd, so no desktop dependency can -# reach the privileged daemon's lockfile (ADR-010). +# Apart from kryptikd's workspace, so no desktop dependency reaches its lockfile (ADR-010). [workspace] resolver = "2" members = ["zoneid", "wlproxy"] diff --git a/compositor/wlproxy/Cargo.toml b/compositor/wlproxy/Cargo.toml index d70487c8..e29d45e2 100755 --- a/compositor/wlproxy/Cargo.toml +++ b/compositor/wlproxy/Cargo.toml @@ -5,8 +5,7 @@ version.workspace = true edition.workspace = true license.workspace = true -# Dependency policy (ADR-010): this parses an untrusted zone's bytes, so libc -# only. The wire format is hand-rolled; Wayland libraries are built to be permissive. +# libc only (ADR-010): this parses a zone's bytes, and Wayland libraries are built to be permissive. [dependencies] libc = "0.2" diff --git a/compositor/wlproxy/src/main.rs b/compositor/wlproxy/src/main.rs index e4c4833c..a55e71b6 100755 --- a/compositor/wlproxy/src/main.rs +++ b/compositor/wlproxy/src/main.rs @@ -1,12 +1,8 @@ -//! kryptik-wlproxy: the per-zone Wayland filtering proxy. +//! kryptik-wlproxy: the per-zone Wayland filtering proxy, single-threaded and unprivileged. //! //! kryptik-wlproxy --zone NAME --listen PATH --upstream PATH [--max-clients N] [--once] //! -//! Each client on PATH (bound into the zone as /run/kryptik/wayland-0) gets a -//! Session to the compositor at --upstream (session.rs, policy.rs). A client -//! that breaks the protocol or reaches for something hidden gets a -//! wl_display.error and nothing more is forwarded. Single-threaded and -//! unprivileged; what a client can make it allocate is bounded. +//! Each client of PATH (the zone's /run/kryptik/wayland-0) gets its own Session to --upstream. mod policy; mod protocol; @@ -21,9 +17,7 @@ use std::time::{Duration, Instant}; use session::{Dir, Session}; -/// The log is a file in /run, which is RAM, and a client can make lines at -/// will: they are rate-limited and cut, the whole is bounded, and a failed -/// write is ignored rather than fatal. +/// Bounded log: the file is in /run, which is RAM, and a client can make lines at will. struct Log { zone: String, since: Instant, lines: u32, dropped: u32, left: usize } impl Log { const PER_SECOND: u32 = 20; @@ -118,14 +112,13 @@ const FDS_PER_SESSION: usize = 2 + 2 * policy::MAX_PENDING_FDS; /// stdio, the listener and room to spare. const FDS_RESERVED: usize = 8; -/// Clients whose descriptors fit under `limit`, and never none. +/// How many clients' descriptors fit under `limit`; at least one. fn clients_within(limit: u64, wanted: usize) -> usize { let room = usize::try_from(limit).unwrap_or(usize::MAX).saturating_sub(FDS_RESERVED) / FDS_PER_SESSION; wanted.min(room).max(1) } -/// Raise the descriptor limit to the hard one and fit max_clients under it: -/// past the limit, accept and recvmsg fail for every client of the zone. +/// Raise the fd limit to the hard one and fit max_clients under it; past it, accept and recvmsg fail. fn fit_descriptors(o: &mut Opts) { let mut rl = libc::rlimit { rlim_cur: 0, rlim_max: 0 }; if unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &mut rl) } != 0 { @@ -168,8 +161,7 @@ fn main() { std::process::exit(1); } }; - /* The zone's uid must be able to connect through the bind mount; the - * directory on this side keeps everyone else out. */ + // 0666 so the zone's uid can connect through the bind mount; the directory keeps others out. let _ = std::fs::set_permissions(&o.listen, std::os::unix::fs::PermissionsExt::from_mode(0o666)); listener.set_nonblocking(true).expect("nonblocking listener"); eprintln!("kryptik-wlproxy[{}]: listening on {} -> {}", o.zone, o.listen.display(), o.upstream.display()); @@ -178,15 +170,11 @@ fn main() { let mut next_id = 1u64; let mut served = 0u64; let mut log = Log::new(&o.zone); - /* After a failed accept the listener rests until this passes: the failed - * connection is still pending, so polling again at once would spin (and a - * client can exhaust descriptors to cause that). */ + // A failed accept leaves the connection pending, so the listener rests or poll would spin. let mut accept_after = Instant::now(); let mut fds: Vec = Vec::new(); loop { - /* The poll set snapshots `sessions`: the listener, then session i at - * 1+2i and 2+2i. `sessions` must not change while entries are read, so - * accepting comes last and the service loop stops at `polled`. */ + // fds: the listener, then session i at 1+2i and 2+2i; valid until `sessions` changes. let polled = sessions.len(); fds.clear(); let pause = accept_after.saturating_duration_since(Instant::now()); @@ -211,7 +199,6 @@ fn main() { eprintln!("kryptik-wlproxy[{}]: poll: {e}", o.zone); std::process::exit(1); } - // Service the sessions that were polled, against their own entries. let mut closed: Vec = Vec::new(); let mut zone_pool_bytes: usize = sessions.iter().map(|l| l.s.shm_pool_bytes).sum(); let mut zone_pool_count: usize = sessions.iter().map(|l| l.s.shm_pool_count).sum(); @@ -273,7 +260,7 @@ fn main() { return; } } - // Accept last, when nothing indexes the snapshot any more. + // Accept last, once nothing indexes `fds` by session. if fds[0].revents & libc::POLLIN != 0 { match listener.accept() { Ok((client, _)) => match UnixStream::connect(&o.upstream) { diff --git a/compositor/wlproxy/src/policy.rs b/compositor/wlproxy/src/policy.rs index 3d629de1..7fb8bb15 100644 --- a/compositor/wlproxy/src/policy.rs +++ b/compositor/wlproxy/src/policy.rs @@ -1,13 +1,9 @@ -//! What a zone's client may reach through the proxy, and what it rewrites. -//! -//! Globals off the allowlist are never advertised, so they cannot be bound: no -//! capture, global input, layer shell (nothing may draw over the chrome), -//! clipboard (the broker is the only cross-zone channel), virtual keyboard, -//! output management or activation; binding one anyway disconnects the client. -//! Every toplevel title and app_id comes out carrying the zone's name. +//! What a zone's client may bind, and how its titles and app_ids carry the zone's name. +//! Nothing else is advertised: no capture, global input, layer shell (nothing may draw over the +//! chrome), clipboard (the broker is the only cross-zone channel), virtual keyboard, output +//! management or activation. -/// Interfaces a zone client may bind, capped at the version the generated -/// tables know: requests of a newer version could not be parsed here. +/// Bindable interfaces, capped at the tables' versions: a newer request could not be parsed. pub const ALLOWED: &[(&str, u32)] = &[ ("wl_compositor", 6), ("wl_subcompositor", 1), @@ -26,11 +22,9 @@ pub fn allowed_version(interface: &str) -> Option { /// The most bytes a rewritten title may carry; only one line is ever shown. pub const MAX_TITLE_BYTES: usize = 256; -/// Prefix a title with `[zone] `. A claimed `[vault] ` just follows the real -/// prefix: `[work] [vault] ...`. +/// Prefix a title with `[zone] `; a claimed `[vault] ` follows the real one: `[work] [vault] ...`. pub fn title_for(zone: &str, title: &str) -> String { - /* Titles reach dwl's line-based status stream and terminal chrome: no - * control may inject records, terminal escapes or bidi overrides. */ + // Titles reach dwl's status lines and terminal chrome: controls and bidi marks become spaces. let title: String = title.chars().map(|c| { if c.is_control() || matches!(c, '\u{061c}' | '\u{200e}' | '\u{200f}' | '\u{2028}'..='\u{202e}' | '\u{2066}'..='\u{2069}') { ' ' } else { c } }).collect(); @@ -39,8 +33,7 @@ pub fn title_for(zone: &str, title: &str) -> String { out } -/// Cut `s` to at most `max` bytes, ending in `...` if anything was cut. The -/// cut lands on a character boundary: `String::truncate` panics inside one. +/// Cut `s` to `max` bytes with a `...`, on a char boundary: `String::truncate` panics inside one. fn bound_utf8(s: &mut String, max: usize) { if s.len() <= max { return; @@ -53,8 +46,7 @@ fn bound_utf8(s: &mut String, max: usize) { s.push_str("..."); } -/// `kryptik..`, the claimed part cut to a safe alphabet so a -/// zone name inside it cannot pass for the real one. +/// `kryptik..`, the claim cut to `[A-Za-z0-9_-]` so it cannot pass for another zone. pub fn app_id_for(zone: &str, claimed: &str) -> String { let cleaned: String = claimed .chars() @@ -64,10 +56,9 @@ pub fn app_id_for(zone: &str, claimed: &str) -> String { format!("kryptik.{zone}.{}", if cleaned.is_empty() { "app".to_string() } else { cleaned }) } -/// Resource bounds per client connection and across a zone's connections. +// Resource bounds per client connection and across a zone's connections. pub const MAX_OBJECTS: usize = 4096; -/// Id slots per range. libwayland reuses freed ids, so its slots never outnumber -/// its peak of live objects; a client that never reuses one stops here. +/// Id slots per range: libwayland reuses freed ids, so only a client that never does reaches this. pub const MAX_ID_SLOTS: usize = 2 * MAX_OBJECTS; pub const MAX_PENDING_BYTES: usize = 1 << 20; // per direction pub const MAX_PENDING_FDS: usize = 64; diff --git a/compositor/wlproxy/src/policy/tests.rs b/compositor/wlproxy/src/policy/tests.rs index 69c0bd20..81f04fef 100644 --- a/compositor/wlproxy/src/policy/tests.rs +++ b/compositor/wlproxy/src/policy/tests.rs @@ -22,8 +22,7 @@ fn capture_and_clipboard_are_hidden() { } } -/// Objects are forgotten only on delete_id, which names client-created ids, -/// so no event a zone can reach may create one. +/// Only delete_id frees objects, and only client-created ones: no reachable event may create one. #[test] fn no_compositor_created_objects() { use crate::protocol::{find, Arg}; @@ -76,7 +75,7 @@ fn title_controls_are_replaced() { /// The bound is in bytes; the cut must still land on a character boundary. #[test] -fn multibyte_title_cut_on_char_boundary() { +fn title_cut_on_char_boundary() { // 200 x U+00E9 is 400 bytes; byte 253 is inside a character. let t = title_for("vault", &"\u{00e9}".repeat(200)); assert!(t.len() <= MAX_TITLE_BYTES, "{}", t.len()); @@ -92,7 +91,7 @@ fn multibyte_title_cut_on_char_boundary() { assert!(t.ends_with("...")); assert!(t.trim_end_matches("...").chars().skip(4).all(|c| c == '\u{1F600}'), "{t:?}"); - // Mixed widths: an accented character exactly straddling the cut. + // Mixed widths: an accented character straddling the cut. let mut title = "x".repeat(MAX_TITLE_BYTES - 3 - "[work] ".len() - 1); title.push('\u{00e9}'); title.push_str("tail"); diff --git a/compositor/wlproxy/src/protocol.rs b/compositor/wlproxy/src/protocol.rs index 12a0091d..3fa56690 100644 --- a/compositor/wlproxy/src/protocol.rs +++ b/compositor/wlproxy/src/protocol.rs @@ -1,6 +1,4 @@ -//! Message signatures from the generated tables (protocol_tables.rs): how many -//! descriptors ride with each message and which object it creates, so the -//! object map stays in step with both peers. +//! Message signatures (protocol_tables.rs): the descriptors each carries and the object it creates. use crate::wire::{ArgReader, WireError}; @@ -24,8 +22,7 @@ pub struct Interface { pub events: &'static [Message], } -/// The interface of that name; None is a refusal in every caller. Indexed on -/// first use, since this runs for every new object and every advertised global. +/// The interface of that name, indexed on first use: this runs for every new object and global. pub fn find(name: &str) -> Option<&'static Interface> { use std::collections::HashMap; use std::sync::OnceLock; @@ -44,8 +41,7 @@ pub enum Arg { Fixed, String { nullable: bool }, Object { nullable: bool }, - /// A new object. `iface` is None for wl_registry.bind, whose new_id follows - /// the interface name and version the client chose. + /// A new object; `iface` is None for wl_registry.bind, where the client names it on the wire. NewId { iface: Option<&'static str> }, Array, Fd, @@ -57,12 +53,10 @@ impl Message { } } -/// The object a message creates and the strings it carries (for rewriting), -/// borrowed from the body: decoding allocates only for a message with strings. +/// The object a message creates and its strings, borrowed: only a message with strings allocates. #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Decoded<'a> { - /// (new object id, interface name). No message creates two (the tables are - /// tested for it), and a body that tries is refused. + /// (id, interface). No message in the tables creates two; a body that tries is refused. pub new_object: Option<(u32, &'a str)>, /// (byte offset of the string's length word within the body, value) pub strings: Vec<(usize, &'a str)>, @@ -80,8 +74,7 @@ impl<'a> Decoded<'a> { } } -/// Validate a body against its signature and collect what the proxy needs. It -/// must parse exactly to its end: a trailing byte is as suspect as a missing one. +/// Check a body against its signature to its exact end: a trailing byte is as bad as a missing one. pub fn decode<'a>(msg: &Message, body: &'a [u8]) -> Result, WireError> { let mut r = ArgReader::new(body); let mut d = Decoded::default(); diff --git a/compositor/wlproxy/src/protocol/tests.rs b/compositor/wlproxy/src/protocol/tests.rs index f244d8b1..9dc1e2d4 100644 --- a/compositor/wlproxy/src/protocol/tests.rs +++ b/compositor/wlproxy/src/protocol/tests.rs @@ -75,11 +75,7 @@ fn strings_are_located_for_rewriting() { assert_eq!(d.strings, vec![(0, "hello")]); } -// --- the decoder, attacked --------------------------------------------- -/* `Header::parse` and `decode` see every byte a client sends. A well-formed - * body for each message in the tables is damaged by a fixed-seed generator, - * so a failure repeats on every machine. The decoder must never panic or - * read past the body, and returns Ok only for an exact parse. */ +// --- Header::parse and decode see every byte a client sends: fuzz them --- /// xorshift64*: small, seeded, the same sequence everywhere. pub(crate) struct Rng(pub u64); @@ -175,7 +171,7 @@ fn decoder_survives_any_body() { } } } - // The generator must actually reach both sides of the decoder. + // The generator must reach both sides of the decoder. assert!(tried > 10_000 && accepted > tried / 50 && accepted < tried, "tried {tried}, accepted {accepted}"); } diff --git a/compositor/wlproxy/src/session.rs b/compositor/wlproxy/src/session.rs index dec7e085..8e4fbb44 100644 --- a/compositor/wlproxy/src/session.rs +++ b/compositor/wlproxy/src/session.rs @@ -1,11 +1,5 @@ -//! One proxied connection, zone client to compositor: framed (wire.rs) against -//! the tables (protocol.rs), checked, rewritten and forwarded. -//! -//! Every live object id is mapped to its interface. A message the map or the -//! tables cannot account for, a hidden bind or an exceeded bound ends the -//! session with one wl_display.error. Received descriptors queue in arrival -//! order; a message takes as many as its signature has `h` arguments, waiting -//! if they have not arrived, and sends them with its own bytes. +//! One proxied connection, zone client to compositor. A message the object map or the tables +//! cannot account for, a hidden bind or an exceeded bound ends it with one wl_display.error. use std::collections::{HashMap, VecDeque}; use std::io; @@ -81,8 +75,7 @@ impl From for SessionError { /// Bytes gathered into one outbound batch: the largest message. const BATCH_BYTES: usize = MAX_MESSAGE_LEN; -/// Descriptors in one sendmsg: libwayland reads at most 28 per message -/// (MAX_FDS_OUT) and ends the connection on more. +/// Descriptors per sendmsg: libwayland reads at most 28 (MAX_FDS_OUT) and hangs up on more. pub(crate) const BATCH_FDS: usize = 28; /// One socket: inbound bytes and descriptors, outbound batches with theirs. @@ -109,8 +102,7 @@ impl Endpoint { &self.inbuf[self.in_pos..] } - /// Move the next `n` pending bytes (one message) into the caller's reused - /// buffer; the caller has checked they are present. + /// Move the next `n` pending bytes into `out`; the caller has checked they are there. fn take(&mut self, n: usize, out: &mut Vec) { out.clear(); out.extend_from_slice(&self.inbuf[self.in_pos..self.in_pos + n]); @@ -130,8 +122,7 @@ impl Endpoint { bytes } - /// One recvmsg with room for descriptors. Returns bytes read: 0 is EOF, - /// usize::MAX that it would block. + /// One recvmsg with room for descriptors: bytes read, 0 at EOF, usize::MAX if it would block. pub fn read(&mut self) -> io::Result { let mut buf = [0u8; 4096]; let mut cmsg = [0usize; 32]; // cmsghdr needs native alignment @@ -149,7 +140,7 @@ impl Endpoint { } return Err(e); } - // Collect descriptors before the bytes, in order. + // Queue the descriptors in order before any check, so a refused read cannot leak them. unsafe { let mut c = libc::CMSG_FIRSTHDR(&msg); while !c.is_null() { @@ -164,8 +155,7 @@ impl Endpoint { c = libc::CMSG_NXTHDR(&msg, c); } } - /* Bounds hold at ingress too, even while pump waits for a missing - * descriptor: nothing may accumulate behind it. */ + // Bounds hold at ingress too, so nothing piles up while pump waits for a descriptor. if msg.msg_flags & libc::MSG_CTRUNC != 0 || self.in_fds.len() > policy::MAX_PENDING_FDS || self.pending_in().len() + n as usize > policy::MAX_PENDING_BYTES @@ -175,7 +165,7 @@ impl Endpoint { if n == 0 { return Ok(0); } - // One compaction per read, here, rather than one per message. + // Compact once per read, not once per message. if self.in_pos > 0 { self.inbuf.drain(..self.in_pos); self.in_pos = 0; @@ -217,8 +207,7 @@ impl Endpoint { return Err(e); } let n = n as usize; - /* The descriptors went with the first byte and must not go again - * with the rest: close our copies. */ + // The descriptors went with the first byte: close ours, so the rest goes without them. self.pending_fds -= fds.len(); for fd in fds.drain(..) { unsafe { libc::close(fd) }; @@ -233,9 +222,7 @@ impl Endpoint { Ok(false) } - /// Queue one message, joining the last batch while it has room. Its - /// descriptors go with that batch, so they arrive in order and no later - /// than the message, which is all libwayland asks. + /// Add to the last batch if it fits; fds ride with it, in order and never after their message. fn queue(&mut self, bytes: &[u8], fds: Vec) { self.pending_out += bytes.len(); self.pending_fds += fds.len(); @@ -270,10 +257,7 @@ impl Endpoint { /// A live object: its interface from the tables and its negotiated version. type Obj = (&'static protocol::Interface, u32); -/// Live objects by id. libwayland hands out ids densely, the client's from 1 -/// and the compositor's from SERVER_ID_BASE, and libwayland-server refuses a -/// new id past the next unused slot. So each range is a vector, and an id that -/// skips ahead is refused here as the compositor would refuse it. +/// Live objects, a vector per id range: libwayland ids are dense, and one that skips is refused. struct Objects { client: Vec>, server: Vec>, @@ -335,13 +319,14 @@ impl Objects { } } -/// The proxied connection. +/// A wl_shm pool's charge on the budgets, kept until the pool and its buffers are all deleted. struct Pool { size: usize, buffers: usize, deleted: bool, } +/// The proxied connection. pub struct Session { pub zone: String, pub client: Endpoint, @@ -354,9 +339,8 @@ pub struct Session { pub shm_pool_bytes: usize, pub shm_pool_count: usize, pub toplevels: usize, - /// Globals the server advertised and we let through: name -> (interface, version) + /// Globals let through to the client: name -> (interface, version cap). globals: HashMap, - /// How many globals were hidden from the client. pub hidden_count: usize, pub forwarded_c2s: u64, pub forwarded_s2c: u64, @@ -416,9 +400,8 @@ impl Session { if !ok || id == 0 { return Err(SessionError::IdOutOfRange { id, dir }); } - /* An interface missing from the tables cannot be tracked. Binds are - * checked against the allowlist, so only a compositor newer than the - * tables can create one: refuse rather than guess. */ + /* Refuse an interface the tables lack: binds are allowlisted, so only a + * compositor newer than the tables can name one. */ let iface = protocol::find(iface_name).ok_or_else(|| SessionError::HiddenInterface(iface_name.to_string()))?; if iface.name == "xdg_toplevel" && self.toplevels >= policy::MAX_TOPLEVELS_PER_SESSION { return Err(SessionError::ResourceLimit("too many toplevels in one session")); @@ -478,7 +461,7 @@ impl Session { let mut stamp: Option> = None; match dir { Dir::ClientToServer => { - // get_registry needs nothing here: its registry is registered below like any child. + // get_registry needs no check here; its new registry is registered below. if iface.name == "wl_registry" && h.opcode == WL_REGISTRY_BIND { let (name, version) = match (decoded.new_object, decoded.bind_version) { (Some((_, n)), Some(v)) => (n, v), @@ -524,8 +507,8 @@ impl Session { || self.shm_pool_bytes + size as usize > policy::MAX_SHM_BYTES_PER_SESSION { return Err(SessionError::ResourceLimit("wl_shm pool budget in one session")); } - // A destroyed pool may still back buffers. Its generation keeps - // those buffers separate if Wayland later reuses the object id. + /* A destroyed pool may still back buffers; the generation keeps + * them apart from a new pool on the same object id. */ self.next_pool += 1; self.shm_pool_count += 1; self.shm_pool_bytes += size as usize; @@ -553,13 +536,11 @@ impl Session { self.buffer_pools.insert(id, generation); self.pools.get_mut(&generation).unwrap().buffers += 1; } - /* Every toplevel gets the zone's app_id right behind the - * get_toplevel that creates it: the compositor draws a toplevel - * with no app_id as zone 0's own, with the trusted border. A - * later set_app_id from the client is rewritten and replaces it. */ + /* Stamp the zone's app_id right behind get_toplevel: the compositor draws + * a toplevel with no app_id as zone 0's own, with the trusted border. */ if iface.name == "xdg_surface" && m.name == "get_toplevel" { if let Some((id, _)) = decoded.new_object { - // Resolved once; tables without set_app_id refuse rather than guess an opcode. + // Looked up once, not hardcoded; tables without set_app_id refuse. static SET_APP_ID: std::sync::OnceLock> = std::sync::OnceLock::new(); let opcode = SET_APP_ID .get_or_init(|| { @@ -616,8 +597,7 @@ impl Session { } } } - /* The new object, whichever side created it. A registry binding - * chooses its version; any other child inherits its parent's. */ + // A bind picks the new object's version; any other new object inherits its parent's. if let Some((id, name)) = decoded.new_object { self.register(id, name, decoded.bind_version.unwrap_or(version), dir)?; } @@ -647,8 +627,7 @@ impl Session { } } - /// Tell the client why with a fatal wl_display.error (code 3, - /// implementation), then close both sides. + /// Tell the client why in a fatal wl_display.error (code 3, implementation); close both sides. pub fn refuse(&mut self, why: &str) { let text = format!("kryptik-wlproxy: {why}"); if let Some(m) = MessageWriter::new(WL_DISPLAY, WL_DISPLAY_ERROR).u32(WL_DISPLAY).u32(3).string(&text).finish() { diff --git a/compositor/wlproxy/src/session/tests.rs b/compositor/wlproxy/src/session/tests.rs index 77815328..88ff8c4c 100644 --- a/compositor/wlproxy/src/session/tests.rs +++ b/compositor/wlproxy/src/session/tests.rs @@ -43,6 +43,33 @@ fn get_registry(id: u32) -> Vec { fn global(reg: u32, name: u32, iface: &str, version: u32) -> Vec { MessageWriter::new(reg, WL_REGISTRY_GLOBAL).u32(name).string(iface).u32(version).finish().unwrap() } +/// A session with toplevel 7 created and the compositor's end drained. +fn with_toplevel() -> (Session, UnixStream, UnixStream) { + let (mut s, mut c, mut sv) = make(); + c.write_all(&get_registry(2)).unwrap(); + sv.write_all(&global(2, 1, "wl_compositor", 6)).unwrap(); + sv.write_all(&global(2, 2, "xdg_wm_base", 6)).unwrap(); + pump_all(&mut s).unwrap(); + c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(1).string("wl_compositor").u32(6).u32(3).finish().unwrap()).unwrap(); + c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(2).string("xdg_wm_base").u32(6).u32(4).finish().unwrap()).unwrap(); + c.write_all(&MessageWriter::new(3, 0).u32(5).finish().unwrap()).unwrap(); // create_surface -> 5 + c.write_all(&MessageWriter::new(4, 2).u32(6).u32(5).finish().unwrap()).unwrap(); // get_xdg_surface -> 6 + c.write_all(&MessageWriter::new(6, 1).u32(7).finish().unwrap()).unwrap(); // get_toplevel -> 7 + pump_all(&mut s).unwrap(); + let _ = read_all(&mut sv); + (s, c, sv) +} +/// A session with wl_shm v2 bound as object 3 and the compositor's end drained. +fn with_shm() -> (Session, UnixStream, UnixStream) { + let (mut s, mut c, mut sv) = make(); + c.write_all(&get_registry(2)).unwrap(); + sv.write_all(&global(2, 1, "wl_shm", 2)).unwrap(); + pump_all(&mut s).unwrap(); + c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(1).string("wl_shm").u32(2).u32(3).finish().unwrap()).unwrap(); + pump_all(&mut s).unwrap(); + let _ = read_all(&mut sv); + (s, c, sv) +} #[test] fn hidden_globals_invisible_and_unbindable() { @@ -129,7 +156,7 @@ fn bind_must_match_advertised_global() { } #[test] -fn new_object_cannot_replace_live_one() { +fn live_object_not_replaced() { let (mut s, mut c, mut sv) = make(); c.write_all(&get_registry(2)).unwrap(); pump_all(&mut s).unwrap(); @@ -183,7 +210,7 @@ fn id_slots_bounded() { } #[test] -fn popup_is_refused_before_it_reaches_dwl() { +fn popup_refused() { let (mut s, mut c, mut sv) = make(); s.objects.place(2, (protocol::find("xdg_surface").unwrap(), 1)); s.objects.place(3, (protocol::find("xdg_positioner").unwrap(), 1)); @@ -194,7 +221,7 @@ fn popup_is_refused_before_it_reaches_dwl() { } #[test] -fn toplevel_budget_reclaims_only_on_compositor_delete_id() { +fn toplevel_limit_and_reclaim() { let (mut s, mut c, mut sv) = make(); s.objects.place(2, (protocol::find("xdg_surface").unwrap(), 1)); for id in 3..3 + policy::MAX_TOPLEVELS_PER_SESSION as u32 { @@ -214,7 +241,7 @@ fn toplevel_budget_reclaims_only_on_compositor_delete_id() { } #[test] -fn shm_pool_creation_and_resize_have_byte_and_count_limits() { +fn shm_pool_limits() { let (mut s, mut c, mut sv) = make(); s.objects.place(3, (protocol::find("wl_shm").unwrap(), 1)); let (fd, _) = UnixStream::pair().unwrap(); @@ -256,7 +283,7 @@ fn shm_pool_creation_and_resize_have_byte_and_count_limits() { } #[test] -fn pool_budget_follows_buffers_across_reused_object_ids() { +fn pool_budget_follows_buffers() { let (mut s, mut c, mut sv) = make(); s.objects.place(3, (protocol::find("wl_shm").unwrap(), 1)); let (fd, _) = UnixStream::pair().unwrap(); @@ -309,19 +336,8 @@ fn messages_respect_bound_versions() { } #[test] -fn titles_and_app_ids_are_rewritten() { - let (mut s, mut c, mut sv) = make(); - c.write_all(&get_registry(2)).unwrap(); - sv.write_all(&global(2, 1, "wl_compositor", 6)).unwrap(); - sv.write_all(&global(2, 2, "xdg_wm_base", 6)).unwrap(); - pump_all(&mut s).unwrap(); - c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(1).string("wl_compositor").u32(6).u32(3).finish().unwrap()).unwrap(); - c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(2).string("xdg_wm_base").u32(6).u32(4).finish().unwrap()).unwrap(); - c.write_all(&MessageWriter::new(3, 0).u32(5).finish().unwrap()).unwrap(); // create_surface -> 5 - c.write_all(&MessageWriter::new(4, 2).u32(6).u32(5).finish().unwrap()).unwrap(); // get_xdg_surface -> 6 - c.write_all(&MessageWriter::new(6, 1).u32(7).finish().unwrap()).unwrap(); // get_toplevel -> 7 - pump_all(&mut s).unwrap(); - let _ = read_all(&mut sv); +fn titles_and_app_ids_rewritten() { + let (mut s, mut c, mut sv) = with_toplevel(); c.write_all(&MessageWriter::new(7, 2).string("Notes").finish().unwrap()).unwrap(); // set_title c.write_all(&MessageWriter::new(7, 3).string("editor").finish().unwrap()).unwrap(); // set_app_id pump_all(&mut s).unwrap(); @@ -350,8 +366,7 @@ fn unnamed_toplevel_is_stamped() { c.write_all(&MessageWriter::new(5, 6).finish().unwrap()).unwrap(); // wl_surface.commit, and no set_app_id ever pump_all(&mut s).unwrap(); let got = read_all(&mut sv); - let msgs = split_messages_for_test(&got); - // get_toplevel, then the proxy's set_app_id on the new object, then the commit. + let msgs = split_messages(&got); assert_eq!(msgs[0].0, (6, 1), "get_toplevel is forwarded first: {msgs:?}"); assert_eq!(msgs[1].0, (7, 3), "the proxy's set_app_id follows on the new toplevel: {msgs:?}"); assert_eq!(msgs[2].0, (5, 6), "the commit comes after the identity: {msgs:?}"); @@ -366,7 +381,7 @@ fn unnamed_toplevel_is_stamped() { } /// (object, opcode) and body of each message in a byte stream. -fn split_messages_for_test(mut bytes: &[u8]) -> Vec<((u32, u16), Vec)> { +fn split_messages(mut bytes: &[u8]) -> Vec<((u32, u16), Vec)> { let mut out = Vec::new(); while bytes.len() >= HEADER_LEN { let h = Header::parse(bytes).unwrap(); @@ -380,18 +395,7 @@ fn split_messages_for_test(mut bytes: &[u8]) -> Vec<((u32, u16), Vec)> { /// Bounded, prefixed, still valid UTF-8, and the session survives it. #[test] fn long_multibyte_title_is_forwarded() { - let (mut s, mut c, mut sv) = make(); - c.write_all(&get_registry(2)).unwrap(); - sv.write_all(&global(2, 1, "wl_compositor", 6)).unwrap(); - sv.write_all(&global(2, 2, "xdg_wm_base", 6)).unwrap(); - pump_all(&mut s).unwrap(); - c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(1).string("wl_compositor").u32(6).u32(3).finish().unwrap()).unwrap(); - c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(2).string("xdg_wm_base").u32(6).u32(4).finish().unwrap()).unwrap(); - c.write_all(&MessageWriter::new(3, 0).u32(5).finish().unwrap()).unwrap(); - c.write_all(&MessageWriter::new(4, 2).u32(6).u32(5).finish().unwrap()).unwrap(); - c.write_all(&MessageWriter::new(6, 1).u32(7).finish().unwrap()).unwrap(); - pump_all(&mut s).unwrap(); - let _ = read_all(&mut sv); + let (mut s, mut c, mut sv) = with_toplevel(); let title = "\u{00e9}".repeat(200); // 400 bytes; byte 253 is mid-character c.write_all(&MessageWriter::new(7, 2).string(&title).finish().unwrap()).unwrap(); pump_all(&mut s).unwrap(); @@ -467,13 +471,7 @@ fn batches_small_messages() { #[test] fn descriptors_ride_with_their_message() { - let (mut s, mut c, mut sv) = make(); - c.write_all(&get_registry(2)).unwrap(); - sv.write_all(&global(2, 1, "wl_shm", 2)).unwrap(); - pump_all(&mut s).unwrap(); - c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(1).string("wl_shm").u32(2).u32(3).finish().unwrap()).unwrap(); - pump_all(&mut s).unwrap(); - let _ = read_all(&mut sv); + let (mut s, mut c, sv) = with_shm(); // wl_shm.create_pool(new_id pool, fd, size): send bytes and one fd together let (probe_a, probe_b) = UnixStream::pair().unwrap(); let msg = MessageWriter::new(3, 0).u32(4).i32(4096).finish().unwrap(); @@ -491,10 +489,9 @@ fn descriptors_ride_with_their_message() { let mut buf = [0u8; 16]; let n = probe_b.read(&mut buf).unwrap(); assert_eq!(&buf[..n], b"same file"); - /* A message with no fd argument must not take one: an fd sent with - * wl_shm.release goes with the create_pool that follows. */ + // An fd sent with wl_shm.release, which takes none, goes with the next create_pool. let (extra_a, _extra_b) = UnixStream::pair().unwrap(); - send_with_fd(c.as_raw_fd(), &MessageWriter::new(3, 1).finish().unwrap(), extra_a.as_raw_fd()); // wl_shm.release (v2), carries no fd + send_with_fd(c.as_raw_fd(), &MessageWriter::new(3, 1).finish().unwrap(), extra_a.as_raw_fd()); c.write_all(&MessageWriter::new(3, 0).u32(5).i32(8192).finish().unwrap()).unwrap(); pump_all(&mut s).unwrap(); let (bytes, fds) = recv_with_fds(sv.as_raw_fd()); @@ -505,13 +502,7 @@ fn descriptors_ride_with_their_message() { #[test] fn message_waits_for_descriptor() { - let (mut s, mut c, mut sv) = make(); - c.write_all(&get_registry(2)).unwrap(); - sv.write_all(&global(2, 1, "wl_shm", 2)).unwrap(); - pump_all(&mut s).unwrap(); - c.write_all(&MessageWriter::new(2, WL_REGISTRY_BIND).u32(1).string("wl_shm").u32(2).u32(3).finish().unwrap()).unwrap(); - pump_all(&mut s).unwrap(); - let _ = read_all(&mut sv); + let (mut s, mut c, mut sv) = with_shm(); // most of the bytes first, no fd: nothing is forwarded yet let msg = MessageWriter::new(3, 0).u32(4).i32(4096).finish().unwrap(); c.write_all(&msg[..12]).unwrap(); @@ -548,7 +539,7 @@ fn rejected_message_closes_its_descriptors() { } #[test] -fn surplus_descriptors_are_bounded_at_ingress() { +fn inbound_descriptors_bounded() { let (mut s, c, _sv) = make(); let (a, mut b) = UnixStream::pair().unwrap(); b.set_nonblocking(true).unwrap(); @@ -598,7 +589,7 @@ fn outgoing_descriptors_are_bounded() { } #[test] -fn input_bounded_while_waiting_for_fd() { +fn input_bounded_awaiting_fd() { let (mut s, mut c, _sv) = make(); s.objects.place(3, (protocol::find("wl_shm").unwrap(), 2)); c.write_all(&MessageWriter::new(3, 0).u32(4).i32(4096).finish().unwrap()).unwrap(); @@ -617,7 +608,7 @@ fn input_bounded_while_waiting_for_fd() { } #[test] -fn disconnect_and_refusal_close_both_sides() { +fn refusal_closes_both_sides() { let (mut s, mut c, sv) = make(); c.write_all(&get_registry(2)).unwrap(); pump_all(&mut s).unwrap(); @@ -694,9 +685,7 @@ fn recv_with_fds(sock: RawFd) -> (Vec, Vec) { (bytes, fds) } -/// A real opening conversation, damaged by the seeded generator and sent in -/// random fragments. The session may refuse but never panic, and all it -/// forwards must be whole, well-formed messages. +/// Damaged openings sent in random fragments: no panic, and only whole messages forwarded. #[test] fn compositor_gets_only_whole_messages() { use crate::protocol::tests::Rng; @@ -713,7 +702,6 @@ fn compositor_gets_only_whole_messages() { let (mut refused, mut through) = (0u32, 0u32); for round in 0..400 { let (mut s, mut c, mut sv) = make(); - // Once get_registry has crossed, the compositor advertises the global to bind. let mut bytes = conversation.clone(); if round > 0 { for _ in 0..1 + rng.below(3) { @@ -726,6 +714,7 @@ fn compositor_gets_only_whole_messages() { } } } + // Once get_registry has crossed, the compositor advertises the global to bind. let mut advertised = false; let mut outcome = Ok(()); let mut rest: &[u8] = &bytes; @@ -750,7 +739,7 @@ fn compositor_gets_only_whole_messages() { } if round == 0 { assert!(outcome.is_ok(), "the undamaged conversation was refused: {:?}", outcome.err().map(|e| e.to_string())); - assert!(s.has_object(2) && s.has_object(3), "the undamaged conversation did not do what it says"); + assert!(s.has_object(2) && s.has_object(3), "the undamaged conversation did not bind the compositor"); } s.client.close_all(); s.server.close_all(); diff --git a/compositor/wlproxy/src/wire.rs b/compositor/wlproxy/src/wire.rs index 3e5894d6..b657c68f 100755 --- a/compositor/wlproxy/src/wire.rs +++ b/compositor/wlproxy/src/wire.rs @@ -1,18 +1,15 @@ -//! The Wayland wire format, parsed defensively: every length comes from a zone, -//! so none is used unchecked, and no input can make this panic. +//! The Wayland wire format, parsed defensively: lengths come from a zone, and no input may panic. //! -//! A message is a header (object id; `size << 16 | opcode`, `size` counting the -//! header) and arguments padded to 4 bytes, all in host byte order. A string's -//! length includes its NUL, and 0 means null. An fd takes no bytes: it travels -//! by SCM_RIGHTS, so only the tables (protocol.rs) say how many a message -//! carries, and a miscount hands a client another message's descriptor. +//! A message is a header (object id; `size << 16 | opcode`, `size` counting the header) and +//! arguments padded to 4 bytes, in host byte order. A string's length includes its NUL; 0 is null. +//! An fd takes no bytes: it travels by SCM_RIGHTS, so only the tables (protocol.rs) say how many +//! a message carries, and a miscount hands a client another message's descriptor. use std::fmt; pub const HEADER_LEN: usize = 8; -/// Largest message. libwayland never sends more than its 4096-byte buffer; the -/// 16-bit size field could claim 64 KiB for the proxy to buffer. +/// Largest message: libwayland sends at most its 4096-byte buffer, though the size field allows 64 KiB. pub const MAX_MESSAGE_LEN: usize = 4096; /// Ids from here up are the server's; a client creating one is refused. @@ -30,8 +27,7 @@ pub enum WireError { UnterminatedString, /// A string that is not valid UTF-8, which every Wayland string must be. NotUtf8, - /// A NUL inside a string: `"wl_shm\0_evil"` equals `"wl_shm"` only to a - /// C-string comparison, and policy must not depend on which kind runs. + /// A NUL inside a string: C would read `"wl_shm\0_evil"` as `"wl_shm"`. InteriorNul, } @@ -61,8 +57,7 @@ pub struct Header { } impl Header { - /// Parse a header from the first 8 bytes of `buf`. `size` is validated - /// here, so no `Header` can carry a bad length. + /// Parse the first 8 bytes of `buf`; `size` is checked here, so no `Header` has a bad length. pub fn parse(buf: &[u8]) -> Result { if buf.len() < HEADER_LEN { return Err(WireError::Truncated); diff --git a/compositor/wlproxy/src/wire/tests.rs b/compositor/wlproxy/src/wire/tests.rs index 33b6160f..13637e3e 100644 --- a/compositor/wlproxy/src/wire/tests.rs +++ b/compositor/wlproxy/src/wire/tests.rs @@ -170,7 +170,7 @@ fn writer_output_is_aligned() { let msg = MessageWriter::new(1, 0).string(s).finish().unwrap(); assert_eq!(msg.len() % 4, 0, "string {s:?} produced {} bytes", msg.len()); let mut r = ArgReader::new(&msg[HEADER_LEN..]); - assert_eq!(r.string().unwrap(), Some(s).filter(|x: &&str| !x.is_empty()).or(Some(""))); + assert_eq!(r.string().unwrap(), Some(s)); } } diff --git a/compositor/wlproxy/tests/live.rs b/compositor/wlproxy/tests/live.rs index 6bb5100a..ee0dea28 100644 --- a/compositor/wlproxy/tests/live.rs +++ b/compositor/wlproxy/tests/live.rs @@ -1,6 +1,4 @@ -//! The proxy as a process, including the event loop that session.rs's unit -//! tests cannot reach. The "upstream" is a Unix listener the test owns, -//! speaking hand-encoded wire messages, so no compositor is needed. +//! The proxy as a process, event loop included; the upstream is the test's own Unix listener. use std::io::{ErrorKind, Read, Write}; use std::os::unix::io::{AsRawFd, RawFd}; @@ -258,8 +256,7 @@ fn serves_clients_through_churn() { assert_eq!(read_exact_or_panic(&mut u1, 12, "client 1's get_registry at the upstream"), get_registry(2)); p.assert_alive("after its first client's first request"); - /* Events flow back filtered: an allowed global arrives, a hidden one does - * not, and the next allowed one arrives right after it. */ + // Events come back filtered: the hidden global between two allowed ones never arrives. u1.write_all(&global(2, 1, "wl_compositor", 6)).unwrap(); let want = global(2, 1, "wl_compositor", 6); assert_eq!(read_exact_or_panic(&mut c1, want.len(), "wl_compositor global at client 1"), want); @@ -333,9 +330,7 @@ fn send_fds(s: &UnixStream, data: &[u8], fds: &[RawFd]) { } } -/// The proxy raises its soft descriptor limit to the hard one and serves only -/// the clients whose queued descriptors fit, so one that parks as many as it -/// may cannot starve the sessions already open. +/// Only clients whose queued fds fit the raised limit are served, so none starves the open ones. #[test] fn clients_fit_descriptor_limit() { // (300 - 8) / 130: two sessions. @@ -404,8 +399,7 @@ fn once_serves_one_client() { assert_no_stale_socket(&listen); } -/// Long multi-byte titles reach the upstream bounded, prefixed and valid, and -/// the proxy survives them. +/// Long multi-byte titles reach the upstream bounded, prefixed and valid. #[test] fn long_unicode_titles_are_rewritten() { let mut p = Proxy::start("work", &[]); @@ -414,7 +408,7 @@ fn long_unicode_titles_are_rewritten() { let _ = read_exact_or_panic(&mut u, 12, "get_registry"); u.write_all(&global(2, 1, "wl_compositor", 6)).unwrap(); u.write_all(&global(2, 2, "xdg_wm_base", 6)).unwrap(); - let _ = read_until(&mut c, |b| split_messages_ok(b, 2), "both globals at the client"); + let _ = read_until(&mut c, |b| holds_messages(b, 2), "both globals at the client"); c.write_all(&bind(2, 1, "wl_compositor", 6, 3)).unwrap(); c.write_all(&bind(2, 2, "xdg_wm_base", 6, 4)).unwrap(); @@ -425,7 +419,7 @@ fn long_unicode_titles_are_rewritten() { c.write_all(&msg(4, 2, &body)).unwrap(); // xdg_wm_base.get_xdg_surface -> 6 c.write_all(&msg(6, 1, &u32ne(7))).unwrap(); // xdg_surface.get_toplevel -> 7 // Six: the proxy's app_id stamp follows get_toplevel. - let _ = read_until(&mut u, |b| split_messages_ok(b, 6), "the five setup requests and the stamped app_id at the upstream"); + let _ = read_until(&mut u, |b| holds_messages(b, 6), "the five setup requests and the stamped app_id at the upstream"); for (label, title) in [ ("accented", "\u{00e9}".repeat(200)), @@ -434,7 +428,7 @@ fn long_unicode_titles_are_rewritten() { ("ascii", "Editor".to_string()), ] { c.write_all(&msg(7, 2, &wl_string(&title))).unwrap(); // xdg_toplevel.set_title - let bytes = read_until(&mut u, |b| split_messages_ok(b, 1), &format!("the {label} title at the upstream")); + let bytes = read_until(&mut u, |b| holds_messages(b, 1), &format!("the {label} title at the upstream")); let msgs = split_messages(&bytes); assert_eq!(msgs.len(), 1); assert_eq!((msgs[0].0, msgs[0].1), (7, 2)); @@ -451,7 +445,7 @@ fn long_unicode_titles_are_rewritten() { } /// Whether `bytes` holds exactly `n` complete messages and nothing else. -fn split_messages_ok(mut bytes: &[u8], n: usize) -> bool { +fn holds_messages(mut bytes: &[u8], n: usize) -> bool { let mut count = 0; while bytes.len() >= 8 { let word = u32::from_ne_bytes([bytes[4], bytes[5], bytes[6], bytes[7]]); diff --git a/compositor/zoneid/Cargo.toml b/compositor/zoneid/Cargo.toml index 677b3678..6ec5fee0 100755 --- a/compositor/zoneid/Cargo.toml +++ b/compositor/zoneid/Cargo.toml @@ -5,8 +5,7 @@ version.workspace = true edition.workspace = true license.workspace = true -# No dependencies: the colour maths is published formulae checked against -# reference values in the tests (the ADR-010 argument, as for kryptikd). +# No dependencies (ADR-010): published colour formulae, tested against reference values. [dependencies] [[bin]] diff --git a/compositor/zoneid/src/bin/zoneid.rs b/compositor/zoneid/src/bin/zoneid.rs index 9bd0331a..4a325930 100755 --- a/compositor/zoneid/src/bin/zoneid.rs +++ b/compositor/zoneid/src/bin/zoneid.rs @@ -1,7 +1,5 @@ //! zoneid: audit zone border colours, propose palettes, simulate vision models. -//! -//! Exit codes: 0 clean or informational, 1 the zone set fails the invariant, -//! 2 usage error, 3 the zone files could not be read. +//! Exit 0 clean or informational, 1 the zone set fails, 2 usage error, 3 unreadable zone files. use std::path::PathBuf; use std::process::ExitCode; @@ -32,7 +30,7 @@ Usage: Show colours as they appear under each vision model. zoneid explain - What the invariant is and why it is shaped this way. + Explain the invariant and its channels. Zones are read from compartments/zones/*.toml by default." } @@ -62,8 +60,7 @@ fn main() -> ExitCode { } } -/// Parse `flag value` pairs, each known flag at most once. Anything else is a -/// usage error, so a misspelt `--zone X` cannot audit the default set and pass. +/// Known `flag value` pairs, each once; a misspelt `--zone X` errs instead of auditing the default. fn options<'a>(args: &'a [String], known: &[&str]) -> Result, String> { let mut out: Vec<(&str, &str)> = Vec::new(); let mut it = args.iter(); @@ -94,8 +91,7 @@ fn cmd_audit(args: &[String]) -> ExitCode { return ExitCode::from(2); } }; - /* --zones, else compartments/zones here, else the set shipped beside this - * crate, so cargo run works from anywhere in the tree. */ + // --zones, else ./compartments/zones, else the crate's own tree, so cargo run works anywhere. let dir = match flag(&args, "--zones") { Some(d) => PathBuf::from(d), None => { @@ -209,9 +205,7 @@ fn cmd_audit(args: &[String]) -> ExitCode { let crit = r.critical().count(); if r.is_fatal() { println!( - "FAIL: {crit} critical collision(s). Two border colours the compositor draws\n\ - are the same window edge to some users, so a window cannot be attributed\n\ - by looking at it." + "FAIL: {crit} critical collision(s): some users cannot tell two border colours apart." ); ExitCode::from(1) } else { @@ -296,10 +290,7 @@ fn cmd_propose(args: &[String]) -> ExitCode { println!(" {:<14} worst pair dE00 {:>6.2}", v.name(), d); } println!("\n overall floor: dE00 {:.2}", p.score); - println!( - "\nHeuristic search, not a proven optimum: it establishes a lower bound on\n\ - what is achievable under these constraints." - ); + println!("\nHeuristic search: a lower bound on what these constraints allow, not an optimum."); ExitCode::SUCCESS } @@ -360,9 +351,8 @@ const EXPLAIN: &str = "\ Zone distinctness A window's border colour is how the user tells which zone it belongs to -(docs/architecture.md). If they cannot tell at a glance which zone a password -prompt belongs to, the zones have failed them. That is a question of -perception, so it is checked against a model of it, not by comparing strings. +(docs/architecture.md). That is a question of perception, so colours are +checked against a model of it, not compared as strings. The rule: every two border colours the compositor draws (each zone's, and its own for a window from no zone, from an unknown zone, or asking for attention) @@ -375,12 +365,12 @@ Channels: label the chrome menu and its f seen when asked for pattern not drawn validated, given no weight -Focus is shown by border width, never by colour, so the window taking your -keystrokes carries exactly its zone's audited colour. +Focus is shown by border width, never by colour, so the focused window +carries its zone's audited colour. -A pass is a floor, not a guarantee: it says two identities differ under a -stated vision model by a stated metric, not that nobody in a hurry, in poor -light, on a badly calibrated screen could confuse them. +A pass is a floor, not a guarantee: two identities differ by a stated metric +under each vision model, which does not rule out confusion in poor light or +on a badly calibrated screen. "; // A binary's root finds a module beside itself; the tests sit under its name. diff --git a/compositor/zoneid/src/color.rs b/compositor/zoneid/src/color.rs index 81787de6..dd6d8845 100755 --- a/compositor/zoneid/src/color.rs +++ b/compositor/zoneid/src/color.rs @@ -1,8 +1,5 @@ -//! Colour science from published formulae, each tested against reference -//! values (CIEDE2000 against the Sharma, Wu & Dalal 2005 data). -//! -//! `Srgb` is gamma-encoded and `LinearRgb` light-linear, both in 0..=1 (not -//! 0..=255). Vision simulation and luminance work on linear values only. +//! Colour science from published formulae, each tested against reference values. +//! Components are in 0..=1, not 0..=255; vision simulation and luminance use linear RGB. use std::fmt; @@ -14,8 +11,7 @@ pub struct Srgb { pub b: f64, } -/// A light-linear RGB colour, nominally in 0..=1. Vision simulation can leave -/// that range; values are clamped only on conversion back to `Srgb`. +/// A light-linear RGB colour, nominally in 0..=1; clamped only on conversion back to `Srgb`. #[derive(Clone, Copy, PartialEq, Debug)] pub struct LinearRgb { pub r: f64, @@ -59,8 +55,7 @@ impl fmt::Display for ParseHexError { } impl Srgb { - /// Parse `#RRGGBB` only: no short form, alpha or names. Coercing a bad zone - /// colour could give a zone another zone's identity. + /// Parse `#RRGGBB` only: coercing another form could give a zone another zone's colour. pub fn from_hex(s: &str) -> Result { let bytes = s.as_bytes(); if bytes.first() != Some(&b'#') { @@ -131,8 +126,7 @@ const RGB_TO_XYZ: [[f64; 3]; 3] = [ [0.019_333_9, 0.119_192_0, 0.950_304_1], ]; -/// The reference white, derived from `RGB_TO_XYZ`: the rounded matrix misses -/// canonical D65 slightly (Y sums to 1.0000001), and deriving keeps L* = 100. +/// The reference white from `RGB_TO_XYZ`'s row sums, so white is L* = 100 despite the rounding. const WHITE: (f64, f64, f64) = ( RGB_TO_XYZ[0][0] + RGB_TO_XYZ[0][1] + RGB_TO_XYZ[0][2], RGB_TO_XYZ[1][0] + RGB_TO_XYZ[1][1] + RGB_TO_XYZ[1][2], @@ -193,8 +187,7 @@ pub fn contrast_ratio(a: Srgb, b: Srgb) -> f64 { (hi + 0.05) / (lo + 0.05) } -/// CIEDE2000 colour difference: CIE 142-2001 as corrected by Sharma, Wu & -/// Dalal (2005), with kL = kC = kH = 1. The corrections matter near blue. +/// CIEDE2000 with Sharma, Wu & Dalal's 2005 corrections (they matter near blue), kL = kC = kH = 1. pub fn ciede2000(p: Lab, q: Lab) -> f64 { const POW25_7: f64 = 6_103_515_625.0; // 25^7 diff --git a/compositor/zoneid/src/color/tests.rs b/compositor/zoneid/src/color/tests.rs index 0420a0cf..48e6c48e 100644 --- a/compositor/zoneid/src/color/tests.rs +++ b/compositor/zoneid/src/color/tests.rs @@ -73,12 +73,10 @@ fn delta_e_is_symmetric() { assert!(close(delta_e(a, b), delta_e(b, a), 1e-9)); } -/// Sharma, Wu & Dalal (2005), "The CIEDE2000 Color-Difference Formula: -/// Implementation Notes, Supplementary Test Data, and Mathematical -/// Observations", Table 1: pairs that catch the hue wraparounds and the RT -/// term near blue. +/// Sharma, Wu & Dalal (2005), "The CIEDE2000 Color-Difference Formula", Table 1: pairs that +/// catch the hue wraparounds and the RT term near blue. #[test] -fn ciede2000_against_sharma_reference_data() { +fn ciede2000_reference_data() { let cases: &[(f64, f64, f64, f64, f64, f64, f64)] = &[ // L1, a1, b1, L2, a2, b2, expect (50.0000, 2.6772, -79.7751, 50.0000, 0.0000, -82.7485, 2.0425), diff --git a/compositor/zoneid/src/cvd.rs b/compositor/zoneid/src/cvd.rs index ae9da9ed..0f86e064 100755 --- a/compositor/zoneid/src/cvd.rs +++ b/compositor/zoneid/src/cvd.rs @@ -1,10 +1,5 @@ -//! Colour-vision deficiency simulation, after Machado, Oliveira & Fernandes -//! (2009), "A Physiologically-based Model for Simulation of Color Vision -//! Deficiency", IEEE TVCG 15(6). -//! -//! Only dichromacy (severity 1.0) is modelled: the milder anomalous forms are -//! strictly less severe, so a palette that passes here passes for them too. -//! The matrices apply to linear RGB, and `simulate` does the conversion. +//! Colour-vision deficiency simulation (Machado, Oliveira & Fernandes 2009, IEEE TVCG 15(6)). +//! Only dichromacy (severity 1.0) is modelled: a palette that passes it passes the milder forms. use crate::color::{LinearRgb, Srgb}; diff --git a/compositor/zoneid/src/cvd/tests.rs b/compositor/zoneid/src/cvd/tests.rs index 7030a314..0195fabc 100644 --- a/compositor/zoneid/src/cvd/tests.rs +++ b/compositor/zoneid/src/cvd/tests.rs @@ -10,9 +10,8 @@ fn normal_vision_is_identity() { } #[test] -fn greys_unchanged_by_any_model() { - /* Each matrix's rows sum to about 1, so greys pass through; a - * transcription error in a matrix makes them drift. */ +fn greys_unchanged() { + // Each matrix's rows sum to about 1, so greys pass; a transcription error makes them drift. for hex in ["#000000", "#404040", "#808080", "#c0c0c0", "#ffffff"] { let c = Srgb::from_hex(hex).unwrap(); for v in Vision::ALL { diff --git a/compositor/zoneid/src/distinct.rs b/compositor/zoneid/src/distinct.rs index a994e80c..b39f500e 100755 --- a/compositor/zoneid/src/distinct.rs +++ b/compositor/zoneid/src/distinct.rs @@ -1,20 +1,15 @@ -//! The distinctness invariant. Every two border colours the compositor draws -//! (each zone's, and its own for unzoned, unknown and urgent windows) must -//! differ by the floor under every vision model, and zones must have distinct -//! glyphs and labels. Patterns are not drawn, so they separate nothing. +//! The distinctness invariant: every two border colours the compositor draws, its own included, +//! differ by the floor under every vision model, and zones have distinct glyphs and labels. use crate::color::{ciede2000, contrast_ratio, Lab, Srgb}; use crate::cvd::{simulate, Vision}; use crate::identity::{Channel, ZoneIdentity}; -/// The colour-difference floor, in CIEDE2000 units. About 1 is just noticeable -/// side by side in a lab, but a border is seen at the edge of vision on an -/// uncalibrated screen. `zoneid propose` reaches 15.70 for six colours under -/// the contrast rule, so 15.0 binds; the shipped colours reach 15.88. +/// The colour-difference floor in CIEDE2000 units (about 1 is just noticeable side by side), +/// set just under the 15.70 `zoneid propose` reaches for six colours under the contrast rule. pub const MIN_DELTA_E: f64 = 15.0; -/// Minimum contrast of a zone border against the background: 3:1, as WCAG 2.1 -/// SC 1.4.11 requires for user interface components. +/// Minimum border contrast against the background: 3:1, WCAG 2.1 SC 1.4.11 for UI components. pub const MIN_BORDER_CONTRAST: f64 = 3.0; /// Backgrounds a border is checked against: a colour tuned for one can vanish on the other. @@ -25,10 +20,8 @@ pub fn backgrounds() -> Vec<(&'static str, Srgb)> { BACKGROUNDS.iter().filter_map(|&(name, hex)| Some((name, Srgb::from_hex(hex).ok()?))).collect() } -/// The compositor's own border colours: unzoned windows (the chrome), zones it -/// has no colour for, and urgent windows. gen-zone-colours.py writes the same -/// values into dwl's header, and a test in zones.rs keeps the two equal. They -/// are held to the floor but not the contrast rule: the unzoned grey is light. +/// The compositor's own colours (unzoned, unknown zone, urgent), also in gen-zone-colours.py; +/// held to the floor but not the contrast rule, as the unzoned grey is light. pub const COMPOSITOR_COLOURS: [(&str, &str); 3] = [("unzoned", "#d8d8d8"), ("unknown", "#a2c9ff"), ("urgent", "#ffd000")]; @@ -118,8 +111,7 @@ impl Report { .filter(|c| c.severity == Severity::Critical) } - /// Whether the zone set should be refused. Only critical collisions refuse; - /// missing channels and low contrast are reported. + /// Whether to refuse the zone set: only critical collisions do, the rest is reported. pub fn is_fatal(&self) -> bool { self.critical().next().is_some() } diff --git a/compositor/zoneid/src/distinct/tests.rs b/compositor/zoneid/src/distinct/tests.rs index 09204466..caa63e54 100644 --- a/compositor/zoneid/src/distinct/tests.rs +++ b/compositor/zoneid/src/distinct/tests.rs @@ -37,7 +37,7 @@ fn pattern_rescues_nothing() { } #[test] -fn zone_in_a_compositor_colour() { +fn zone_in_compositor_colour() { for (name, hex) in COMPOSITOR_COLOURS { let r = analyze(&[id("x", hex)], Thresholds::default()); assert!(r.critical().any(|c| c.a == "x" && c.b == name), "{name}"); @@ -60,7 +60,7 @@ fn missing_channels() { } #[test] -fn low_contrast_reported_per_background() { +fn low_contrast_per_background() { // Near-black: invisible on the dark desktop, fine on the light one. let z = [id("ink", "#101010")]; let r = analyze(&z, Thresholds::default()); @@ -69,7 +69,7 @@ fn low_contrast_reported_per_background() { } #[test] -fn duplicate_glyphs_and_labels_reported() { +fn duplicate_glyphs_and_labels() { let a = ZoneIdentity::new("a", "#aa3333", None, Some("!"), Some("WORK")).unwrap(); let b = ZoneIdentity::new("b", "#2f6f9f", None, Some("!"), Some("w-o-r-k")).unwrap(); let r = analyze(&[a, b], Thresholds::default()); @@ -97,7 +97,7 @@ fn single_zone() { /// A known-bad palette must keep failing, worst under deuteranopia. #[test] fn known_bad_palette_fails() { - let shipped = [ + let palette = [ id("dev", "#b5651d"), id("net", "#2f6f9f"), id("personal", "#7a4fa3"), @@ -105,12 +105,8 @@ fn known_bad_palette_fails() { id("vault", "#c9a227"), id("work", "#3a7d44"), ]; - let r = analyze(&shipped, Thresholds::default()); - assert!( - r.is_fatal(), - "the original palette is expected to fail; if this now passes, the \ - metric has changed and needs looking at" - ); + let r = analyze(&palette, Thresholds::default()); + assert!(r.is_fatal(), "this palette must fail; a pass means the metric changed"); let pair = |a: &str, b: &str, v: Vision| { r.critical().any(|c| { @@ -124,7 +120,6 @@ fn known_bad_palette_fails() { ); assert!( pair("untrusted", "work", Vision::Deuteranopia), - "untrusted/work under deuteranopia is the pair that matters to the \ - threat model: sketchy links and your job, same window edge" + "untrusted/work should collide under deuteranopia too" ); } diff --git a/compositor/zoneid/src/identity.rs b/compositor/zoneid/src/identity.rs index 9718f0d6..0231d947 100755 --- a/compositor/zoneid/src/identity.rs +++ b/compositor/zoneid/src/identity.rs @@ -1,16 +1,12 @@ -//! The zone identity model: four channels and what a valid value is in each. -//! -//! Glyphs come from an allowlist, as syscalls do in the seccomp filter: a -//! denylist must foresee every bidi control, combining mark and confusable, -//! and missing one lets a zone file forge another zone's tag. Labels are -//! printable ASCII, which rules out bidi overrides and homographs at once. +//! The zone identity model: four channels and what a valid value is in each. Glyphs come from +//! an allowlist, as a denylist missing one confusable lets a zone forge another zone's tag; +//! labels are printable ASCII, which rules out bidi overrides and homographs. use std::fmt; use crate::color::{ParseHexError, Srgb}; -/// The border stroke a zone asks for. Validated to catch typos, but every -/// border is drawn solid, so zoneid gives it no weight. +/// The border stroke a zone asks for: validated, but every border is drawn solid. #[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)] pub enum Pattern { Solid, @@ -57,9 +53,8 @@ impl fmt::Display for Pattern { } } -/// Glyphs a zone may use. Each entry is in DejaVu and Liberation, has text -/// (not emoji) presentation, differs from every other entry at 12px, is no -/// mark, control, format character or space, and is unambiguous under NFKC. +/// Glyphs a zone may use: each is in DejaVu and Liberation, text (not emoji) presentation, +/// distinct from the others at 12px, no mark, control, format or space, unambiguous under NFKC. pub const GLYPH_ALLOWLIST: &[char] = &[ // Geometric shapes, U+25xx and U+26xx. '\u{25CF}', // ● BLACK CIRCLE @@ -130,21 +125,16 @@ impl fmt::Display for IdentityError { ), IdentityError::GlyphNotAllowed(c) => write!( f, - "glyph: U+{:04X} is not on the allowlist. Glyphs are allowlisted, \ - not filtered, so that font coverage and confusability are \ - reviewed rather than assumed; see GLYPH_ALLOWLIST", + "glyph: U+{:04X} is not on the allowlist; see GLYPH_ALLOWLIST", *c as u32 ), IdentityError::LabelEmpty => write!(f, "label: must not be empty"), IdentityError::LabelTooLong(n) => { write!(f, "label: at most 12 characters, got {n}") } - IdentityError::LabelNotAscii(c) => write!( - f, - "label: U+{:04X} is not printable ASCII. Labels are ASCII-only so \ - that bidi overrides cannot make one zone render as another", - *c as u32 - ), + IdentityError::LabelNotAscii(c) => { + write!(f, "label: U+{:04X} is not printable ASCII", *c as u32) + } IdentityError::LabelPadded => { write!(f, "label: must not begin or end with a space") } @@ -152,8 +142,7 @@ impl fmt::Display for IdentityError { } } -/// A zone's visual identity as configured. Missing non-colour channels stay -/// `None` rather than defaulting, because their absence is a finding. +/// A zone's identity as configured; a missing channel stays `None`, as its absence is a finding. #[derive(Clone, Debug)] pub struct ZoneIdentity { pub zone: String, @@ -195,7 +184,7 @@ impl ZoneIdentity { }) } - /// Channels this zone actually configures. + /// Channels this zone configures. pub fn present_channels(&self) -> Vec { let mut v = vec![Channel::Color]; if self.pattern.is_some() { @@ -210,8 +199,7 @@ impl ZoneIdentity { v } - /// Whether something other than colour on screen names this zone: the - /// glyph or label the chrome shows. A pattern is not drawn. + /// Whether the chrome can name this zone by glyph or label; a pattern is not drawn. pub fn has_non_color_channel(&self) -> bool { self.glyph.is_some() || self.label.is_some() } diff --git a/compositor/zoneid/src/identity/tests.rs b/compositor/zoneid/src/identity/tests.rs index 39b1949f..f6b13201 100644 --- a/compositor/zoneid/src/identity/tests.rs +++ b/compositor/zoneid/src/identity/tests.rs @@ -16,7 +16,7 @@ fn unknown_pattern_refused() { } #[test] -fn glyph_allowlist_has_no_duplicates() { +fn glyph_allowlist_unique() { let mut seen = Vec::new(); for &c in GLYPH_ALLOWLIST { assert!(!seen.contains(&c), "U+{:04X} listed twice", c as u32); @@ -25,13 +25,12 @@ fn glyph_allowlist_has_no_duplicates() { } #[test] -fn glyph_allowlist_contains_nothing_dangerous() { - // The allowlist is the whole defence, so check what is on it. +fn glyph_allowlist_safe() { + // The allowlist is the only defence, so check what is on it. for &c in GLYPH_ALLOWLIST { assert!(!c.is_control(), "U+{:04X} is a control character", c as u32); assert!(!c.is_whitespace(), "U+{:04X} is whitespace", c as u32); - /* Cf format characters (the bidi controls), and the variation - * selectors that switch a character to emoji rendering. */ + // Format characters (the bidi controls) and the selectors that switch to emoji rendering. let n = c as u32; assert!( !(0x200B..=0x200F).contains(&n), @@ -113,7 +112,7 @@ fn label_bounds() { } #[test] -fn label_key_folds_case_and_punctuation() { +fn label_key_folding() { let mk = |l: &str| { ZoneIdentity::new("z", "#aa3333", None, None, Some(l)) .unwrap() @@ -122,7 +121,7 @@ fn label_key_folds_case_and_punctuation() { }; assert_eq!(mk("WORK"), mk("work")); assert_eq!(mk("WORK"), mk("W-O-R-K")); - assert_ne!(mk("WORK"), mk("W0RK")); // zero vs O is a real difference + assert_ne!(mk("WORK"), mk("W0RK")); // zero and O stay different } #[test] diff --git a/compositor/zoneid/src/lib.rs b/compositor/zoneid/src/lib.rs index bc48c6ed..b31bf705 100755 --- a/compositor/zoneid/src/lib.rs +++ b/compositor/zoneid/src/lib.rs @@ -1,16 +1,12 @@ -//! Zone visual identity: can the user tell every two border colours the -//! compositor draws apart, under each of four vision models? -//! -//! The `zoneid` binary audits the zone files in CI and searches for palettes -//! that pass (docs/architecture.md). kryptikd only checks the shape of the -//! `[ui]` keys; neither it nor the proxy uses this crate. The check is a floor -//! on a colour-difference metric, not a guarantee against confusion. +//! Zone visual identity: can every two border colours the compositor draws be told apart +//! under each of four vision models? The `zoneid` binary audits the zone files in CI and +//! proposes palettes; kryptikd checks only the shape of the `[ui]` keys and does not use this. //! //! | Channel | Shown by | //! |---|---| //! | `color` | the whole window border, drawn by dwl | //! | `glyph`, `label` | the chrome, for the focused window | -//! | `pattern` | nothing yet: validated, given no weight | +//! | `pattern` | nothing: validated, given no weight | pub mod color; pub mod cvd; diff --git a/compositor/zoneid/src/palette.rs b/compositor/zoneid/src/palette.rs index c777bd5a..b9744b7a 100755 --- a/compositor/zoneid/src/palette.rs +++ b/compositor/zoneid/src/palette.rs @@ -1,9 +1,6 @@ -//! Palette search, which shows the floor can be met. -//! -//! A palette scores its smallest difference between any two colours under any -//! vision model (a minimum, as an average hides one colliding pair). The -//! search is farthest-point traversal, then steepest ascent from fixed -//! restarts: reproducible, and a lower bound rather than an optimum. +//! Palette search, to show the floor can be met. A palette scores its smallest difference under +//! any vision model (an average would hide one colliding pair). Farthest-point traversal, then +//! steepest ascent from fixed restarts: reproducible, and a lower bound, not an optimum. use crate::color::{contrast_ratio, ciede2000, Lab, Srgb}; use crate::cvd::{simulate, Vision}; @@ -26,16 +23,12 @@ struct Candidate { /// Search parameters, for measuring which constraint binds. #[derive(Clone, Copy, Debug)] pub struct SearchOptions { - /// Minimum contrast against both backgrounds. 1.0 drops the constraint, - /// which models a border drawn with a contrasting keyline. + /// Minimum contrast against both backgrounds; 1.0 models a border with a contrasting keyline. pub min_contrast: f64, - /// Sampling step through each sRGB axis for the coarse search. 17 gives - /// 16 levels per channel. + /// Sampling step along each sRGB axis for the coarse search; 17 gives 16 levels per channel. pub step: u32, - /// Sampling step for refining each colour around the coarse result; 0 - /// disables it. In a release build the coarse grid alone reaches 13.98 at - /// step 17 (15.42 at step 6, in 3.1 s); step 17 refined every 3 reaches - /// 15.70 in 1.8 s. + /// Refinement step around the coarse result, 0 for none. In a release build step 17 alone + /// reaches 13.98 (step 6: 15.42 in 3.1 s); refined every 3 it reaches 15.70 in 1.8 s. pub refine: u32, } @@ -53,8 +46,7 @@ impl Default for SearchOptions { const REFINE_RADIUS: i32 = 17; impl Candidate { - /// The candidate for an 8-bit sRGB triple, or `None` if it fails the - /// contrast floor against any background. + /// The candidate for an 8-bit sRGB triple; `None` if out of range or below the contrast floor. fn from_rgb8(r: i32, g: i32, b: i32, backgrounds: &[Srgb], min_contrast: f64) -> Option { if !(0..=255).contains(&r) || !(0..=255).contains(&g) || !(0..=255).contains(&b) { return None; @@ -116,8 +108,7 @@ fn distance(a: &Labs, b: &Labs) -> f64 { worst } -/// `start` lowered to `c`'s distance from each of `others`, stopping early -/// once it is at or below `floor` (the best score so far). +/// `start` lowered to `c`'s distance from each of `others`; stops once at or below `floor`. fn nearest<'a>(c: &Labs, others: impl IntoIterator, start: f64, floor: f64) -> f64 { let mut near = start; for o in others { @@ -129,8 +120,7 @@ fn nearest<'a>(c: &Labs, others: impl IntoIterator, start: f64, near } -/// A palette's score: its smallest difference, between two members or -/// between a member and a compositor colour. +/// A palette's score: the smallest difference among its members and the compositor's colours. fn score(pal: &[Labs], fixed: &[Labs]) -> f64 { let mut s = f64::INFINITY; for (i, a) in pal.iter().enumerate() { @@ -139,17 +129,15 @@ fn score(pal: &[Labs], fixed: &[Labs]) -> f64 { s } -/// The palette without member `slot`, and its score, so scoring a replacement -/// is `nearest(colour, others, rest, ..)`: one distance per member. +/// The palette without `slot`, and its score, so a replacement costs one distance per member. fn without(pal: &[Labs], slot: usize, fixed: &[Labs]) -> (Vec, f64) { let others: Vec = pal.iter().enumerate().filter(|&(i, _)| i != slot).map(|(_, l)| *l).collect(); let rest = score(&others, fixed); (others, rest) } -/// Move each colour to the best point within `REFINE_RADIUS`, sampled every -/// `fine` levels, until nothing improves. Candidates are made on demand; fixed -/// order and strict improvement keep it deterministic. +/// Move each colour to the best point within `REFINE_RADIUS`, every `fine` levels, until nothing +/// improves; fixed order and strict improvement keep it deterministic. fn refine(pal: &mut [Candidate], fixed: &[Labs], min_contrast: f64, fine: u32) { if fine == 0 { return; @@ -204,8 +192,7 @@ pub struct Proposal { pub worst_per_vision: Vec<(Vision, f64)>, } -/// Search for `n` maximally distinguishable colours. `None` if `n` is 0 or -/// fewer than `n` colours meet the contrast requirement. +/// Search for `n` distinguishable colours; `None` if `n` is 0 or too few clear the contrast floor. pub fn propose(n: usize) -> Option { propose_with(n, SearchOptions::default()) } @@ -231,9 +218,8 @@ pub fn propose_with(n: usize, opts: SearchOptions) -> Option { // Membership of `chosen` by candidate index, asked once per candidate below. let mut in_set = vec![false; cands.len()]; in_set[seed] = true; - /* Farthest-point traversal: take the candidate furthest from - * everything picked and from the compositor's colours. `near[c]` is - * that distance, kept current by one pass against the newest pick. */ + /* Farthest-point traversal: `near[c]` is c's distance to everything picked and to the + * compositor's colours, kept current by one pass against the newest pick. */ let mut near: Vec = cands.iter().zip(&to_fixed).map(|(c, &f)| f.min(distance(&c.lab, &cands[seed].lab))).collect(); while chosen.len() < n { diff --git a/compositor/zoneid/src/palette/tests.rs b/compositor/zoneid/src/palette/tests.rs index 510f7b5b..f6c47122 100644 --- a/compositor/zoneid/src/palette/tests.rs +++ b/compositor/zoneid/src/palette/tests.rs @@ -62,14 +62,13 @@ fn six_colour_palette_passes() { assert!(p.score >= MIN_DELTA_E + 0.5, "default search reached only {:.2}", p.score); } -/// The coarse grid alone does not reach the floor (13.98 at step 17); -/// refinement does. +/// The coarse grid alone stays under the floor (13.98 at step 17); refinement reaches it. #[test] fn refinement_reaches_floor() { let coarse = propose_with(6, SearchOptions { refine: 0, ..SearchOptions::default() }).unwrap(); let refined = default_six(); assert!(coarse.score < refined.score, "coarse {:.2} vs refined {:.2}", coarse.score, refined.score); - assert!(coarse.score < MIN_DELTA_E, "the coarse grid now passes on its own ({:.2}); update this comment and the doc on SearchOptions::refine", coarse.score); + assert!(coarse.score < MIN_DELTA_E, "the coarse grid passes alone ({:.2}): update the docs here and on SearchOptions::refine", coarse.score); } #[test] diff --git a/compositor/zoneid/src/toml.rs b/compositor/zoneid/src/toml.rs index 320af47e..cdf35997 100755 --- a/compositor/zoneid/src/toml.rs +++ b/compositor/zoneid/src/toml.rs @@ -1,10 +1,5 @@ -//! A small TOML reader for what zone files use, hand-rolled as ADR-010 argues -//! for kryptikd: comments, `[section]` headers, and `key = value` with a basic, -//! literal or bare value, all kept as text. -//! -//! Anything else (arrays, inline tables, dotted keys, array-of-tables, -//! multi-line strings) is an error, never skipped. Comments are stripped by the -//! scanner that tracks quotes, so `"#aa3333"` keeps its `#`. +//! The TOML subset zone files use (ADR-010): comments, `[section]` headers, and `key = value` +//! with a basic, literal or bare value, kept as text. Anything else is an error, never skipped. use std::fmt; @@ -44,20 +39,13 @@ impl fmt::Display for TomlError { TomlErrorKind::TrailingGarbage(s) => { write!(f, "unexpected text after value: {s:?}") } - TomlErrorKind::Unsupported(what) => write!( - f, - "{what} is not supported by this reader. It is refused rather \ - than skipped so that a zone file using it is never read as \ - something other than what it says" - ), + TomlErrorKind::Unsupported(what) => write!(f, "{what} is not supported"), TomlErrorKind::BadEscape(c) => write!(f, "unknown string escape \\{c}"), } } } -/// Sections in file order, each a list of key/value pairs. Read as kryptikd -/// reads it (a repeated `[section]` continues, a repeated key is an error), so -/// zoneid audits the colour kryptikd enforces. +/// Sections in file order, read as kryptikd reads them, so the audited colour is the enforced one. #[derive(Debug, Default, Clone)] pub struct Document { pub sections: Vec
, @@ -163,7 +151,6 @@ pub fn parse(input: &str) -> Result { fn strip_comment(line: &str) -> Result<&str, TomlErrorKind> { let bytes = line.as_bytes(); let mut i = 0; - // The quote character, if any, this position is inside. let mut quote: Option = None; while i < bytes.len() { @@ -215,8 +202,7 @@ fn parse_value(v: &str) -> Result { Ok(v.to_string()) } -/// Read a basic string body (after the opening quote). Returns the unescaped -/// value and how many bytes of `s` were consumed including the closing quote. +/// Unescape a basic string body after its opening quote; returns it and the bytes consumed. fn read_basic_string(s: &str) -> Result<(String, usize), TomlErrorKind> { let mut out = String::new(); let mut it = s.char_indices(); diff --git a/compositor/zoneid/src/toml/tests.rs b/compositor/zoneid/src/toml/tests.rs index 4d23887d..e0be02cc 100644 --- a/compositor/zoneid/src/toml/tests.rs +++ b/compositor/zoneid/src/toml/tests.rs @@ -1,7 +1,7 @@ use super::*; #[test] -fn hash_in_string_is_not_comment() { +fn quoted_hash_kept() { let d = parse("[ui]\nborder_color = \"#aa3333\"\n").unwrap(); assert_eq!(d.get("ui", "border_color"), Some("#aa3333")); } @@ -13,7 +13,7 @@ fn comment_after_colour() { } #[test] -fn full_line_comments_and_blank_lines() { +fn comments_and_blank_lines() { let d = parse("# a zone\n\n[zone]\nname = \"work\"\n").unwrap(); assert_eq!(d.get("zone", "name"), Some("work")); } @@ -60,7 +60,7 @@ fn unsupported_constructs_are_errors() { } #[test] -fn unterminated_string_is_an_error() { +fn unterminated_string_refused() { let e = parse("[a]\nv = \"oops\n").unwrap_err(); assert_eq!(e.kind, TomlErrorKind::UnterminatedString); } @@ -85,9 +85,9 @@ fn trailing_garbage_refused() { assert!(matches!(e.kind, TomlErrorKind::TrailingGarbage(_))); } -/// A real zone file, verbatim. +/// A whole zone file, in the shipped layout. #[test] -fn parses_real_zone_file() { +fn parses_zone_file() { // r##: the file contains `"#`. let src = r##" # untrusted - for opening things you do not trust. diff --git a/compositor/zoneid/src/zones.rs b/compositor/zoneid/src/zones.rs index 7bc44a4f..21a81c87 100644 --- a/compositor/zoneid/src/zones.rs +++ b/compositor/zoneid/src/zones.rs @@ -1,13 +1,11 @@ -//! Zone identities from a zone directory (as kryptikd installs under -//! /etc/kryptik/zones); only `[zone] name` and the `[ui]` channels are read. +//! Zone identities from a zone directory; only `[zone] name` and the `[ui]` channels are read. use std::path::{Path, PathBuf}; use crate::identity::ZoneIdentity; use crate::toml; -/// Read every `*.toml` in `dir` as a zone definition, skipping any without a -/// `[ui] border_color`: a zone with no colour is not a colour collision. +/// Load every `*.toml` in `dir`, skipping zones with no `[ui] border_color` to collide. pub fn load_zones(dir: &Path) -> Result, String> { let entries = std::fs::read_dir(dir) .map_err(|e| format!("cannot read {}: {e}", dir.display()))?; @@ -56,7 +54,7 @@ pub fn load_zones(dir: &Path) -> Result, String> { Ok(out) } -/// The zone directory this crate ships beside, for its own tests. +/// The zone directory beside this crate in the source tree. pub fn shipped_zone_dir() -> PathBuf { Path::new(env!("CARGO_MANIFEST_DIR")).join("../../compartments/zones") } diff --git a/compositor/zoneid/src/zones/tests.rs b/compositor/zoneid/src/zones/tests.rs index 3f8102ff..d0395b81 100644 --- a/compositor/zoneid/src/zones/tests.rs +++ b/compositor/zoneid/src/zones/tests.rs @@ -2,7 +2,7 @@ use super::*; use crate::distinct::{analyze, Thresholds, COMPOSITOR_COLOURS, MIN_DELTA_E}; use crate::identity::Channel; -/// dwl's colour header holds exactly the audited colours, so dwl draws none unchecked. +/// dwl's colour header holds the audited colours and no others, so dwl draws none unchecked. #[test] fn header_colours_are_audited() { let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../build/desktop/zone-colours.h"); @@ -24,8 +24,7 @@ fn header_colours_are_audited() { assert_eq!(drawn, audited); } -/// Every shipped pair clears the floor under every vision model, with no -/// other finding, and every zone carries all four channels. +/// The shipped zones pass with no finding at all, and each carries all four channels. #[test] fn shipped_zones_pass_invariant() { let zones = load_zones(&shipped_zone_dir()).expect("the shipped zone files parse"); diff --git a/tools/desktop/gen-wl-protocol.py b/tools/desktop/gen-wl-protocol.py index fd975ab5..a13a48cc 100755 --- a/tools/desktop/gen-wl-protocol.py +++ b/tools/desktop/gen-wl-protocol.py @@ -8,14 +8,13 @@ SIMPLE = {"int": "Arg::Int", "uint": "Arg::Uint", "fixed": "Arg::Fixed", "array": "Arg::Array", "fd": "Arg::Fd"} def args_of(msg): - """The message's arguments as Rust, in wire order, and how many are descriptors. - A type this does not know stops the generator.""" + """The message's arguments as Rust, in wire order, and its fd count; an unknown type raises.""" args, fds = [], 0 for a in msg.findall("arg"): t = a.get("type"); nullable = "true" if a.get("allow-null") == "true" else "false" if t == "new_id": iface = a.get("interface") - # No interface (wl_registry.bind): the id follows the interface name and version on the wire. + # No interface (wl_registry.bind): the client names it on the wire, before the id. args.append(f'Arg::NewId {{ iface: Some("{iface}") }}' if iface else "Arg::NewId { iface: None }") elif t in ("string", "object"): args.append(f"Arg::{t.capitalize()} {{ nullable: {nullable} }}")