From 837d2d2d434a4c464cb05cf5c62dd1461bccd223 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:21:16 +0100 Subject: [PATCH 01/13] chore(deps): use one random source on getrandom 0.4 Bump the crate to the version tempfile already compiles, which removes the duplicate 0.3 build. Make it required and replace the hand-written libc getrandom loop in the daemon protocol, so portal tokens and protocol identifiers share one source and one unsafe block goes away. --- Cargo.lock | 41 ++++----------------------------- Cargo.toml | 5 ++-- src/daemon/protocol_v2/linux.rs | 31 ++----------------------- 3 files changed, 9 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1c79f711..0d12739b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -294,7 +294,7 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" dependencies = [ - "getrandom 0.4.3", + "getrandom", ] [[package]] @@ -513,18 +513,6 @@ dependencies = [ "system-deps 9.0.1", ] -[[package]] -name = "getrandom" -version = "0.3.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "899def5c37c4fd7b2664648c28120ecec138e4d395b459e5ca34f9cce2dd77fd" -dependencies = [ - "cfg-if", - "libc", - "r-efi 5.3.0", - "wasip2", -] - [[package]] name = "getrandom" version = "0.4.3" @@ -534,7 +522,7 @@ dependencies = [ "cfg-if", "js-sys", "libc", - "r-efi 6.0.0", + "r-efi", "wasm-bindgen", ] @@ -1155,12 +1143,6 @@ dependencies = [ "proc-macro2", ] -[[package]] -name = "r-efi" -version = "5.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" - [[package]] name = "r-efi" version = "6.0.0" @@ -1528,7 +1510,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.4.3", + "getrandom", "once_cell", "rustix", "windows-sys 0.61.2", @@ -1724,15 +1706,6 @@ version = "0.11.1+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ccf3ec651a847eb01de73ccad15eb7d99f80485de043efb2f370cd654f4ea44b" -[[package]] -name = "wasip2" -version = "1.0.4+wasi-0.2.12" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b67efb37e106e55ce722a510d6b5f9c17f083e5fc79afc2badeb12cc313d9487" -dependencies = [ - "wit-bindgen", -] - [[package]] name = "wasm-bindgen" version = "0.2.129" @@ -1903,7 +1876,7 @@ dependencies = [ "anyhow", "cairo-rs", "flate2", - "getrandom 0.3.4", + "getrandom", "glib", "gtk4", "gtk4-layer-shell", @@ -2035,12 +2008,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "wit-bindgen" -version = "0.57.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1ebf944e87a7c253233ad6766e082e3cd714b5d03812acc24c318f549614536e" - [[package]] name = "xcursor" version = "0.3.11" diff --git a/Cargo.toml b/Cargo.toml index 5ea8103d..11a74b88 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -92,6 +92,8 @@ serde = { version = "1.0", features = ["derive"] } serde_ignored = "0.1" schemars = { version = "1.2", features = ["derive"], optional = true } libc = "0.2" +# Kernel random bytes for portal handle tokens and daemon protocol identifiers. +getrandom = { version = "0.4", features = ["std"] } # Securely created, self-deleting temporary files for the OCR engine handoff. tempfile = "3" flate2 = "1.0" @@ -105,7 +107,6 @@ tokio = { version = "1.0", features = ["rt-multi-thread", "macros", "time", "syn # Screenshot capture zbus = { version = "5.0", optional = true, default-features = false, features = ["tokio"] } -getrandom = { version = "0.3", optional = true } serde_json = "1.0" png = "0.18" @@ -125,7 +126,7 @@ xkbcommon = { version = "0.8", optional = true } tablet-input = [] # D-Bus dependent functionality (portal capture, notifications, tray) dbus = ["zbus"] -portal = ["dbus", "dep:getrandom"] +portal = ["dbus"] tray = ["dbus", "ksni"] config-schema = ["dep:schemars"] # GTK4-rendered toolbars on layer-shell compositors; the built-in Cairo diff --git a/src/daemon/protocol_v2/linux.rs b/src/daemon/protocol_v2/linux.rs index 58790f1d..b2cb536f 100644 --- a/src/daemon/protocol_v2/linux.rs +++ b/src/daemon/protocol_v2/linux.rs @@ -63,33 +63,6 @@ pub(crate) struct CommandQueueWatcher { buffered_offset: usize, } -fn fill_random(bytes: &mut [u8]) -> io::Result<()> { - let mut filled = 0; - while filled < bytes.len() { - // SAFETY: the remaining slice is writable for its full length. getrandom - // writes at most that length and does not retain the pointer. - let result = unsafe { - libc::getrandom(bytes[filled..].as_mut_ptr().cast(), bytes.len() - filled, 0) - }; - if result > 0 { - filled += result as usize; - continue; - } - if result == 0 { - return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, - "getrandom returned zero bytes", - )); - } - let error = io::Error::last_os_error(); - if error.kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(error); - } - Ok(()) -} - fn lowercase_hex(bytes: &[u8], output: &mut fmt::Formatter<'_>) -> fmt::Result { for byte in bytes { write!(output, "{byte:02x}")?; @@ -100,7 +73,7 @@ fn lowercase_hex(bytes: &[u8], output: &mut fmt::Formatter<'_>) -> fmt::Result { impl ProtocolId { pub(crate) fn generate() -> io::Result { let mut bytes = [0; 16]; - fill_random(&mut bytes)?; + getrandom::fill(&mut bytes)?; Ok(Self(bytes)) } } @@ -120,7 +93,7 @@ impl fmt::Display for ProtocolId { impl ProtocolToken { pub(crate) fn generate() -> io::Result { let mut bytes = [0; 32]; - fill_random(&mut bytes)?; + getrandom::fill(&mut bytes)?; Ok(Self(bytes)) } } From e009114d63b373e2005aff2664ef68ef6fd11358 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:24:09 +0100 Subject: [PATCH 02/13] ci: enforce a dependency policy with cargo-deny Add deny.toml and a dependency-policy job covering the whole workspace. Bans, licenses, and sources block; advisories report without blocking, so a newly published advisory cannot fail an unrelated pull request. zbus 6 absorbed zvariant, zvariant_derive, and zbus_names, so RustSec now lists them as unmaintained. ksni still requires zbus 5, so those advisories are ignored with that reason until it moves. Both packages now state their MIT license instead of leaving the check to detect it. The configurator is marked publish = false: it has an unversioned path dependency and cannot be published. --- .github/AGENTS.md | 1 + .github/workflows/ci.yml | 23 ++++++++++++++++++++++ AGENTS.md | 1 + CONTRIBUTING.md | 7 +++++++ Cargo.toml | 1 + configurator/Cargo.toml | 2 ++ deny.toml | 42 ++++++++++++++++++++++++++++++++++++++++ flake.nix | 1 + 8 files changed, 78 insertions(+) create mode 100644 deny.toml diff --git a/.github/AGENTS.md b/.github/AGENTS.md index ceaeed1f..0aaf8baf 100644 --- a/.github/AGENTS.md +++ b/.github/AGENTS.md @@ -16,6 +16,7 @@ binary points at that page. Renaming or removing an id silently drops the prefill for binaries that can never be updated retroactively. - Keep Linux system dependencies aligned with real build needs for Wayland, Cairo, Pango, GTK4/libadwaita, D-Bus, packaging, and Nix checks. +- `deny.toml` is the dependency policy and the `dependency-policy` job enforces it. Advisories report without blocking, so a new advisory cannot fail an unrelated pull request; bans, licenses, and sources block. - Do not weaken release checks or skip package layout/version validation without a documented reason. ## Coupled Changes diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8ff15073..b19df8af 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -144,3 +144,26 @@ jobs: - name: Check flake package and development shell instantiation run: dotnet run tools/wayscriber.cs --no-build -- check nix-instantiation + + dependency-policy: + name: Dependency policy (${{ matrix.checks }}) + runs-on: ubuntu-latest + timeout-minutes: 10 + strategy: + fail-fast: false + matrix: + checks: + - advisories + - bans licenses sources + # A newly published advisory must not block unrelated pull requests. + continue-on-error: ${{ matrix.checks == 'advisories' }} + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Check dependency policy + uses: EmbarkStudios/cargo-deny-action@v2 + with: + arguments: --workspace --locked + command: check ${{ matrix.checks }} + rust-version: 1.99.1 diff --git a/AGENTS.md b/AGENTS.md index 1f19ebe2..d0f4a16b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -51,6 +51,7 @@ - Config changes often require `config.example.toml`, `docs/CONFIG.md`, configurator models/views, schema behavior, and tests. - Tool/action/keybinding changes often require config defaults, action metadata, help/command UI, toolbar UI, configurator labels/search, docs, and tests. - Daemon, service, shortcut, path, and packaging changes often need updates across `src/daemon/`, `src/paths/`, `src/systemd_user_service.rs`, `src/shortcut_hint.rs`, `configurator/src/app/daemon_setup/`, and `packaging/`. +- Dependency changes must satisfy `deny.toml`. A new license or duplicate version needs an entry there with its reason. ## Validation - Full local CI is `./tools/lint-and-test.sh`. It needs the .NET SDK selected by `global.json`. Without it, `cargo test` still runs the Rust source guards in `tests/repository_guards` (process sites, config writers, shared dependencies, use statements, no Python), but not the C# checks, including the source-coverage check that finds `.rs` files Cargo never compiles. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bda172f3..4407555b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -139,6 +139,13 @@ Rust tests include the repository guards in `tests/repository_guards`. The sourc gate uses current rustc dep-info and rejects tracked or unignored `.rs` files that are outside the supported Cargo target/feature matrix. +Dependency changes must also satisfy the policy in `deny.toml`, which CI enforces in its own +job. The Nix shell includes `cargo-deny`; elsewhere, install it and run: + +```bash +cargo deny --workspace --locked check +``` + The all-feature portal transport tests require `dbus-daemon`. Each fixture owns a private session bus and connects through its explicit address, leaving the desktop session bus alone. diff --git a/Cargo.toml b/Cargo.toml index 11a74b88..fedf4467 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,6 +3,7 @@ name = "wayscriber" version = "0.9.28" edition = "2024" rust-version = "1.99.1" +license = "MIT" description = "Screen annotation tool for Wayland compositors" homepage = "https://wayscriber.com" repository = "https://github.com/devmobasa/wayscriber" diff --git a/configurator/Cargo.toml b/configurator/Cargo.toml index 0b9608ed..a41be09e 100644 --- a/configurator/Cargo.toml +++ b/configurator/Cargo.toml @@ -3,6 +3,8 @@ name = "wayscriber-configurator" version = "0.9.28" edition = "2024" rust-version = "1.99.1" +license = "MIT" +publish = false [lints] workspace = true diff --git a/deny.toml b/deny.toml new file mode 100644 index 00000000..d70375b6 --- /dev/null +++ b/deny.toml @@ -0,0 +1,42 @@ +# Dependency policy for the workspace. CI runs it in the `dependency-policy` job; +# `cargo deny --workspace --locked check` gives the same result locally. + +[graph] +all-features = true +targets = [ + { triple = "x86_64-unknown-linux-gnu" }, + { triple = "aarch64-unknown-linux-gnu" }, +] + +[advisories] +# zbus 6 absorbed these crates. ksni, which the `tray` feature needs, still +# requires zbus 5, so remove these entries once it moves. +ignore = [ + { id = "RUSTSEC-2026-0370", reason = "zvariant: blocked on ksni supporting zbus 6" }, + { id = "RUSTSEC-2026-0371", reason = "zbus_names: blocked on ksni supporting zbus 6" }, + { id = "RUSTSEC-2026-0372", reason = "zvariant_derive: blocked on ksni supporting zbus 6" }, +] + +[licenses] +allow = [ + "Apache-2.0", + "Apache-2.0 WITH LLVM-exception", + "MIT", + "Unicode-3.0", + "Unlicense", +] + +[bans] +multiple-versions = "deny" +wildcards = "deny" +allow-wildcard-paths = true +skip = [ + { crate = "miniz_oxide@0.8", reason = "png needs 0.8 while flate2 needs 0.9" }, + { crate = "syn@2", reason = "proc macros have not all moved to syn 3" }, + { crate = "system-deps@7", reason = "build-only: the GTK sys crates span three majors" }, + { crate = "system-deps@8", reason = "build-only: the GTK sys crates span three majors" }, +] + +[sources] +unknown-registry = "deny" +unknown-git = "deny" diff --git a/flake.nix b/flake.nix index 43b179a8..46258f24 100644 --- a/flake.nix +++ b/flake.nix @@ -32,6 +32,7 @@ servicePath = pkgs.lib.makeBinPath [ pkgs.grim pkgs.slurp pkgs.wl-clipboard ]; developmentInputs = with pkgs; [ rustToolchain + cargo-deny util-linux pkg-config cairo From 8d0c13f946e57b22e85a53dfa0b2796899897e7f Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:24:09 +0100 Subject: [PATCH 03/13] ci: add weekly dependabot updates Propose Cargo and GitHub Actions updates, grouping compatible Cargo bumps. Hold zbus at its current major while ksni requires zbus 5. --- .github/AGENTS.md | 1 + .github/dependabot.yml | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 .github/dependabot.yml diff --git a/.github/AGENTS.md b/.github/AGENTS.md index 0aaf8baf..9babd9c4 100644 --- a/.github/AGENTS.md +++ b/.github/AGENTS.md @@ -17,6 +17,7 @@ prefill for binaries that can never be updated retroactively. - Keep Linux system dependencies aligned with real build needs for Wayland, Cairo, Pango, GTK4/libadwaita, D-Bus, packaging, and Nix checks. - `deny.toml` is the dependency policy and the `dependency-policy` job enforces it. Advisories report without blocking, so a new advisory cannot fail an unrelated pull request; bans, licenses, and sources block. +- `dependabot.yml` proposes weekly Cargo and GitHub Actions updates. Its zbus major-version ignore is coupled to the advisory ignores in `deny.toml`. - Do not weaken release checks or skip package layout/version validation without a documented reason. ## Coupled Changes diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..08ed61af --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,27 @@ +version: 2 +updates: + - package-ecosystem: cargo + directory: / + schedule: + interval: weekly + open-pull-requests-limit: 5 + groups: + compatible: + update-types: + - minor + - patch + ignore: + # ksni, which the tray feature needs, still requires zbus 5. Remove this + # with the matching advisory ignores in deny.toml. + - dependency-name: zbus + update-types: + - version-update:semver-major + + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + groups: + actions: + patterns: + - "*" From 1dafb9385b2f2c5616e83c2c14fec1f938ac4dd3 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:25:15 +0100 Subject: [PATCH 04/13] docs: add nextest as the fast local test run The local gate serializes tests because of a native font race. nextest gives each test its own process, so the whole suite can run in parallel while iterating. It is local only: it skips doctests and the isolated render tests, so the gate stays as it is. Add it to the Nix shell. --- AGENTS.md | 1 + CONTRIBUTING.md | 10 ++++++++++ flake.nix | 1 + 3 files changed, 12 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index d0f4a16b..2f013d49 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,6 +54,7 @@ - Dependency changes must satisfy `deny.toml`. A new license or duplicate version needs an entry there with its reason. ## Validation +- For a fast full-suite run while iterating, use `cargo nextest run --workspace --all-features`. Each test gets its own process, so it runs in parallel despite the native-font race that keeps the gate serial. It skips doctests and the isolated render tests, so it does not replace the gate. - Full local CI is `./tools/lint-and-test.sh`. It needs the .NET SDK selected by `global.json`. Without it, `cargo test` still runs the Rust source guards in `tests/repository_guards` (process sites, config writers, shared dependencies, use statements, no Python), but not the C# checks, including the source-coverage check that finds `.rs` files Cargo never compiles. - That script runs the C# repository checks (assets, version, nixpkgs recipe, source coverage, legacy tools), the C# tool tests (which also run the packaging shell contracts), `cargo fmt --all -- --check`, clippy with all targets/features, all-feature tests, and no-default-feature tests. - Run `git diff --check` before handoff. When relevant files are untracked and the index must stay unchanged, check those files directly as well. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4407555b..700426fd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -125,6 +125,16 @@ cargo test session:: cargo test -p wayscriber-configurator ``` +For a fast run of the whole suite, use [cargo-nextest](https://nexte.st/), which the Nix shell +includes. It gives each test its own process, so it can run in parallel despite the native font +race that keeps the local CI entry point serial: + +```bash +cargo nextest run --workspace --all-features +``` + +It skips doctests and the isolated render tests, so it does not replace that entry point. + Before submitting a broad or cross-package change, run the local CI entry point: ```bash diff --git a/flake.nix b/flake.nix index 46258f24..583af8f8 100644 --- a/flake.nix +++ b/flake.nix @@ -33,6 +33,7 @@ developmentInputs = with pkgs; [ rustToolchain cargo-deny + cargo-nextest util-linux pkg-config cairo From d3b67cae569942700585f06aa8a0d5dc04cf7640 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:27:51 +0100 Subject: [PATCH 05/13] refactor(cli): tokenize arguments with lexopt Replace the hand-written tokenizer and its short-cluster and attached value helpers. Every accepted form and value rule stays the same, now pinned by tests, including that a value starting with - must be attached. Two edges change: a bare -- now ends options as usual, and an unknown -x reports as an unknown short option like one inside a cluster. Non-UTF-8 session paths now reach the specific error for that case instead of the generic one; they are still rejected. --- Cargo.lock | 7 + Cargo.toml | 2 + src/cli.rs | 236 ++++++++++--------------- src/cli/tests.rs | 103 +++++++++++ tools/csharp/Commands/ChecksCommand.cs | 1 + 5 files changed, 207 insertions(+), 142 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0d12739b..2449a5d8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -870,6 +870,12 @@ dependencies = [ "zbus", ] +[[package]] +name = "lexopt" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "803ec87c9cfb29b9d2633f20cba1f488db3fd53f2158b1024cbefb47ba05d413" + [[package]] name = "libadwaita" version = "0.9.2" @@ -1882,6 +1888,7 @@ dependencies = [ "gtk4-layer-shell", "input", "ksni", + "lexopt", "libc", "log", "pango", diff --git a/Cargo.toml b/Cargo.toml index fedf4467..d871ff0d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -99,6 +99,8 @@ getrandom = { version = "0.4", features = ["std"] } tempfile = "3" flate2 = "1.0" zune-jpeg = "0.5" +# Command-line tokenizing without a parser framework. +lexopt = "0.3" # Grapheme/word segmentation for the text-editor caret and selection. unicode-segmentation = "1" diff --git a/src/cli.rs b/src/cli.rs index d58b8d8e..ec2a87ff 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -2,6 +2,8 @@ use std::env; use std::ffi::OsString; use std::path::PathBuf; +use lexopt::{Arg, Parser}; + use crate::domain::Language; use crate::tray_action::TrayAction; use crate::{build_info, i18n, runtime_capabilities}; @@ -105,96 +107,70 @@ impl Cli { I: IntoIterator, T: Into, { - let mut args = args - .into_iter() - .map(|arg| { - arg.into() - .into_string() - .map_err(|_| "arguments must be valid UTF-8".to_string()) - }) - .collect::, _>>()?; - - if args.is_empty() { - args.push("wayscriber".to_string()); - } - + let mut parser = Parser::from_iter(args.into_iter().map(Into::into)); let mut cli = Cli::default(); - let mut index = 1; - while index < args.len() { - let arg = &args[index]; - match arg.as_str() { - "-h" | "--help" => return Ok(CliOutcome::Help), - "-V" | "--version" => return Ok(CliOutcome::Version), - "-d" | "--daemon" => cli.daemon = true, - "--daemon-toggle" => cli.daemon_toggle = true, - "--daemon-action" => { - index += 1; - cli.daemon_action = Some(value_after(&args, index, "--daemon-action")?); + + while let Some(arg) = parser.next().map_err(|error| parse_error(&error))? { + match arg { + Arg::Short('h') => return Ok(CliOutcome::Help), + Arg::Short('V') => return Ok(CliOutcome::Version), + Arg::Long("help") => { + reject_attached_value(&mut parser, "--help")?; + return Ok(CliOutcome::Help); } - "--light-toggle" => cli.light_toggle = true, - "--light-draw-toggle" => cli.light_draw_toggle = true, - "--light-draw-on" => cli.light_draw_on = true, - "--light-draw-off" => cli.light_draw_off = true, - "-a" | "--active" => cli.active = true, - "-m" | "--mode" => { - index += 1; - cli.mode = Some(value_after(&args, index, "--mode")?); + Arg::Long("version") => { + reject_attached_value(&mut parser, "--version")?; + return Ok(CliOutcome::Version); } - "--no-tray" => cli.no_tray = true, - "--freeze-on-show" => cli.freeze_on_show = true, - "--clear-session" => cli.clear_session = true, - "--clear-tool-state" => cli.clear_tool_state = true, - "--session-info" => cli.session_info = true, - "--rename-session" => { - index += 1; - cli.rename_session = Some(value_after(&args, index, "--rename-session")?); + Arg::Short('d') | Arg::Long("daemon") => cli.daemon = true, + Arg::Long("daemon-toggle") => cli.daemon_toggle = true, + Arg::Long("daemon-action") => { + cli.daemon_action = Some(string_value(&mut parser, "--daemon-action")?); } - "--session-file" => { - index += 1; + Arg::Long("light-toggle") => cli.light_toggle = true, + Arg::Long("light-draw-toggle") => cli.light_draw_toggle = true, + Arg::Long("light-draw-on") => cli.light_draw_on = true, + Arg::Long("light-draw-off") => cli.light_draw_off = true, + Arg::Short('a') | Arg::Long("active") => cli.active = true, + Arg::Short('m') => cli.mode = Some(string_value(&mut parser, "-m")?), + Arg::Long("mode") => cli.mode = Some(string_value(&mut parser, "--mode")?), + Arg::Long("no-tray") => cli.no_tray = true, + Arg::Long("freeze-on-show") => cli.freeze_on_show = true, + Arg::Long("clear-session") => cli.clear_session = true, + Arg::Long("clear-tool-state") => cli.clear_tool_state = true, + Arg::Long("session-info") => cli.session_info = true, + Arg::Long("rename-session") => { + cli.rename_session = Some(string_value(&mut parser, "--rename-session")?); + } + Arg::Long("session-file") => { cli.session_file = - Some(PathBuf::from(value_after(&args, index, "--session-file")?)); + Some(PathBuf::from(option_value(&mut parser, "--session-file")?)); } - "--freeze" => cli.freeze = true, - "--exit-after-capture" => cli.exit_after_capture = true, - "--no-exit-after-capture" => cli.no_exit_after_capture = true, - "--resume-session" => cli.resume_session = true, - "--no-resume-session" => cli.no_resume_session = true, - "--about" => cli.about = true, - "--check-update" => cli.check_update = true, - "--language" => { - index += 1; - cli.language = Some(parse_language(&value_after(&args, index, "--language")?)?); + Arg::Long("freeze") => cli.freeze = true, + Arg::Long("exit-after-capture") => cli.exit_after_capture = true, + Arg::Long("no-exit-after-capture") => cli.no_exit_after_capture = true, + Arg::Long("resume-session") => cli.resume_session = true, + Arg::Long("no-resume-session") => cli.no_resume_session = true, + Arg::Long("about") => cli.about = true, + Arg::Long("check-update") => cli.check_update = true, + Arg::Long("language") => { + cli.language = Some(parse_language(&string_value(&mut parser, "--language")?)?); } - "--list-languages" => cli.list_languages = true, - runtime_capabilities::RUNTIME_CAPABILITIES_FLAG => { + Arg::Long("list-languages") => cli.list_languages = true, + Arg::Long(name) + if runtime_capabilities::RUNTIME_CAPABILITIES_FLAG.strip_prefix("--") + == Some(name) => + { cli.runtime_capabilities = true; } - _ if arg.starts_with("--daemon-action=") => { - cli.daemon_action = Some(value_from_equals(arg, "--daemon-action")?); + Arg::Long(name) => { + let option = format!("--{name}"); + reject_attached_value(&mut parser, &option)?; + return Err(unknown_argument(&option)); } - _ if arg.starts_with("--mode=") => { - cli.mode = Some(value_from_equals(arg, "--mode")?); - } - _ if arg.starts_with("--session-file=") => { - cli.session_file = - Some(PathBuf::from(value_from_equals(arg, "--session-file")?)); - } - _ if arg.starts_with("--rename-session=") => { - cli.rename_session = Some(value_from_equals(arg, "--rename-session")?); - } - _ if arg.starts_with("--language=") => { - cli.language = Some(parse_language(&value_from_equals(arg, "--language")?)?); - } - _ if is_short_option_cluster(arg) => { - match parse_short_option_cluster(&args, index, &mut cli)? { - ShortOptionOutcome::Continue(next_index) => index = next_index, - ShortOptionOutcome::Help => return Ok(CliOutcome::Help), - ShortOptionOutcome::Version => return Ok(CliOutcome::Version), - } - } - _ => return Err(format!("unknown argument '{arg}'")), + Arg::Short(flag) => return Err(format!("unknown short option '-{flag}'")), + Arg::Value(value) => return Err(unknown_argument(&value.display().to_string())), } - index += 1; } cli.validate()?; @@ -525,77 +501,53 @@ fn parse_language(value: &str) -> Result { }) } -fn is_short_option_cluster(arg: &str) -> bool { - arg.starts_with('-') && !arg.starts_with("--") && arg.len() > 2 +/// Reads an option's value, either attached (`--mode=x`, `-mx`, `-m=x`) or as +/// the next argument. A next argument starting with `-` is the next option, so +/// a value that starts with `-` has to be attached. +fn option_value(parser: &mut Parser, name: &str) -> Result { + let requires_value = || format!("{name} requires a value"); + + if let Some(value) = parser.optional_value() { + return if value.is_empty() { + Err(requires_value()) + } else { + Ok(value) + }; + } + + let value = parser.value().map_err(|_| requires_value())?; + if value.as_encoded_bytes().starts_with(b"-") { + return Err(requires_value()); + } + Ok(value) } -enum ShortOptionOutcome { - Continue(usize), - Help, - Version, +fn string_value(parser: &mut Parser, name: &str) -> Result { + option_value(parser, name)? + .into_string() + .map_err(|_| "arguments must be valid UTF-8".to_string()) } -fn parse_short_option_cluster( - args: &[String], - index: usize, - cli: &mut Cli, -) -> Result { - let arg = &args[index]; - for (offset, flag) in arg[1..].char_indices() { - match flag { - 'h' => return Ok(ShortOptionOutcome::Help), - 'V' => return Ok(ShortOptionOutcome::Version), - 'd' => cli.daemon = true, - 'a' => cli.active = true, - 'm' => { - let value_start = 1 + offset + flag.len_utf8(); - let value = if value_start < arg.len() { - attached_short_value(arg, value_start, "-m")? - } else { - value_after(args, index + 1, "-m")? - }; - cli.mode = Some(value); - return Ok(ShortOptionOutcome::Continue(if value_start < arg.len() { - index - } else { - index + 1 - })); - } - _ => return Err(format!("unknown short option '-{flag}'")), +/// Rejects `--flag=value` for an option that is about to stop parsing, which +/// would otherwise skip the check `Parser::next` makes on its following call. +fn reject_attached_value(parser: &mut Parser, name: &str) -> Result<(), String> { + match parser.optional_value() { + Some(value) => Err(unknown_argument(&format!("{name}={}", value.display()))), + None => Ok(()), + } +} + +fn parse_error(error: &lexopt::Error) -> String { + match error { + lexopt::Error::UnexpectedValue { option, value } => { + unknown_argument(&format!("{option}={}", value.display())) } + other => other.to_string(), } - Ok(ShortOptionOutcome::Continue(index)) } -fn value_after(args: &[String], index: usize, name: &str) -> Result { - let value = args - .get(index) - .ok_or_else(|| format!("{name} requires a value"))?; - if value.starts_with('-') { - return Err(format!("{name} requires a value")); - } - Ok(value.clone()) -} - -fn attached_short_value(arg: &str, value_start: usize, name: &str) -> Result { - let value = arg[value_start..] - .strip_prefix('=') - .unwrap_or_else(|| &arg[value_start..]); - if value.is_empty() { - return Err(format!("{name} requires a value")); - } - Ok(value.to_string()) -} - -fn value_from_equals(arg: &str, name: &str) -> Result { - let value = arg - .split_once('=') - .map(|(_, value)| value) - .unwrap_or_default(); - if value.is_empty() { - return Err(format!("{name} requires a value")); - } - Ok(value.to_string()) +fn unknown_argument(arg: &str) -> String { + format!("unknown argument '{arg}'") } fn conflict(left: &str, right: &str) -> String { diff --git a/src/cli/tests.rs b/src/cli/tests.rs index 3d28a639..cbe12ea9 100644 --- a/src/cli/tests.rs +++ b/src/cli/tests.rs @@ -1,3 +1,5 @@ +use std::ffi::OsString; +use std::os::unix::ffi::OsStringExt; use std::path::PathBuf; use super::{Cli, CliOutcome}; @@ -601,3 +603,104 @@ fn list_languages_is_a_print_and_exit_command() { ); } } + +#[test] +fn a_value_starting_with_a_dash_must_be_attached() { + for args in [ + &["wayscriber", "--mode", "-x"][..], + &["wayscriber", "-m", "-x"], + &["wayscriber", "--mode", "--active"], + ] { + assert!( + Cli::try_parse_from(args.iter().copied()) + .unwrap_err() + .ends_with("requires a value"), + "{args:?}" + ); + } + + assert_eq!( + parse_cli(["wayscriber", "--mode=-x"]).mode.as_deref(), + Some("-x") + ); + assert_eq!( + parse_cli(["wayscriber", "-m=-x"]).mode.as_deref(), + Some("-x") + ); + assert_eq!( + parse_cli(["wayscriber", "-m-x"]).mode.as_deref(), + Some("-x") + ); +} + +#[test] +fn an_empty_attached_value_is_missing_but_a_separate_one_is_not() { + for (args, name) in [ + (&["wayscriber", "--mode="][..], "--mode"), + (&["wayscriber", "-m="], "-m"), + (&["wayscriber", "--language="], "--language"), + ] { + assert_eq!( + Cli::try_parse_from(args.iter().copied()).unwrap_err(), + format!("{name} requires a value") + ); + } + + assert_eq!( + parse_cli(["wayscriber", "--mode", ""]).mode.as_deref(), + Some("") + ); +} + +#[test] +fn unknown_and_misused_options_are_rejected() { + for (args, expected) in [ + (&["wayscriber", "--nope"][..], "unknown argument '--nope'"), + (&["wayscriber", "--nope=1"], "unknown argument '--nope=1'"), + ( + &["wayscriber", "--active=1"], + "unknown argument '--active=1'", + ), + (&["wayscriber", "--help=1"], "unknown argument '--help=1'"), + ( + &["wayscriber", "--version=1"], + "unknown argument '--version=1'", + ), + (&["wayscriber", "stray"], "unknown argument 'stray'"), + (&["wayscriber", "-"], "unknown argument '-'"), + (&["wayscriber", "--", "stray"], "unknown argument 'stray'"), + (&["wayscriber", "-x"], "unknown short option '-x'"), + (&["wayscriber", "-dx"], "unknown short option '-x'"), + ] { + assert_eq!( + Cli::try_parse_from(args.iter().copied()).unwrap_err(), + expected, + "{args:?}" + ); + } +} + +#[test] +fn a_session_path_keeps_its_bytes_but_other_values_must_be_utf8() { + let raw_path = OsString::from_vec(b"/tmp/s\xff.wayscriber-session".to_vec()); + + let outcome = Cli::try_parse_from([ + OsString::from("wayscriber"), + OsString::from("--active"), + OsString::from("--session-file"), + raw_path.clone(), + ]) + .unwrap(); + let CliOutcome::Run(cli) = outcome else { + panic!("expected CLI run outcome, got {outcome:?}"); + }; + assert_eq!(cli.session_file, Some(PathBuf::from(raw_path))); + + let error = Cli::try_parse_from([ + OsString::from("wayscriber"), + OsString::from("--mode"), + OsString::from_vec(b"\xff".to_vec()), + ]) + .unwrap_err(); + assert_eq!(error, "arguments must be valid UTF-8"); +} diff --git a/tools/csharp/Commands/ChecksCommand.cs b/tools/csharp/Commands/ChecksCommand.cs index 2c09fd54..f897b69b 100644 --- a/tools/csharp/Commands/ChecksCommand.cs +++ b/tools/csharp/Commands/ChecksCommand.cs @@ -334,6 +334,7 @@ internal static class ChecksCommand [RepositoryNames.Gtk4LayerShell] = [RepositoryNames.Gtk4LayerShell], ["input"] = ["libinput"], ["ksni"] = [], + ["lexopt"] = [], ["libc"] = [], ["log"] = [], ["pango"] = ["pango"], From 572bc3ba44f8ad33ab002a31fbe5bce732e238a9 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:32:16 +0100 Subject: [PATCH 06/13] refactor: take advisory locks through rustix Add rustix as a direct dependency. It is already compiled through the Wayland stack, so this adds no new crate. Use its typed flock operations for the durable, daemon protocol, and action journal locks, which removes their unsafe blocks. Deadline locks still always retry without blocking. --- Cargo.lock | 1 + Cargo.toml | 2 ++ src/daemon/protocol_v2/action.rs | 22 +++++++-------- src/daemon/protocol_v2/action/tests.rs | 16 +++-------- src/daemon/protocol_v2/command/action_link.rs | 9 ++++--- src/daemon/protocol_v2/command/claimed.rs | 12 ++++++--- src/daemon/protocol_v2/command/client.rs | 21 ++++++++++++--- src/daemon/protocol_v2/command/layout.rs | 27 ++++++++++--------- src/daemon/protocol_v2/command/owner.rs | 19 ++++++++----- src/daemon/protocol_v2/command/recovery.rs | 3 ++- src/daemon/protocol_v2/command/staging.rs | 3 ++- src/daemon/protocol_v2/command/tests.rs | 14 +++++----- src/durable_io/lock.rs | 22 +++++---------- tools/csharp/Commands/ChecksCommand.cs | 1 + 14 files changed, 96 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2449a5d8..8d11b397 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1894,6 +1894,7 @@ dependencies = [ "pango", "pangocairo", "png", + "rustix", "schemars", "serde", "serde_ignored", diff --git a/Cargo.toml b/Cargo.toml index d871ff0d..f6b9622d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -93,6 +93,8 @@ serde = { version = "1.0", features = ["derive"] } serde_ignored = "0.1" schemars = { version = "1.2", features = ["derive"], optional = true } libc = "0.2" +# Safe wrappers for the kernel interfaces that the std library does not cover. +rustix = { version = "1", features = ["event", "fs", "net", "pipe", "process", "stdio", "termios", "thread", "time"] } # Kernel random bytes for portal handle tokens and daemon protocol identifiers. getrandom = { version = "0.4", features = ["std"] } # Securely created, self-deleting temporary files for the OCR engine handoff. diff --git a/src/daemon/protocol_v2/action.rs b/src/daemon/protocol_v2/action.rs index ee637e91..bb378674 100644 --- a/src/daemon/protocol_v2/action.rs +++ b/src/daemon/protocol_v2/action.rs @@ -2,7 +2,6 @@ use std::collections::BTreeMap; use std::fs::{self, File, OpenOptions}; use std::io::{self, ErrorKind}; -use std::os::fd::AsRawFd; use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; #[cfg(test)] @@ -11,6 +10,7 @@ use std::sync::LazyLock; use std::sync::Mutex; use anyhow::{Context, Result, anyhow, bail}; +use rustix::fs::{self as rustix_fs, FlockOperation}; use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; @@ -224,10 +224,13 @@ fn try_open_journal_lock(root: &Path, nonblocking: bool) -> Result> if !file.metadata()?.is_file() { bail!("action journal lock is not a regular file"); } - let operation = libc::LOCK_EX | if nonblocking { libc::LOCK_NB } else { 0 }; - // SAFETY: file owns the descriptor; flock retains no pointer. - if unsafe { libc::flock(file.as_raw_fd(), operation) } != 0 { - let error = io::Error::last_os_error(); + let operation = if nonblocking { + FlockOperation::NonBlockingLockExclusive + } else { + FlockOperation::LockExclusive + }; + if let Err(errno) = rustix_fs::flock(&file, operation) { + let error = io::Error::from(errno); if nonblocking && error.kind() == ErrorKind::WouldBlock { return Ok(None); } @@ -237,12 +240,9 @@ fn try_open_journal_lock(root: &Path, nonblocking: bool) -> Result> } fn unlock(file: &File) -> Result<()> { - // SAFETY: file owns the descriptor; flock retains no pointer. - if unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_UN) } == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()).context("failed to unlock action journal") - } + rustix_fs::flock(file, FlockOperation::Unlock) + .map_err(io::Error::from) + .context("failed to unlock action journal") } fn write_record(path: &Path, value: &T) -> Result<()> { diff --git a/src/daemon/protocol_v2/action/tests.rs b/src/daemon/protocol_v2/action/tests.rs index 8ba46acb..181ebeb3 100644 --- a/src/daemon/protocol_v2/action/tests.rs +++ b/src/daemon/protocol_v2/action/tests.rs @@ -1,5 +1,7 @@ use std::{env, thread}; +use rustix::fs::{self as rustix_fs, FlockOperation}; + use crate::{test_env, test_temp}; use super::*; @@ -179,21 +181,11 @@ fn event_loop_claim_and_finish_defer_instead_of_waiting_for_locks() { .write(true) .open(decision_path) .unwrap(); - assert_eq!( - // SAFETY: `held_decision` keeps the descriptor open for the call, - // and flock retains no pointer. - unsafe { libc::flock(held_decision.as_raw_fd(), libc::LOCK_EX) }, - 0 - ); + rustix_fs::flock(&held_decision, FlockOperation::LockExclusive).unwrap(); let ActionFinishOutcome::Deferred(action) = action.try_finish(true, None).unwrap() else { panic!("contended command finish should defer"); }; - assert_eq!( - // SAFETY: `held_decision` keeps the descriptor open for the call, - // and flock retains no pointer. - unsafe { libc::flock(held_decision.as_raw_fd(), libc::LOCK_UN) }, - 0 - ); + rustix_fs::flock(&held_decision, FlockOperation::Unlock).unwrap(); assert!(matches!( action.try_finish(true, None).unwrap(), ActionFinishOutcome::Complete diff --git a/src/daemon/protocol_v2/command/action_link.rs b/src/daemon/protocol_v2/command/action_link.rs index f91e706f..e474792e 100644 --- a/src/daemon/protocol_v2/command/action_link.rs +++ b/src/daemon/protocol_v2/command/action_link.rs @@ -2,6 +2,7 @@ use std::io::ErrorKind; use std::time::Duration; use anyhow::{Context, Result, bail}; +use rustix::fs::FlockOperation; use super::super::BootClock; use super::super::action::PreparedAction; @@ -37,14 +38,14 @@ fn claim_command_action_inner( let path = control_path(&command_root(), command_identity); let decision = open_lock(&path.join("decision.lock"), false)?; if nonblocking { - match flock(&decision, libc::LOCK_EX | libc::LOCK_NB) { + match flock(&decision, FlockOperation::NonBlockingLockExclusive) { Ok(()) => {} Err(error) if error.kind() == ErrorKind::WouldBlock => return Ok(None), Err(error) => return Err(error).context("failed to try command-action decision lock"), } } else { let deadline = BootClock::now()?.checked_add(Duration::from_millis(200))?; - lock_until(&decision, libc::LOCK_EX, deadline)?; + lock_until(&decision, FlockOperation::LockExclusive, deadline)?; } let mut control = read_control(&path)?; let exact = |status: &ActionStatus| match status { @@ -193,14 +194,14 @@ fn finish_command_action_inner( let path = control_path(&command_root(), command_identity); let decision = open_lock(&path.join("decision.lock"), false)?; if nonblocking { - match flock(&decision, libc::LOCK_EX | libc::LOCK_NB) { + match flock(&decision, FlockOperation::NonBlockingLockExclusive) { Ok(()) => {} Err(error) if error.kind() == ErrorKind::WouldBlock => return Ok(false), Err(error) => return Err(error).context("failed to try command-action decision lock"), } } else { let deadline = BootClock::now()?.checked_add(Duration::from_millis(200))?; - lock_until(&decision, libc::LOCK_EX, deadline)?; + lock_until(&decision, FlockOperation::LockExclusive, deadline)?; } let mut control = read_control(&path)?; let applied = result == CommandActionResult::Applied; diff --git a/src/daemon/protocol_v2/command/claimed.rs b/src/daemon/protocol_v2/command/claimed.rs index 1c386649..3fb2bfd5 100644 --- a/src/daemon/protocol_v2/command/claimed.rs +++ b/src/daemon/protocol_v2/command/claimed.rs @@ -1,6 +1,7 @@ use std::time::Duration; use anyhow::{Result, anyhow, bail}; +use rustix::fs::FlockOperation; #[cfg(test)] use super::super::wire::CommandControl; @@ -61,7 +62,8 @@ impl ClaimedCommand { journal.prepare_command(&expected.identity, &expected.target_daemon_token, action)?; let deadline = BootDeadline::from_nanos(expected.submission_clock.authorization_deadline_boottime_ns); - if let Err(error) = lock_until(&self.decision_lock, libc::LOCK_EX, deadline) { + if let Err(error) = lock_until(&self.decision_lock, FlockOperation::LockExclusive, deadline) + { let _ = journal.abandon(&prepared, "command decision lock was not reacquired"); return Err(error); } @@ -71,7 +73,11 @@ impl ClaimedCommand { let reason = "command was canceled during action preparation"; journal.abandon(&prepared, reason)?; let recovery_deadline = BootClock::now()?.checked_add(Duration::from_millis(200))?; - lock_until(&self.decision_lock, libc::LOCK_EX, recovery_deadline)?; + lock_until( + &self.decision_lock, + FlockOperation::LockExclusive, + recovery_deadline, + )?; let mut latest = read_control(&self.control_path)?; if latest != current || !matches!( @@ -221,6 +227,6 @@ impl ClaimedCommand { impl Drop for ClaimedCommand { fn drop(&mut self) { - let _ = flock(&self.decision_lock, libc::LOCK_UN); + let _ = flock(&self.decision_lock, FlockOperation::Unlock); } } diff --git a/src/daemon/protocol_v2/command/client.rs b/src/daemon/protocol_v2/command/client.rs index 08ed3847..21c03c2d 100644 --- a/src/daemon/protocol_v2/command/client.rs +++ b/src/daemon/protocol_v2/command/client.rs @@ -3,6 +3,7 @@ use std::time::Duration; use std::{fs, process, thread}; use anyhow::{Context, Result, bail}; +use rustix::fs::FlockOperation; #[cfg(test)] use anyhow::anyhow; @@ -62,8 +63,16 @@ impl ClientCommand { create_private_directory(&stage)?; let decision_lock = open_lock(&stage.join("decision.lock"), true)?; let caller_lease = open_lock(&stage.join("caller.lease"), true)?; - lock_until(&caller_lease, libc::LOCK_SH, authorization_deadline)?; - lock_until(&decision_lock, libc::LOCK_EX, authorization_deadline)?; + lock_until( + &caller_lease, + FlockOperation::LockShared, + authorization_deadline, + )?; + lock_until( + &decision_lock, + FlockOperation::LockExclusive, + authorization_deadline, + )?; let mut control = CommandControl { protocol_version: DAEMON_COMMAND_PROTOCOL_VERSION, @@ -154,7 +163,7 @@ impl ClientCommand { let slice = now .checked_add(Duration::from_millis(20))? .min(self.response_deadline); - if !try_lock_until(&self.decision_lock, libc::LOCK_EX, slice)? { + if !try_lock_until(&self.decision_lock, FlockOperation::LockExclusive, slice)? { if slice < self.response_deadline { continue; } @@ -201,7 +210,11 @@ impl ClientCommand { let _ = unlock(&self.caller_lease); return Ok(TerminalCommandResult::AdmittedIndeterminate(reason)); } - lock_until(&self.decision_lock, libc::LOCK_EX, self.response_deadline)?; + lock_until( + &self.decision_lock, + FlockOperation::LockExclusive, + self.response_deadline, + )?; let mut control = read_control(&self.control_path)?; if cancel_open(&mut control)? { write_control(&self.control_path, &control)?; diff --git a/src/daemon/protocol_v2/command/layout.rs b/src/daemon/protocol_v2/command/layout.rs index 00e956c2..67efc6f9 100644 --- a/src/daemon/protocol_v2/command/layout.rs +++ b/src/daemon/protocol_v2/command/layout.rs @@ -1,13 +1,13 @@ use std::collections::BTreeSet; use std::fs::{self, File, OpenOptions}; use std::io::{self, ErrorKind}; -use std::os::fd::AsRawFd; use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; use std::thread; use std::time::Duration; use anyhow::{Context, Result, anyhow, bail}; +use rustix::fs::{self as rustix_fs, FlockOperation}; use serde::de::DeserializeOwned; use crate::durable_io::AtomicWriteOptions; @@ -279,22 +279,25 @@ pub(super) fn open_lock(path: &Path, create_new: bool) -> Result { Ok(file) } -pub(super) fn flock(file: &File, operation: libc::c_int) -> io::Result<()> { - // SAFETY: file owns a valid descriptor and flock does not retain it. - if unsafe { libc::flock(file.as_raw_fd(), operation) } == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()) +pub(super) fn flock(file: &File, operation: FlockOperation) -> io::Result<()> { + rustix_fs::flock(file, operation).map_err(io::Error::from) +} + +fn nonblocking(operation: FlockOperation) -> FlockOperation { + match operation { + FlockOperation::LockShared => FlockOperation::NonBlockingLockShared, + FlockOperation::LockExclusive => FlockOperation::NonBlockingLockExclusive, + other => other, } } pub(super) fn unlock(file: &File) -> Result<()> { - flock(file, libc::LOCK_UN).context("failed to unlock protocol descriptor") + flock(file, FlockOperation::Unlock).context("failed to unlock protocol descriptor") } pub(super) fn lock_until( file: &File, - operation: libc::c_int, + operation: FlockOperation, deadline: BootDeadline, ) -> Result<()> { if try_lock_until(file, operation, deadline)? { @@ -306,11 +309,11 @@ pub(super) fn lock_until( pub(super) fn try_lock_until( file: &File, - operation: libc::c_int, + operation: FlockOperation, deadline: BootDeadline, ) -> Result { loop { - match flock(file, operation | libc::LOCK_NB) { + match flock(file, nonblocking(operation)) { Ok(()) => return Ok(true), Err(error) if error.kind() == ErrorKind::WouldBlock => { if BootClock::now()? >= deadline { @@ -341,7 +344,7 @@ pub(super) fn try_admission_lock(root: &Path, deadline: BootDeadline) -> Result< } Err(error) => return Err(error), }; - if try_lock_until(&file, libc::LOCK_EX, deadline)? { + if try_lock_until(&file, FlockOperation::LockExclusive, deadline)? { Ok(Some(file)) } else { Ok(None) diff --git a/src/daemon/protocol_v2/command/owner.rs b/src/daemon/protocol_v2/command/owner.rs index 62b0771a..07e7d001 100644 --- a/src/daemon/protocol_v2/command/owner.rs +++ b/src/daemon/protocol_v2/command/owner.rs @@ -5,6 +5,7 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::{Context, Result, bail}; +use rustix::fs::FlockOperation; use super::super::wire::{ ActionStatus, CallerDisposition, ChildReportStatus, CommandDecision, CommandResponse, @@ -191,7 +192,11 @@ impl CommandOwner { Err(error) => return Err(error), }; let retry_deadline = BootClock::now()?.checked_add(Duration::from_millis(200))?; - if !try_lock_until(&decision_lock, libc::LOCK_EX, retry_deadline)? { + if !try_lock_until( + &decision_lock, + FlockOperation::LockExclusive, + retry_deadline, + )? { return Ok(None); } let Ok(mut control) = read_control(&path) else { @@ -316,7 +321,7 @@ impl CommandOwner { Err(error) => return Err(error), }; let deadline = BootClock::now()?.checked_add(Duration::from_millis(200))?; - lock_until(&decision, libc::LOCK_EX, deadline)?; + lock_until(&decision, FlockOperation::LockExclusive, deadline)?; let mut control = read_control(&path)?; if control.identity == reference.command_identity && control.queue_order == reference.queue_order @@ -341,13 +346,13 @@ impl CommandOwner { observed: &super::super::wire::CommandControl, ) -> Result<()> { let caller_lease = open_lock(&path.join("caller.lease"), false)?; - match flock(&caller_lease, libc::LOCK_EX | libc::LOCK_NB) { + match flock(&caller_lease, FlockOperation::NonBlockingLockExclusive) { Ok(()) => {} Err(error) if error.kind() == ErrorKind::WouldBlock => return Ok(()), Err(error) => return Err(error).context("failed to inspect unqueued caller lease"), } let decision = open_lock(&path.join("decision.lock"), false)?; - match flock(&decision, libc::LOCK_EX | libc::LOCK_NB) { + match flock(&decision, FlockOperation::NonBlockingLockExclusive) { Ok(()) => {} Err(error) if error.kind() == ErrorKind::WouldBlock => { unlock(&caller_lease)?; @@ -390,7 +395,7 @@ impl CommandOwner { } let decision_lock = open_lock(&path.join("decision.lock"), false)?; let deadline = BootClock::now()?.checked_add(Duration::from_millis(200))?; - if !try_lock_until(&decision_lock, libc::LOCK_EX, deadline)? { + if !try_lock_until(&decision_lock, FlockOperation::LockExclusive, deadline)? { return Ok(None); } let mut control = read_control(&path)?; @@ -440,11 +445,11 @@ impl CommandOwner { for entry in read_dir_bounded(&controls_dir(&self.root), MAX_DIRECTORY_ENTRIES_PER_SCAN)? { let path = entry.path(); let lease = open_lock(&path.join("caller.lease"), false)?; - if flock(&lease, libc::LOCK_EX | libc::LOCK_NB).is_err() { + if flock(&lease, FlockOperation::NonBlockingLockExclusive).is_err() { continue; } let decision = open_lock(&path.join("decision.lock"), false)?; - if flock(&decision, libc::LOCK_EX | libc::LOCK_NB).is_err() { + if flock(&decision, FlockOperation::NonBlockingLockExclusive).is_err() { unlock(&lease)?; continue; } diff --git a/src/daemon/protocol_v2/command/recovery.rs b/src/daemon/protocol_v2/command/recovery.rs index d5639540..4a6f8524 100644 --- a/src/daemon/protocol_v2/command/recovery.rs +++ b/src/daemon/protocol_v2/command/recovery.rs @@ -3,6 +3,7 @@ use std::path::Path; use std::time::Duration; use anyhow::{Result, anyhow, bail}; +use rustix::fs::FlockOperation; use super::super::BootClock; use super::super::wire::{ @@ -32,7 +33,7 @@ pub(super) fn recover_previous_generation(root: &Path, daemon_token: &str) -> Re continue; }; let deadline = BootClock::now()?.checked_add(Duration::from_millis(200))?; - lock_until(&decision_lock, libc::LOCK_EX, deadline)?; + lock_until(&decision_lock, FlockOperation::LockExclusive, deadline)?; let Ok(mut control) = read_control(&path) else { unlock(&decision_lock)?; quarantine_entry(root, &path, QuarantineKind::Control)?; diff --git a/src/daemon/protocol_v2/command/staging.rs b/src/daemon/protocol_v2/command/staging.rs index 5b6cba77..48449536 100644 --- a/src/daemon/protocol_v2/command/staging.rs +++ b/src/daemon/protocol_v2/command/staging.rs @@ -3,6 +3,7 @@ use std::path::Path; use std::time::Duration; use anyhow::Result; +use rustix::fs::FlockOperation; use super::super::{BootClock, BootIdentity}; use super::MAX_COMMAND_STAGING_DIRECTORIES; @@ -24,7 +25,7 @@ pub(super) fn recover_staging(root: &Path) -> Result<()> { let _ = fs::remove_dir_all(path); continue; }; - if flock(&lease, libc::LOCK_EX | libc::LOCK_NB).is_ok() { + if flock(&lease, FlockOperation::NonBlockingLockExclusive).is_ok() { fs::remove_dir_all(path)?; } } diff --git a/src/daemon/protocol_v2/command/tests.rs b/src/daemon/protocol_v2/command/tests.rs index fdf5d264..4418c3bb 100644 --- a/src/daemon/protocol_v2/command/tests.rs +++ b/src/daemon/protocol_v2/command/tests.rs @@ -1,6 +1,8 @@ use std::env; use std::time::Duration; +use rustix::fs::FlockOperation; + use crate::{test_env, test_temp}; use super::super::BootClock; @@ -208,7 +210,7 @@ fn contended_published_command_claim_is_deferred_and_retried() { &token, ) .unwrap(); - flock(&client.decision_lock, libc::LOCK_EX).unwrap(); + flock(&client.decision_lock, FlockOperation::LockExclusive).unwrap(); assert!(owner.claim_next().unwrap().is_none()); let now = BootClock::now().unwrap(); @@ -252,7 +254,7 @@ fn contended_ref_less_recovery_is_deferred_and_retried() { &control.identity, )) .unwrap(); - flock(&client.decision_lock, libc::LOCK_EX).unwrap(); + flock(&client.decision_lock, FlockOperation::LockExclusive).unwrap(); assert!(owner.claim_next().unwrap().is_none()); unlock(&client.decision_lock).unwrap(); @@ -287,7 +289,7 @@ fn terminal_collection_defers_while_admission_is_busy() { drop(client); let admission = open_lock(&owner.root.join("admission.lock"), false).unwrap(); - flock(&admission, libc::LOCK_EX).unwrap(); + flock(&admission, FlockOperation::LockExclusive).unwrap(); assert_eq!(owner.collect_terminal().unwrap(), 0); let now = BootClock::now().unwrap(); let retry = owner @@ -715,20 +717,20 @@ fn lock_deadline_is_reported_without_parsing_error_text() { let path = root.join("structured-timeout.lock"); let held = open_lock(&path, true).unwrap(); let contender = open_lock(&path, false).unwrap(); - flock(&held, libc::LOCK_EX).unwrap(); + flock(&held, FlockOperation::LockExclusive).unwrap(); let deadline = BootClock::now() .unwrap() .checked_add(Duration::from_millis(5)) .unwrap(); - assert!(!try_lock_until(&contender, libc::LOCK_EX, deadline).unwrap()); + assert!(!try_lock_until(&contender, FlockOperation::LockExclusive, deadline).unwrap()); unlock(&held).unwrap(); let deadline = BootClock::now() .unwrap() .checked_add(Duration::from_millis(5)) .unwrap(); - assert!(try_lock_until(&contender, libc::LOCK_EX, deadline).unwrap()); + assert!(try_lock_until(&contender, FlockOperation::LockExclusive, deadline).unwrap()); unlock(&contender).unwrap(); }); } diff --git a/src/durable_io/lock.rs b/src/durable_io/lock.rs index f8dab3f7..a236e4fd 100644 --- a/src/durable_io/lock.rs +++ b/src/durable_io/lock.rs @@ -4,19 +4,11 @@ use std::fs::File; use std::io; #[cfg(unix)] -use std::os::unix::io::AsRawFd; +use rustix::fs::{self as rustix_fs, FlockOperation}; #[cfg(unix)] -fn flock(file: &File, op: libc::c_int) -> io::Result<()> { - let fd = file.as_raw_fd(); - // SAFETY: `fd` stays open for the borrowed `file`, and flock takes no - // pointers or ownership of the descriptor. - let result = unsafe { libc::flock(fd, op) }; - if result == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()) - } +fn flock(file: &File, operation: FlockOperation) -> io::Result<()> { + rustix_fs::flock(file, operation).map_err(io::Error::from) } #[cfg(not(unix))] @@ -30,7 +22,7 @@ fn flock(_file: &File, _op: i32) -> io::Result<()> { pub fn lock_shared(file: &File) -> io::Result<()> { #[cfg(unix)] { - flock(file, libc::LOCK_SH) + flock(file, FlockOperation::LockShared) } #[cfg(not(unix))] { @@ -41,7 +33,7 @@ pub fn lock_shared(file: &File) -> io::Result<()> { pub fn lock_exclusive(file: &File) -> io::Result<()> { #[cfg(unix)] { - flock(file, libc::LOCK_EX) + flock(file, FlockOperation::LockExclusive) } #[cfg(not(unix))] { @@ -52,7 +44,7 @@ pub fn lock_exclusive(file: &File) -> io::Result<()> { pub fn try_lock_exclusive(file: &File) -> io::Result<()> { #[cfg(unix)] { - flock(file, libc::LOCK_EX | libc::LOCK_NB) + flock(file, FlockOperation::NonBlockingLockExclusive) } #[cfg(not(unix))] { @@ -63,7 +55,7 @@ pub fn try_lock_exclusive(file: &File) -> io::Result<()> { pub fn unlock(file: &File) -> io::Result<()> { #[cfg(unix)] { - flock(file, libc::LOCK_UN) + flock(file, FlockOperation::Unlock) } #[cfg(not(unix))] { diff --git a/tools/csharp/Commands/ChecksCommand.cs b/tools/csharp/Commands/ChecksCommand.cs index f897b69b..e979081f 100644 --- a/tools/csharp/Commands/ChecksCommand.cs +++ b/tools/csharp/Commands/ChecksCommand.cs @@ -340,6 +340,7 @@ internal static class ChecksCommand ["pango"] = ["pango"], ["pangocairo"] = ["pango", "cairo"], ["png"] = [], + ["rustix"] = [], ["schemars"] = [], ["serde"] = [], ["serde_ignored"] = [], From b9e5f8f7fb16218530b50d3e668f2bfdb0226571 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:40:28 +0100 Subject: [PATCH 07/13] refactor: poll and wake through rustix Move the runtime wake eventfd, the Wayland and daemon poll loops, and the input monitor to rustix, removing their unsafe blocks. Timeout rounding, interruption retries, and readiness rules stay the same. The runtime poll seam now passes readiness flags, since rustix does not let tests write them into a poll descriptor. A test helper replaces the repeated poll blocks in the wake tests. --- .../wayland/backend/event_loop/dispatch.rs | 32 +-- src/backend/wayland/backend/helpers.rs | 157 +++++---------- src/backend/wayland/backend/runtime_wake.rs | 187 ++++++++---------- src/backend/wayland/backend/signals.rs | 12 +- src/backend/wayland/input_monitor/monitor.rs | 13 +- .../wayland/input_monitor/monitor/worker.rs | 65 +++--- src/backend/wayland/portal_task.rs | 18 +- src/backend/wayland/runtime_operation.rs | 27 +-- src/backend/wayland/session/persistence.rs | 29 +-- src/daemon/core.rs | 81 +++----- src/toolbar_gtk/bridge.rs | 29 +-- src/toolbar_gtk/runtime.rs | 20 +- 12 files changed, 217 insertions(+), 453 deletions(-) diff --git a/src/backend/wayland/backend/event_loop/dispatch.rs b/src/backend/wayland/backend/event_loop/dispatch.rs index 0d85bb31..0a6edcf2 100644 --- a/src/backend/wayland/backend/event_loop/dispatch.rs +++ b/src/backend/wayland/backend/event_loop/dispatch.rs @@ -64,11 +64,8 @@ pub(super) fn dispatch_events( #[cfg(test)] mod tests { - use std::io; - use std::io::ErrorKind; - use std::os::fd::AsRawFd; use std::path::PathBuf; - use std::time::Instant; + use std::time::{Duration, Instant}; use super::*; use crate::backend::wayland::session::{ @@ -126,27 +123,12 @@ mod tests { } fn wait_for_runtime_wake(runtime_wake: &RuntimeWakeSource) { - let mut pollfd = libc::pollfd { - fd: runtime_wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - loop { - // SAFETY: pollfd and the runtime wake descriptor stay valid for this - // bounded test wait. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }; - if ready > 0 { - assert_ne!(pollfd.revents & libc::POLLIN, 0); - return; - } - if ready == 0 { - panic!("persistence worker did not wake the production route"); - } - let err = io::Error::last_os_error(); - if err.kind() != ErrorKind::Interrupted { - panic!("runtime wake poll failed: {err}"); - } - } + assert!( + runtime_wake + .is_readable_within(Duration::from_secs(1)) + .unwrap(), + "persistence worker did not wake the production route" + ); } #[test] diff --git a/src/backend/wayland/backend/helpers.rs b/src/backend/wayland/backend/helpers.rs index 1f1da97e..1a5f23f2 100644 --- a/src/backend/wayland/backend/helpers.rs +++ b/src/backend/wayland/backend/helpers.rs @@ -3,8 +3,9 @@ use std::io::ErrorKind; use anyhow::{Context, Result}; use log::warn; +use rustix::event::{PollFd, PollFlags}; use std::env; -use std::os::fd::{AsRawFd, RawFd}; +use std::os::fd::BorrowedFd; use std::time::Duration; use wayland_client::EventQueue; use wayland_client::backend::{ReadEventsGuard, WaylandError}; @@ -13,7 +14,8 @@ use super::super::state::WaylandState; #[cfg(test)] use super::runtime_wake::timeout_to_poll_ms; use super::runtime_wake::{ - RuntimeWakeSource, TerminalReadinessPolicy, poll_with_retry, validate_poll_readiness, + RuntimeWakeSource, TerminalReadinessPolicy, poll_for_ms, poll_with_retry, + validate_poll_readiness, }; use crate::capture::CaptureFailureKind; use crate::i18n::tr; @@ -73,13 +75,13 @@ struct RuntimeReadOutcome { } trait PreparedWaylandRead { - fn connection_raw_fd(&self) -> RawFd; + fn connection_fd(&self) -> BorrowedFd<'_>; fn read(self) -> Result; } impl PreparedWaylandRead for ReadEventsGuard { - fn connection_raw_fd(&self) -> RawFd { - self.connection_fd().as_raw_fd() + fn connection_fd(&self) -> BorrowedFd<'_> { + ReadEventsGuard::connection_fd(self) } fn read(self) -> Result { @@ -87,52 +89,28 @@ impl PreparedWaylandRead for ReadEventsGuard { } } +/// Applies the retry and readiness rules to `poll_once`, which reports the +/// Wayland and runtime wake readiness in that order. Both empty means timeout. fn poll_runtime_fds_with( - wayland_fd: RawFd, - wake_fd: RawFd, timeout: Option, - mut poll_once: impl FnMut(&mut [libc::pollfd], i32) -> io::Result, + mut poll_once: impl FnMut(i32) -> io::Result<[PollFlags; 2]>, ) -> io::Result { poll_with_retry(timeout, |timeout_ms| { - let mut pollfds = [ - libc::pollfd { - fd: wayland_fd, - events: libc::POLLIN, - revents: 0, - }, - libc::pollfd { - fd: wake_fd, - events: libc::POLLIN, - revents: 0, - }, - ]; - match poll_once(&mut pollfds, timeout_ms) { - Ok(0) => Ok(None), - Ok(_) => { - let readiness = RuntimePollReadiness { - // A Wayland socket can report final buffered protocol data - // together with HUP/ERR. Read and dispatch it first so the - // compositor's actual disconnect error is preserved. - wayland: validate_poll_readiness( - &pollfds[0], - "Wayland", - TerminalReadinessPolicy::ReadBuffered, - )?, - wake: validate_poll_readiness( - &pollfds[1], - "runtime wake", - TerminalReadinessPolicy::Reject, - )?, - }; - if !readiness.wayland && !readiness.wake { - return Err(io::Error::other( - "runtime poll reported readiness without a readable descriptor", - )); - } - Ok(Some(readiness)) - } - Err(err) => Err(err), + let [wayland, wake] = poll_once(timeout_ms)?; + if wayland.is_empty() && wake.is_empty() { + return Ok(None); } + Ok(Some(RuntimePollReadiness { + // A Wayland socket can report final buffered protocol data + // together with HUP/ERR. Read and dispatch it first so the + // compositor's actual disconnect error is preserved. + wayland: validate_poll_readiness( + wayland, + "Wayland", + TerminalReadinessPolicy::ReadBuffered, + )?, + wake: validate_poll_readiness(wake, "runtime wake", TerminalReadinessPolicy::Reject)?, + })) }) .map(|readiness| { readiness.unwrap_or(RuntimePollReadiness { @@ -143,25 +121,17 @@ fn poll_runtime_fds_with( } fn poll_runtime_fds( - wayland_fd: RawFd, - wake_fd: RawFd, + wayland_fd: BorrowedFd<'_>, + wake_fd: BorrowedFd<'_>, timeout: Option, ) -> io::Result { - poll_runtime_fds_with(wayland_fd, wake_fd, timeout, |pollfds, timeout_ms| { - // SAFETY: pollfds is a live mutable slice for the duration of this call, - // and both descriptors remain borrowed by their runtime owners. - let ready = unsafe { - libc::poll( - pollfds.as_mut_ptr(), - pollfds.len() as libc::nfds_t, - timeout_ms, - ) - }; - if ready < 0 { - Err(io::Error::last_os_error()) - } else { - Ok(ready) - } + poll_runtime_fds_with(timeout, |timeout_ms| { + let mut pollfds = [ + PollFd::from_borrowed_fd(wayland_fd, PollFlags::IN), + PollFd::from_borrowed_fd(wake_fd, PollFlags::IN), + ]; + poll_for_ms(&mut pollfds, timeout_ms)?; + Ok([pollfds[0].revents(), pollfds[1].revents()]) }) } @@ -170,12 +140,8 @@ fn read_events_with_runtime_wake( runtime_wake: &RuntimeWakeSource, timeout: Option, ) -> Result { - let readiness = poll_runtime_fds( - guard.connection_raw_fd(), - runtime_wake.poll_fd().as_raw_fd(), - timeout, - ) - .context("runtime readiness poll failed")?; + let readiness = poll_runtime_fds(guard.connection_fd(), runtime_wake.poll_fd(), timeout) + .context("runtime readiness poll failed")?; let wayland_read = if readiness.wayland { let _ = @@ -331,6 +297,7 @@ mod tests { use crate::set_runtime_session_override; use std::collections::VecDeque; use std::io::Write; + use std::os::fd::AsFd; use std::os::unix::net::UnixStream; use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -372,12 +339,7 @@ mod tests { wake.handle().wake().unwrap(); assert_eq!( - poll_runtime_fds( - wayland_read.as_raw_fd(), - wake.poll_fd().as_raw_fd(), - Some(Duration::ZERO), - ) - .unwrap(), + poll_runtime_fds(wayland_read.as_fd(), wake.poll_fd(), Some(Duration::ZERO),).unwrap(), RuntimePollReadiness { wayland: false, wake: true, @@ -392,12 +354,7 @@ mod tests { wayland_write.write_all(&[1]).unwrap(); assert_eq!( - poll_runtime_fds( - wayland_read.as_raw_fd(), - wake.poll_fd().as_raw_fd(), - Some(Duration::ZERO), - ) - .unwrap(), + poll_runtime_fds(wayland_read.as_fd(), wake.poll_fd(), Some(Duration::ZERO),).unwrap(), RuntimePollReadiness { wayland: true, wake: false, @@ -413,12 +370,7 @@ mod tests { drop(wayland_write); assert_eq!( - poll_runtime_fds( - wayland_read.as_raw_fd(), - wake.poll_fd().as_raw_fd(), - Some(Duration::ZERO), - ) - .unwrap(), + poll_runtime_fds(wayland_read.as_fd(), wake.poll_fd(), Some(Duration::ZERO),).unwrap(), RuntimePollReadiness { wayland: true, wake: false, @@ -434,8 +386,8 @@ mod tests { assert_eq!( poll_runtime_fds( - wayland_read.as_raw_fd(), - wake.poll_fd().as_raw_fd(), + wayland_read.as_fd(), + wake.poll_fd(), Some(Duration::from_secs(30)), ) .unwrap(), @@ -454,12 +406,7 @@ mod tests { wake.handle().wake().unwrap(); assert_eq!( - poll_runtime_fds( - wayland_read.as_raw_fd(), - wake.poll_fd().as_raw_fd(), - Some(Duration::ZERO), - ) - .unwrap(), + poll_runtime_fds(wayland_read.as_fd(), wake.poll_fd(), Some(Duration::ZERO),).unwrap(), RuntimePollReadiness { wayland: true, wake: true, @@ -475,8 +422,8 @@ mod tests { } impl PreparedWaylandRead for FakePreparedWaylandRead { - fn connection_raw_fd(&self) -> RawFd { - self.stream.as_raw_fd() + fn connection_fd(&self) -> BorrowedFd<'_> { + self.stream.as_fd() } fn read(mut self) -> Result { @@ -684,13 +631,12 @@ mod tests { #[test] fn runtime_poll_retries_interruption() { let mut calls = 0; - let readiness = poll_runtime_fds_with(10, 11, None, |pollfds, _| { + let readiness = poll_runtime_fds_with(None, |_| { calls += 1; if calls == 1 { return Err(io::Error::from(ErrorKind::Interrupted)); } - pollfds[1].revents = libc::POLLIN; - Ok(1) + Ok([PollFlags::empty(), PollFlags::IN]) }) .unwrap(); @@ -706,11 +652,8 @@ mod tests { #[test] fn runtime_poll_rejects_invalid_descriptor_readiness() { - let err = poll_runtime_fds_with(10, 11, None, |pollfds, _| { - pollfds[1].revents = libc::POLLNVAL; - Ok(1) - }) - .unwrap_err(); + let err = + poll_runtime_fds_with(None, |_| Ok([PollFlags::empty(), PollFlags::NVAL])).unwrap_err(); assert!(err.to_string().contains("runtime wake")); assert!(err.to_string().contains("readiness")); @@ -718,9 +661,9 @@ mod tests { #[test] fn runtime_poll_timeout_reports_no_readiness() { - let readiness = poll_runtime_fds_with(10, 11, Some(Duration::ZERO), |_, timeout_ms| { + let readiness = poll_runtime_fds_with(Some(Duration::ZERO), |timeout_ms| { assert_eq!(timeout_ms, 0); - Ok(0) + Ok([PollFlags::empty(); 2]) }) .unwrap(); diff --git a/src/backend/wayland/backend/runtime_wake.rs b/src/backend/wayland/backend/runtime_wake.rs index 4b1ad245..80e4df96 100644 --- a/src/backend/wayland/backend/runtime_wake.rs +++ b/src/backend/wayland/backend/runtime_wake.rs @@ -1,8 +1,11 @@ use std::io; -use std::os::fd::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd}; +use std::os::fd::{AsFd, BorrowedFd, OwnedFd}; use std::sync::Arc; use std::time::{Duration, Instant}; +use rustix::event::{self, EventfdFlags, PollFd, PollFlags, Timespec}; +use rustix::io::{self as rustix_io, Errno}; + #[derive(Debug)] pub(crate) struct RuntimeWakeSource { fd: Arc, @@ -20,33 +23,43 @@ pub(super) enum TerminalReadinessPolicy { } pub(super) fn validate_poll_readiness( - pollfd: &libc::pollfd, + revents: PollFlags, label: &str, terminal_policy: TerminalReadinessPolicy, ) -> io::Result { - let readable = pollfd.revents & libc::POLLIN != 0; - if pollfd.revents & libc::POLLNVAL != 0 { + let readable = revents.contains(PollFlags::IN); + if revents.contains(PollFlags::NVAL) { return Err(io::Error::other(format!( "{label} poll descriptor failed with readiness {:#x}", - pollfd.revents + revents.bits() ))); } - let terminal = pollfd.revents & (libc::POLLERR | libc::POLLHUP); - if terminal != 0 && !(terminal_policy == TerminalReadinessPolicy::ReadBuffered && readable) { + let terminal = revents.intersects(PollFlags::ERR | PollFlags::HUP); + if terminal && !(terminal_policy == TerminalReadinessPolicy::ReadBuffered && readable) { return Err(io::Error::other(format!( "{label} poll descriptor failed with readiness {:#x}", - pollfd.revents + revents.bits() ))); } - if pollfd.revents != 0 && !readable { + if !revents.is_empty() && !readable { return Err(io::Error::other(format!( "{label} poll descriptor returned unexpected readiness {:#x}", - pollfd.revents + revents.bits() ))); } Ok(readable) } +/// `poll(2)` with a millisecond timeout from [`timeout_to_poll_ms`], where a +/// negative value blocks. +pub(super) fn poll_for_ms(fds: &mut [PollFd<'_>], timeout_ms: i32) -> io::Result { + let timeout = (timeout_ms >= 0).then(|| Timespec { + tv_sec: (timeout_ms / 1_000).into(), + tv_nsec: (timeout_ms % 1_000 * 1_000_000).into(), + }); + Ok(event::poll(fds, timeout.as_ref())?) +} + pub(super) fn poll_with_retry( timeout: Option, mut attempt: impl FnMut(i32) -> io::Result>, @@ -67,15 +80,9 @@ pub(super) fn poll_with_retry( impl RuntimeWakeSource { pub(crate) fn try_new() -> io::Result { - // SAFETY: eventfd returns a new owned descriptor on success. EFD_NONBLOCK - // keeps both producer writes and consumer drains bounded, and EFD_CLOEXEC - // prevents subprocesses from extending the runtime descriptor lifetime. - let raw_fd = unsafe { libc::eventfd(0, libc::EFD_NONBLOCK | libc::EFD_CLOEXEC) }; - if raw_fd < 0 { - return Err(io::Error::last_os_error()); - } - // SAFETY: raw_fd was just returned by eventfd and has not been wrapped. - let fd = unsafe { OwnedFd::from_raw_fd(raw_fd) }; + // NONBLOCK keeps both producer writes and consumer drains bounded, and + // CLOEXEC prevents subprocesses from extending the descriptor lifetime. + let fd = event::eventfd(0, EventfdFlags::NONBLOCK | EventfdFlags::CLOEXEC)?; Ok(Self { fd: Arc::new(fd) }) } @@ -93,32 +100,20 @@ impl RuntimeWakeSource { /// One eventfd read consumes the entire accumulated counter, so a second /// successful-path read would only issue a guaranteed-EAGAIN syscall. pub(crate) fn drain(&self) -> io::Result { + let mut value = [0_u8; size_of::()]; loop { - let mut value = 0_u64; - // SAFETY: value points to a writable u64 and the owned eventfd remains - // valid for the duration of this read. - let result = unsafe { - libc::read( - self.fd.as_raw_fd(), - (&raw mut value).cast(), - size_of::(), - ) - }; - if result == size_of::() as isize { - return Ok(true); - } - if result < 0 { - let err = io::Error::last_os_error(); - match err.kind() { - io::ErrorKind::Interrupted => continue, - io::ErrorKind::WouldBlock => return Ok(false), - _ => return Err(err), + match rustix_io::read(&self.fd, &mut value) { + Ok(read) if read == value.len() => return Ok(true), + Ok(read) => { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + format!("runtime wake eventfd returned a short read ({read} bytes)"), + )); } + Err(Errno::INTR) => {} + Err(Errno::WOULDBLOCK) => return Ok(false), + Err(errno) => return Err(errno.into()), } - return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, - format!("runtime wake eventfd returned a short read ({result} bytes)"), - )); } } @@ -126,24 +121,15 @@ impl RuntimeWakeSource { #[cfg(test)] pub(crate) fn wait_readable(&self, timeout: Option) -> io::Result { let readable = poll_with_retry(timeout, |timeout_ms| { - let mut pollfd = libc::pollfd { - fd: self.fd.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd is valid and the Arc-owned descriptor remains open - // throughout this bounded or producer-woken wait. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, timeout_ms) }; - if ready == 0 { + let mut pollfds = [PollFd::new(&self.fd, PollFlags::IN)]; + if poll_for_ms(&mut pollfds, timeout_ms)? == 0 { return Ok(None); } - if ready < 0 { - return Err(io::Error::last_os_error()); - } - if !validate_poll_readiness(&pollfd, "runtime wake", TerminalReadinessPolicy::Reject)? { + let revents = pollfds[0].revents(); + if !validate_poll_readiness(revents, "runtime wake", TerminalReadinessPolicy::Reject)? { return Err(io::Error::other(format!( "runtime wake poll reported readiness without a readable descriptor ({:#x})", - pollfd.revents + revents.bits() ))); } Ok(Some(())) @@ -154,6 +140,19 @@ impl RuntimeWakeSource { Ok(false) } } + + /// Reports whether a wake arrives within `timeout`, without draining it. + #[cfg(test)] + pub(crate) fn is_readable_within(&self, timeout: Duration) -> io::Result { + let readable = poll_with_retry(Some(timeout), |timeout_ms| { + let mut pollfds = [PollFd::new(&self.fd, PollFlags::IN)]; + if poll_for_ms(&mut pollfds, timeout_ms)? == 0 { + return Ok(None); + } + Ok(Some(pollfds[0].revents().contains(PollFlags::IN))) + })?; + Ok(readable == Some(true)) + } } pub(super) fn timeout_to_poll_ms(timeout: Option) -> i32 { @@ -169,34 +168,22 @@ pub(super) fn timeout_to_poll_ms(timeout: Option) -> i32 { impl RuntimeWakeHandle { pub(crate) fn wake(&self) -> io::Result<()> { - let value = 1_u64; + let value = 1_u64.to_ne_bytes(); loop { - // SAFETY: value points to a readable u64 and the shared eventfd remains - // valid for the duration of this write. - let result = unsafe { - libc::write( - self.fd.as_raw_fd(), - (&raw const value).cast(), - size_of::(), - ) - }; - if result == size_of::() as isize { - return Ok(()); - } - if result < 0 { - let err = io::Error::last_os_error(); - match err.kind() { - io::ErrorKind::Interrupted => continue, - // A saturated eventfd is already readable, so the wake is - // successfully coalesced rather than lost. - io::ErrorKind::WouldBlock => return Ok(()), - _ => return Err(err), + match rustix_io::write(&self.fd, &value) { + Ok(written) if written == value.len() => return Ok(()), + Ok(written) => { + return Err(io::Error::new( + io::ErrorKind::WriteZero, + format!("runtime wake eventfd returned a short write ({written} bytes)"), + )); } + Err(Errno::INTR) => {} + // A saturated eventfd is already readable, so the wake is + // successfully coalesced rather than lost. + Err(Errno::WOULDBLOCK) => return Ok(()), + Err(errno) => return Err(errno.into()), } - return Err(io::Error::new( - io::ErrorKind::WriteZero, - format!("runtime wake eventfd returned a short write ({result} bytes)"), - )); } } } @@ -207,21 +194,12 @@ mod tests { use std::time::{Duration, Instant}; use std::{fs, thread}; + use rustix::process::Pid; + use rustix::thread as rustix_thread; + use super::*; - fn poll_readable(source: &RuntimeWakeSource, timeout_ms: i32) -> bool { - let mut pollfd = libc::pollfd { - fd: source.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd is valid for the duration of this call. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, timeout_ms) }; - assert!(ready >= 0, "poll failed: {}", io::Error::last_os_error()); - ready > 0 && pollfd.revents & libc::POLLIN != 0 - } - - fn wait_until_task_is_blocked_in_poll(tid: libc::pid_t) { + fn wait_until_task_is_blocked_in_poll(tid: Pid) { let stat_path = format!("/proc/self/task/{tid}/stat"); let deadline = Instant::now() + Duration::from_secs(1); loop { @@ -248,9 +226,9 @@ mod tests { let source = RuntimeWakeSource::try_new().unwrap(); source.handle().wake().unwrap(); - assert!(poll_readable(&source, 0)); + assert!(source.is_readable_within(Duration::ZERO).unwrap()); assert!(source.drain().unwrap()); - assert!(!poll_readable(&source, 0)); + assert!(!source.is_readable_within(Duration::ZERO).unwrap()); } #[test] @@ -259,10 +237,7 @@ mod tests { let handle = source.handle(); let (poll_entry_tx, poll_entry_rx) = mpsc::channel(); let poller = thread::spawn(move || { - // SAFETY: gettid has no preconditions and returns the calling Linux - // thread's id, used only to observe its scheduler state in this test. - let tid = unsafe { libc::syscall(libc::SYS_gettid) as libc::pid_t }; - poll_entry_tx.send(tid).unwrap(); + poll_entry_tx.send(rustix_thread::gettid()).unwrap(); assert!(source.wait_readable(Some(Duration::from_secs(1))).unwrap()); }); @@ -281,7 +256,7 @@ mod tests { } assert!(source.drain().unwrap()); - assert!(!poll_readable(&source, 0)); + assert!(!source.is_readable_within(Duration::ZERO).unwrap()); } #[test] @@ -307,22 +282,18 @@ mod tests { #[test] fn terminal_readiness_policy_distinguishes_streams_from_wake_descriptors() { - let pollfd = libc::pollfd { - fd: 7, - events: libc::POLLIN, - revents: libc::POLLIN | libc::POLLHUP, - }; + let revents = PollFlags::IN | PollFlags::HUP; assert!( validate_poll_readiness( - &pollfd, + revents, "buffered stream", TerminalReadinessPolicy::ReadBuffered, ) .unwrap() ); assert!( - validate_poll_readiness(&pollfd, "runtime wake", TerminalReadinessPolicy::Reject,) + validate_poll_readiness(revents, "runtime wake", TerminalReadinessPolicy::Reject) .is_err() ); } diff --git a/src/backend/wayland/backend/signals.rs b/src/backend/wayland/backend/signals.rs index 637bb00c..4d18e240 100644 --- a/src/backend/wayland/backend/signals.rs +++ b/src/backend/wayland/backend/signals.rs @@ -114,21 +114,13 @@ pub(super) fn setup_signal_handlers( #[cfg(test)] mod tests { - use std::os::fd::AsRawFd; + use std::time::Duration; use super::*; use crate::backend::wayland::RuntimeWakeSource; fn wait_for_wake(source: &RuntimeWakeSource) { - let mut pollfd = libc::pollfd { - fd: source.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: source retains the descriptor throughout this bounded wait. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }; - assert_eq!(ready, 1); - assert_ne!(pollfd.revents & libc::POLLIN, 0); + assert!(source.is_readable_within(Duration::from_secs(1)).unwrap()); source.drain().unwrap(); } diff --git a/src/backend/wayland/input_monitor/monitor.rs b/src/backend/wayland/input_monitor/monitor.rs index 40ec2314..62022db9 100644 --- a/src/backend/wayland/input_monitor/monitor.rs +++ b/src/backend/wayland/input_monitor/monitor.rs @@ -16,7 +16,9 @@ //! device or keymap problem can never panic across the libinput FFI edge. use std::io; -use std::os::fd::{AsRawFd, OwnedFd}; +use std::os::fd::OwnedFd; + +use rustix::io as rustix_io; use std::sync::mpsc::{Receiver, TryRecvError, channel}; use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; @@ -188,15 +190,10 @@ impl InputMonitor { /// A reader wedged inside libinput is left to finish on its own rather than /// blocking overlay shutdown; its descriptors close with the process. fn stop(&mut self) { - let byte = [1_u8]; - // SAFETY: the write end is owned and open for the duration of the call, - // and `byte` is a valid one-byte buffer. - let written = - unsafe { libc::write(self.stop.as_raw_fd(), byte.as_ptr().cast(), byte.len()) }; - if written < 0 { + if let Err(errno) = rustix_io::write(&self.stop, &[1]) { log::warn!( "Failed to signal the system input reader: {}", - io::Error::last_os_error() + io::Error::from(errno) ); } let Some(join) = self.join.take() else { diff --git a/src/backend/wayland/input_monitor/monitor/worker.rs b/src/backend/wayland/input_monitor/monitor/worker.rs index d15741a0..3649399d 100644 --- a/src/backend/wayland/input_monitor/monitor/worker.rs +++ b/src/backend/wayland/input_monitor/monitor/worker.rs @@ -13,9 +13,12 @@ use input::event::pointer::{Axis, ButtonState, PointerScrollEvent}; use input::event::tablet_tool::{TabletToolEvent, TipState}; use input::event::{Event, EventTrait, KeyboardEvent, PointerEvent}; use input::{DeviceCapability, Libinput, LibinputInterface}; +use rustix::event::{self, PollFd, PollFlags, Timespec}; +use rustix::io::Errno; +use rustix::pipe::{self, PipeFlags}; use std::fs::{File, OpenOptions}; use std::io; -use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd}; +use std::os::fd::OwnedFd; use std::os::unix::fs::OpenOptionsExt; use std::path::Path; use std::sync::mpsc::Sender; @@ -28,17 +31,7 @@ use repeat::{KeyboardKeys, RepeatPress, RepeatState}; /// Non-blocking, close-on-exec pipe used only to interrupt the reader's poll. pub(super) fn stop_pipe() -> io::Result<(OwnedFd, OwnedFd)> { - let mut fds = [0 as RawFd; 2]; - // SAFETY: `fds` is a writable two-element array, the shape pipe2(2) expects. - let result = unsafe { libc::pipe2(fds.as_mut_ptr(), libc::O_CLOEXEC) }; - if result < 0 { - return Err(io::Error::last_os_error()); - } - // SAFETY: both descriptors were just returned by pipe2 and are unwrapped. - let read = unsafe { OwnedFd::from_raw_fd(fds[0]) }; - // SAFETY: as above for the write end. - let write = unsafe { OwnedFd::from_raw_fd(fds[1]) }; - Ok((read, write)) + pipe::pipe_with(PipeFlags::CLOEXEC).map_err(io::Error::from) } /// Open evdev nodes for libinput. Unprivileged `open(2)`: it succeeds when the @@ -239,47 +232,33 @@ fn wait_for_input( stop_read: &OwnedFd, timeout: Option, ) -> io::Result { - // A repeat deadline in the past polls with 0 and returns Timeout at once. - let timeout_ms = timeout.map_or(-1, |timeout| { - timeout.as_millis().min(i32::MAX as u128) as i32 - }); + // A repeat deadline in the past polls with zero and returns Timeout at once. + let timeout = timeout + .map(Timespec::try_from) + .transpose() + .map_err(io::Error::other)?; loop { let mut fds = [ - libc::pollfd { - fd: libinput.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }, - libc::pollfd { - fd: stop_read.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }, + PollFd::new(libinput, PollFlags::IN), + PollFd::new(stop_read, PollFlags::IN), ]; - // SAFETY: `fds` is a valid two-element array and both descriptors are - // owned (or borrowed from an owner) for the duration of the call. A - // negative timeout blocks until one becomes ready. - let ready = unsafe { libc::poll(fds.as_mut_ptr(), 2, timeout_ms) }; - if ready < 0 { - let err = io::Error::last_os_error(); - if err.kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(err); + match event::poll(&mut fds, timeout.as_ref()) { + Ok(0) => return Ok(PollOutcome::Timeout), + Ok(_) => {} + Err(Errno::INTR) => continue, + Err(errno) => return Err(errno.into()), } - if ready == 0 { - return Ok(PollOutcome::Timeout); - } - if fds[1].revents != 0 { + if !fds[1].revents().is_empty() { return Ok(PollOutcome::Stop); } - if fds[0].revents & (libc::POLLERR | libc::POLLNVAL | libc::POLLHUP) != 0 { + let revents = fds[0].revents(); + if revents.intersects(PollFlags::ERR | PollFlags::NVAL | PollFlags::HUP) { return Err(io::Error::other(format!( "libinput poll descriptor failed with readiness {:#x}", - fds[0].revents + revents.bits() ))); } - if fds[0].revents & libc::POLLIN != 0 { + if revents.contains(PollFlags::IN) { return Ok(PollOutcome::Ready); } } diff --git a/src/backend/wayland/portal_task.rs b/src/backend/wayland/portal_task.rs index 398018fe..61fd2763 100644 --- a/src/backend/wayland/portal_task.rs +++ b/src/backend/wayland/portal_task.rs @@ -217,7 +217,6 @@ impl Drop for PortalTaskExitGuard { #[cfg(test)] mod tests { use std::future; - use std::os::fd::AsRawFd; use tokio::task; @@ -225,13 +224,7 @@ mod tests { use crate::backend::wayland::RuntimeWakeSource; fn wait_for_wake(wake: &RuntimeWakeSource) { - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: the descriptor and pollfd remain valid during the bounded wait. - assert_eq!(unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }, 1); + assert!(wake.is_readable_within(Duration::from_secs(1)).unwrap()); } #[tokio::test(flavor = "multi_thread")] @@ -264,14 +257,7 @@ mod tests { }); drop(task); task::yield_now().await; - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: the descriptor and pollfd are valid for this non-blocking poll. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, 0) }; - assert_eq!(ready, 0); + assert!(!wake.is_readable_within(Duration::ZERO).unwrap()); } #[tokio::test] diff --git a/src/backend/wayland/runtime_operation.rs b/src/backend/wayland/runtime_operation.rs index 4d4c98cf..71794a1a 100644 --- a/src/backend/wayland/runtime_operation.rs +++ b/src/backend/wayland/runtime_operation.rs @@ -379,12 +379,14 @@ impl Drop for RuntimeOperationExitGuard { #[cfg(test)] mod tests { - use std::os::fd::AsRawFd; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::mpsc; use std::time::{Duration, Instant}; use std::{fs, thread}; + use rustix::process::Pid; + use rustix::thread as rustix_thread; + use super::*; use crate::backend::wayland::RuntimeWakeSource; @@ -396,17 +398,10 @@ mod tests { } fn wait_for_wake(wake: &RuntimeWakeSource) { - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd and the runtime wake descriptor remain valid for the bounded wait. - assert_eq!(unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }, 1); - assert_ne!(pollfd.revents & libc::POLLIN, 0); + assert!(wake.is_readable_within(Duration::from_secs(1)).unwrap()); } - fn wait_until_thread_is_blocked_in_poll(tid: libc::pid_t) { + fn wait_until_thread_is_blocked_in_poll(tid: Pid) { let stat_path = format!("/proc/self/task/{tid}/stat"); let deadline = Instant::now() + Duration::from_secs(1); loop { @@ -516,9 +511,7 @@ mod tests { .unwrap(); let (tid_tx, tid_rx) = mpsc::channel(); let poller = thread::spawn(move || { - // SAFETY: gettid has no preconditions and is used only to observe this test thread. - let tid = unsafe { libc::syscall(libc::SYS_gettid) as libc::pid_t }; - tid_tx.send(tid).unwrap(); + tid_tx.send(rustix_thread::gettid()).unwrap(); wait_for_wake(&wake); }); let tid = tid_rx.recv().unwrap(); @@ -592,13 +585,7 @@ mod tests { ) ); assert!(!controller.is_active()); - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd and the runtime wake descriptor are valid for this non-blocking poll. - assert_eq!(unsafe { libc::poll(&raw mut pollfd, 1, 0) }, 0); + assert!(!wake.is_readable_within(Duration::ZERO).unwrap()); assert_eq!( controller .try_submit(42, "test-after-spawn-failure", || 2) diff --git a/src/backend/wayland/session/persistence.rs b/src/backend/wayland/session/persistence.rs index f26d2ee9..94d9bbc1 100644 --- a/src/backend/wayland/session/persistence.rs +++ b/src/backend/wayland/session/persistence.rs @@ -470,9 +470,7 @@ fn assert_send_static() {} #[cfg(test)] mod tests { - use std::io::ErrorKind; - use std::os::fd::AsRawFd; - use std::{fs, io}; + use std::fs; use crate::test_temp; @@ -503,27 +501,10 @@ mod tests { } fn wait_for_runtime_wake(wake: &RuntimeWakeSource) { - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - loop { - // SAFETY: pollfd and the runtime-owned descriptor remain valid for - // this bounded test wait. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }; - if ready > 0 { - assert_ne!(pollfd.revents & libc::POLLIN, 0); - return; - } - if ready == 0 { - panic!("persistence worker did not wake runtime within one second"); - } - let err = io::Error::last_os_error(); - if err.kind() != ErrorKind::Interrupted { - panic!("runtime wake poll failed: {err}"); - } - } + assert!( + wake.is_readable_within(Duration::from_secs(1)).unwrap(), + "persistence worker did not wake runtime within one second" + ); } #[test] diff --git a/src/daemon/core.rs b/src/daemon/core.rs index 2daa4b31..b1a84568 100644 --- a/src/daemon/core.rs +++ b/src/daemon/core.rs @@ -3,10 +3,12 @@ use std::{io, mem, process, thread}; use anyhow::{Context, Result}; use log::{info, warn}; +use rustix::event::{self, PollFd, PollFlags}; +use rustix::io::Errno; use std::fs; use std::fs::OpenOptions; use std::io::ErrorKind; -use std::os::fd::{AsRawFd, BorrowedFd}; +use std::os::fd::BorrowedFd; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; @@ -922,48 +924,31 @@ fn wait_for_daemon_lifecycle( deadline_source: Option<&BootDeadlineSource>, overlay_exit_fd: Option>, ) -> Result { - let mut pollfds = vec![libc::pollfd { - fd: daemon_wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }]; + let mut pollfds = vec![PollFd::from_borrowed_fd( + daemon_wake.poll_fd(), + PollFlags::IN, + )]; if let Some(watcher) = command_watcher { - pollfds.push(libc::pollfd { - fd: watcher.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }); + pollfds.push(PollFd::from_borrowed_fd(watcher.poll_fd(), PollFlags::IN)); } let command_index = command_watcher.map(|_| 1); let deadline_index = deadline_source.map(|source| { let index = pollfds.len(); - pollfds.push(libc::pollfd { - fd: source.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }); + pollfds.push(PollFd::from_borrowed_fd(source.poll_fd(), PollFlags::IN)); index }); let child_index = overlay_exit_fd.map(|fd| { let index = pollfds.len(); - pollfds.push(libc::pollfd { - fd: fd.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }); + pollfds.push(PollFd::from_borrowed_fd(fd, PollFlags::IN)); index }); loop { - // SAFETY: the descriptor remains owned by `daemon_wake` throughout poll. - let ready = unsafe { - libc::poll( - pollfds.as_mut_ptr(), - pollfds - .len() - .try_into() - .expect("poll descriptor count fits"), - -1, - ) + let ready = match event::poll(&mut pollfds, None) { + Ok(ready) => ready, + Err(Errno::INTR) => continue, + Err(errno) => { + return Err(io::Error::from(errno)).context("daemon lifecycle poll failed"); + } }; if ready == 0 { return Ok(DaemonLifecycleReadiness { @@ -971,36 +956,28 @@ fn wait_for_daemon_lifecycle( deadline: false, }); } - if ready < 0 { - let err = io::Error::last_os_error(); - if err.kind() == ErrorKind::Interrupted { - continue; - } - return Err(err).context("daemon lifecycle poll failed"); - } for pollfd in &pollfds { - let terminal = pollfd.revents & (libc::POLLERR | libc::POLLHUP | libc::POLLNVAL); - if terminal != 0 { + let revents = pollfd.revents(); + if revents.intersects(PollFlags::ERR | PollFlags::HUP | PollFlags::NVAL) { return Err(anyhow::anyhow!( "daemon lifecycle descriptor returned terminal readiness {:#x}", - pollfd.revents + revents.bits() )); } } - let daemon_ready = pollfds[0].revents & libc::POLLIN != 0; - let command_ready = command_index - .and_then(|index| pollfds.get(index)) - .is_some_and(|pollfd| pollfd.revents & libc::POLLIN != 0); - let deadline_ready = deadline_index - .and_then(|index| pollfds.get(index)) - .is_some_and(|pollfd| pollfd.revents & libc::POLLIN != 0); - let child_ready = child_index - .and_then(|index| pollfds.get(index)) - .is_some_and(|pollfd| pollfd.revents & libc::POLLIN != 0); + let is_readable = |index: Option| { + index + .and_then(|index| pollfds.get(index)) + .is_some_and(|pollfd| pollfd.revents().contains(PollFlags::IN)) + }; + let daemon_ready = is_readable(Some(0)); + let command_ready = is_readable(command_index); + let deadline_ready = is_readable(deadline_index); + let child_ready = is_readable(child_index); if !daemon_ready && !command_ready && !deadline_ready && !child_ready { return Err(anyhow::anyhow!( "daemon wake descriptor returned invalid readiness {:#x}", - pollfds[0].revents + pollfds[0].revents().bits() )); } if daemon_ready { diff --git a/src/toolbar_gtk/bridge.rs b/src/toolbar_gtk/bridge.rs index abe214f9..5e40a9b9 100644 --- a/src/toolbar_gtk/bridge.rs +++ b/src/toolbar_gtk/bridge.rs @@ -527,7 +527,8 @@ mod tests { use std::fs; use glib::MainContext; - use std::os::fd::AsRawFd; + use rustix::process::Pid; + use rustix::thread as rustix_thread; use std::sync::mpsc; use super::*; @@ -570,18 +571,10 @@ mod tests { } fn wake_is_readable(source: &RuntimeWakeSource) -> bool { - let mut pollfd = libc::pollfd { - fd: source.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd and the source descriptor are valid for this non-blocking poll. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, 0) }; - assert!(ready >= 0); - ready > 0 && pollfd.revents & libc::POLLIN != 0 + source.is_readable_within(Duration::ZERO).unwrap() } - fn wait_until_thread_is_blocked_in_poll(tid: libc::pid_t) { + fn wait_until_thread_is_blocked_in_poll(tid: Pid) { let stat_path = format!("/proc/self/task/{tid}/stat"); let deadline = Instant::now() + Duration::from_secs(1); loop { @@ -814,18 +807,8 @@ mod tests { let (wake, _health, publisher) = channel(); let (tid_tx, tid_rx) = mpsc::channel(); let poller = thread::spawn(move || { - // SAFETY: gettid has no preconditions and is used only to observe this test thread. - let tid = unsafe { libc::syscall(libc::SYS_gettid) as libc::pid_t }; - tid_tx.send(tid).unwrap(); - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd and the source descriptor remain valid for the bounded wait. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }; - assert_eq!(ready, 1); - assert_ne!(pollfd.revents & libc::POLLIN, 0); + tid_tx.send(rustix_thread::gettid()).unwrap(); + assert!(wake.is_readable_within(Duration::from_secs(1)).unwrap()); }); let tid = tid_rx.recv().unwrap(); diff --git a/src/toolbar_gtk/runtime.rs b/src/toolbar_gtk/runtime.rs index 05bea772..8b703b6b 100644 --- a/src/toolbar_gtk/runtime.rs +++ b/src/toolbar_gtk/runtime.rs @@ -125,8 +125,8 @@ pub(super) fn run( #[cfg(test)] mod tests { use std::cell::Cell; - use std::os::fd::AsRawFd; use std::rc::Rc; + use std::time::Duration; use super::*; use crate::backend::wayland::RuntimeWakeSource; @@ -140,16 +140,8 @@ mod tests { clean: Cell::new(false), }); - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd and the source descriptor are valid for this non-blocking poll. - let ready = unsafe { libc::poll(&raw mut pollfd, 1, 0) }; - assert_eq!(ready, 1); + assert!(wake.is_readable_within(Duration::ZERO).unwrap()); assert!(health.failed()); - assert_ne!(pollfd.revents & libc::POLLIN, 0); } #[test] @@ -162,13 +154,7 @@ mod tests { }); assert!(!health.failed()); - let mut pollfd = libc::pollfd { - fd: wake.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd and the source descriptor are valid for this non-blocking poll. - assert_eq!(unsafe { libc::poll(&raw mut pollfd, 1, 0) }, 0); + assert!(!wake.is_readable_within(Duration::ZERO).unwrap()); } #[test] From f108e45964fe96a23e880707b3f90d5356a5beb2 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:45:07 +0100 Subject: [PATCH 08/13] refactor: use rustix for anchored file operations Move the no-replace renames, the pinned runtime state paths, and the input node access check to rustix, removing their unsafe blocks. The paths keep their existing NUL checks and error messages. Share one safe FIFO helper across the tests that repeated an unsafe mkfifo block, since the std version is still unstable. --- src/backend/wayland/clipboard/file_list.rs | 12 +- src/backend/wayland/input_monitor/probe.rs | 23 ++- .../implementation/tests/persistence.rs | 5 +- .../wayland/state/toolbar/events/presets.rs | 5 +- .../state/toolbar/events/quick_colors.rs | 5 +- src/durable_io.rs | 28 ++-- src/i18n/tests.rs | 7 +- src/runtime_ui_state/store/fs.rs | 132 +++++++----------- src/runtime_ui_state/store/tests.rs | 6 +- src/session/artifacts/move_file.rs | 27 ++-- src/session/options/tests.rs | 22 +-- src/session/snapshot/generation/tests.rs | 4 +- src/session/snapshot/tests.rs | 22 +-- src/session/storage/tests.rs | 23 +-- src/test_temp.rs | 18 +++ 15 files changed, 118 insertions(+), 221 deletions(-) diff --git a/src/backend/wayland/clipboard/file_list.rs b/src/backend/wayland/clipboard/file_list.rs index df30b8a2..5920001d 100644 --- a/src/backend/wayland/clipboard/file_list.rs +++ b/src/backend/wayland/clipboard/file_list.rs @@ -205,12 +205,11 @@ fn is_gnome_copied_files_mime(mime_type: &str) -> bool { #[cfg(test)] mod tests { - use std::io; use std::os::unix; use super::*; use crate::backend::wayland::clipboard::image::choose_supported_mime; - use crate::test_temp::TempDir; + use crate::test_temp::{self, TempDir}; use std::fs; #[test] @@ -265,16 +264,9 @@ mod tests { #[test] #[cfg(unix)] fn uri_list_paste_rejects_fifo_without_blocking() { - use std::ffi::CString; - use std::os::unix::ffi::OsStrExt; - let temp = TempDir::new().unwrap(); let fifo_path = temp.path().join("not-an-image"); - let c_path = CString::new(fifo_path.as_os_str().as_bytes()).unwrap(); - // SAFETY: `c_path` is a NUL-terminated string that outlives the call, - // and mkfifo only reads it. - let result = unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }; - assert_eq!(result, 0, "mkfifo failed: {}", io::Error::last_os_error()); + test_temp::make_fifo(&fifo_path); let uri = file_uri_for_path(&fifo_path); let offered = vec![TEXT_URI_LIST_MIME.to_string()]; diff --git a/src/backend/wayland/input_monitor/probe.rs b/src/backend/wayland/input_monitor/probe.rs index 8c4ef7d9..7d536e1c 100644 --- a/src/backend/wayland/input_monitor/probe.rs +++ b/src/backend/wayland/input_monitor/probe.rs @@ -5,14 +5,14 @@ //! silently pick overlay mode and `system` can warn with actionable guidance //! before anything is spawned. -use std::ffi::CString; -use std::os::unix::ffi::OsStrExt; +use rustix::fs::{self as rustix_fs, Access}; +use rustix::io::Errno; use std::path::Path; use crate::env_vars; use std::ffi::OsString; use std::io::ErrorKind; -use std::{env, fs, io}; +use std::{env, fs}; const DEV_INPUT_DIR: &str = "/dev/input"; /// Seat to follow when the session does not name one. libinput's udev backend @@ -211,18 +211,11 @@ fn classify_seat_nodes(seat: &str) -> EventNodeAccess { } fn node_path_access(path: &Path) -> NodePathAccess { - let Ok(c_path) = CString::new(path.as_os_str().as_bytes()) else { - return NodePathAccess::Unknown; - }; - // SAFETY: `c_path` is a valid NUL-terminated C string that outlives the - // call, and `access(2)` only reads it. - if unsafe { libc::access(c_path.as_ptr(), libc::R_OK) } == 0 { - return NodePathAccess::Readable; - } - match io::Error::last_os_error().raw_os_error() { - Some(libc::EACCES | libc::EPERM) => NodePathAccess::PermissionDenied, - Some(libc::ENOENT | libc::ENOTDIR | libc::ENODEV) => NodePathAccess::Absent, - _ => NodePathAccess::Unknown, + match rustix_fs::access(path, Access::READ_OK) { + Ok(()) => NodePathAccess::Readable, + Err(Errno::ACCESS | Errno::PERM) => NodePathAccess::PermissionDenied, + Err(Errno::NOENT | Errno::NOTDIR | Errno::NODEV) => NodePathAccess::Absent, + Err(_) => NodePathAccess::Unknown, } } diff --git a/src/backend/wayland/state/keybindings/implementation/tests/persistence.rs b/src/backend/wayland/state/keybindings/implementation/tests/persistence.rs index 63db8a1f..fcf75a95 100644 --- a/src/backend/wayland/state/keybindings/implementation/tests/persistence.rs +++ b/src/backend/wayland/state/keybindings/implementation/tests/persistence.rs @@ -1,3 +1,5 @@ +use rustix::process as rustix_process; + use crate::config::ConfigDocument; use crate::{config, test_temp}; @@ -387,8 +389,7 @@ fn a_read_only_config_fails_the_write_and_leaves_the_file_alone() { fs::set_permissions(temp.path(), fs::Permissions::from_mode(0o755)).expect("restore dir"); fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).expect("restore file"); - // SAFETY: geteuid has no preconditions and cannot fail. - if unsafe { libc::geteuid() } == 0 { + if rustix_process::geteuid().is_root() { // Root ignores these modes; the scenario cannot be staged. return; } diff --git a/src/backend/wayland/state/toolbar/events/presets.rs b/src/backend/wayland/state/toolbar/events/presets.rs index 19324ed4..1ce43d72 100644 --- a/src/backend/wayland/state/toolbar/events/presets.rs +++ b/src/backend/wayland/state/toolbar/events/presets.rs @@ -136,6 +136,8 @@ impl WaylandState { mod tests { use std::path::{Path, PathBuf}; + use rustix::process as rustix_process; + use crate::config::ConfigDocument; use crate::input::Tool; use crate::{config, test_temp}; @@ -367,8 +369,7 @@ redo = [\"Ctrl+Alt+Shift+Q\"] fs::set_permissions(temp.path(), fs::Permissions::from_mode(0o755)).expect("restore dir"); fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).expect("restore file"); - // SAFETY: geteuid has no preconditions and cannot fail. - if unsafe { libc::geteuid() } == 0 { + if rustix_process::geteuid().is_root() { return; } assert!(result.is_err(), "a read-only config must fail the write"); diff --git a/src/backend/wayland/state/toolbar/events/quick_colors.rs b/src/backend/wayland/state/toolbar/events/quick_colors.rs index 6fb19cbc..01f4e359 100644 --- a/src/backend/wayland/state/toolbar/events/quick_colors.rs +++ b/src/backend/wayland/state/toolbar/events/quick_colors.rs @@ -122,6 +122,8 @@ impl WaylandState { mod tests { use std::path::{Path, PathBuf}; + use rustix::process as rustix_process; + use crate::config::{ColorSpec, ConfigDocument}; use crate::{config, test_temp}; @@ -343,8 +345,7 @@ color = \"#33F54F\" fs::set_permissions(temp.path(), fs::Permissions::from_mode(0o755)).expect("restore dir"); fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).expect("restore file"); - // SAFETY: geteuid has no preconditions and cannot fail. - if unsafe { libc::geteuid() } == 0 { + if rustix_process::geteuid().is_root() { return; } assert!(result.is_err(), "a read-only config must fail the write"); diff --git a/src/durable_io.rs b/src/durable_io.rs index c6c6a5c7..e611a20c 100644 --- a/src/durable_io.rs +++ b/src/durable_io.rs @@ -8,6 +8,9 @@ use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{SystemTime, UNIX_EPOCH}; +#[cfg(target_os = "linux")] +use rustix::fs::{self as rustix_fs, RenameFlags}; + use std::ffi::{CString, OsStr, OsString}; #[cfg(unix)] use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; @@ -594,23 +597,14 @@ fn metadata_mode(_metadata: &fs::Metadata) -> Option { pub fn rename_no_replace(source: &Path, target: &Path) -> io::Result<()> { let source = path_to_cstring(source)?; let target = path_to_cstring(target)?; - // SAFETY: The C strings are valid, NUL-terminated paths. AT_FDCWD makes both - // paths relative to the process cwd, matching std::fs::rename path semantics. - let result = unsafe { - libc::syscall( - libc::SYS_renameat2, - libc::AT_FDCWD, - source.as_ptr(), - libc::AT_FDCWD, - target.as_ptr(), - libc::RENAME_NOREPLACE, - ) - }; - if result == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()) - } + rustix_fs::renameat_with( + rustix_fs::CWD, + &source, + rustix_fs::CWD, + &target, + RenameFlags::NOREPLACE, + ) + .map_err(io::Error::from) } #[cfg(target_os = "linux")] diff --git a/src/i18n/tests.rs b/src/i18n/tests.rs index 3b11ce0a..40f80b98 100644 --- a/src/i18n/tests.rs +++ b/src/i18n/tests.rs @@ -1,11 +1,10 @@ use std::collections::BTreeMap; -use std::ffi::CString; use std::os::unix; use std::sync::mpsc; use std::time::Duration; use std::{fs, thread}; -use crate::test_env; +use crate::{test_env, test_temp}; use super::*; @@ -314,9 +313,7 @@ fn a_symlinked_override_is_refused() { fn a_fifo_override_is_refused_without_waiting_for_a_writer() { let directory = tempfile::tempdir().expect("override directory"); let path = directory.path().join("zh-CN.toml"); - let raw_path = CString::new(path.as_os_str().as_encoded_bytes()).expect("path"); - // SAFETY: `raw_path` is a valid NUL-terminated path for the call. - assert_eq!(unsafe { libc::mkfifo(raw_path.as_ptr(), 0o600) }, 0); + test_temp::make_fifo(&path); let (sender, receiver) = mpsc::channel(); thread::spawn(move || { diff --git a/src/runtime_ui_state/store/fs.rs b/src/runtime_ui_state/store/fs.rs index ff9749cc..f44fa043 100644 --- a/src/runtime_ui_state/store/fs.rs +++ b/src/runtime_ui_state/store/fs.rs @@ -1,8 +1,7 @@ use std::ffi::{CString, OsStr, OsString}; use std::fs::{self, File, OpenOptions}; use std::io::{self, Write}; -use std::mem::MaybeUninit; -use std::os::fd::{AsRawFd, FromRawFd}; +use std::os::fd::{AsFd, AsRawFd, BorrowedFd}; use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; use std::path::PathBuf; @@ -10,6 +9,11 @@ use std::process; use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; +#[cfg(target_os = "linux")] +use rustix::fs::RenameFlags; +use rustix::fs::{self as rustix_fs, AtFlags, Mode, OFlags}; +use rustix::io::Errno; + use crate::runtime_ui_state::{RuntimeStateFileIdentity, RuntimeStateResolvedParent}; static NEXT_SIBLING_ID: AtomicU64 = AtomicU64::new(1); @@ -62,8 +66,8 @@ impl PinnedParent { }) } - fn fd(&self) -> libc::c_int { - self.inner.file.as_raw_fd() + fn fd(&self) -> BorrowedFd<'_> { + self.inner.file.as_fd() } fn sync(&self) -> io::Result<()> { @@ -77,7 +81,7 @@ impl PinnedParent { return Ok(self.inner.observed_path.clone()); } - let proc_path = PathBuf::from(format!("/proc/self/fd/{}", self.fd())); + let proc_path = PathBuf::from(format!("/proc/self/fd/{}", self.fd().as_raw_fd())); let current = fs::read_link(proc_path)?; if !fs::metadata(¤t) .is_ok_and(|metadata| file_identity(&metadata) == self.inner.identity) @@ -100,15 +104,13 @@ pub(super) struct PinnedPath { impl PinnedPath { pub(super) fn open_read(&self) -> io::Result { let leaf = os_str_c_string(&self.leaf)?; - // SAFETY: the parent fd and CString remain valid for the duration of the call. - let fd = unsafe { - libc::openat( - self.parent.fd(), - leaf.as_ptr(), - libc::O_RDONLY | libc::O_CLOEXEC | libc::O_NOFOLLOW | libc::O_NONBLOCK, - ) - }; - file_from_fd(fd) + let fd = rustix_fs::openat( + self.parent.fd(), + &leaf, + OFlags::RDONLY | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::NONBLOCK, + Mode::empty(), + )?; + Ok(File::from(fd)) } pub(super) fn reported_path(&self) -> io::Result { @@ -119,40 +121,23 @@ impl PinnedPath { self.parent.sync() } - fn create_new(&self, mode: libc::mode_t) -> io::Result { + fn create_new(&self, mode: Mode) -> io::Result { let leaf = os_str_c_string(&self.leaf)?; - // SAFETY: the parent fd and CString remain valid for the duration of the call. - let fd = unsafe { - libc::openat( - self.parent.fd(), - leaf.as_ptr(), - libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL | libc::O_CLOEXEC | libc::O_NOFOLLOW, - mode, - ) - }; - file_from_fd(fd) + let fd = rustix_fs::openat( + self.parent.fd(), + &leaf, + OFlags::WRONLY | OFlags::CREATE | OFlags::EXCL | OFlags::CLOEXEC | OFlags::NOFOLLOW, + mode, + )?; + Ok(File::from(fd)) } fn exists_nofollow(&self) -> io::Result { let leaf = os_str_c_string(&self.leaf)?; - let mut stat = MaybeUninit::::uninit(); - // SAFETY: the pointers are valid and stat points to writable storage. - let result = unsafe { - libc::fstatat( - self.parent.fd(), - leaf.as_ptr(), - stat.as_mut_ptr(), - libc::AT_SYMLINK_NOFOLLOW, - ) - }; - if result == 0 { - return Ok(true); - } - let error = io::Error::last_os_error(); - if error.kind() == io::ErrorKind::NotFound { - Ok(false) - } else { - Err(error) + match rustix_fs::statat(self.parent.fd(), &leaf, AtFlags::SYMLINK_NOFOLLOW) { + Ok(_) => Ok(true), + Err(Errno::NOENT) => Ok(false), + Err(errno) => Err(errno.into()), } } } @@ -184,18 +169,20 @@ impl Drop for CleanupPath { } pub(super) fn create_synced_temp(path: &PinnedPath, bytes: &[u8]) -> io::Result { - let path = create_unique_sibling(path, "tmp", |candidate| match candidate.create_new(0o600) { - Ok(mut file) => { - let result = file.write_all(bytes).and_then(|()| file.sync_all()); - drop(file); - if let Err(error) = result { - let _ = remove_file(candidate); - return Err(error); + let path = create_unique_sibling(path, "tmp", |candidate| { + match candidate.create_new(Mode::RUSR | Mode::WUSR) { + Ok(mut file) => { + let result = file.write_all(bytes).and_then(|()| file.sync_all()); + drop(file); + if let Err(error) = result { + let _ = remove_file(candidate); + return Err(error); + } + Ok(true) } - Ok(true) + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => Ok(false), + Err(error) => Err(error), } - Err(error) if error.kind() == io::ErrorKind::AlreadyExists => Ok(false), - Err(error) => Err(error), })?; Ok(CleanupPath::new(path)) } @@ -236,21 +223,14 @@ pub(super) fn rename_noreplace(source: &PinnedPath, destination: &PinnedPath) -> { let source_leaf = os_str_c_string(&source.leaf)?; let destination_leaf = os_str_c_string(&destination.leaf)?; - // SAFETY: both parent fds and CStrings remain valid for the call. - let result = unsafe { - libc::renameat2( - source.parent.fd(), - source_leaf.as_ptr(), - destination.parent.fd(), - destination_leaf.as_ptr(), - libc::RENAME_NOREPLACE, - ) - }; - if result == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()) - } + rustix_fs::renameat_with( + source.parent.fd(), + &source_leaf, + destination.parent.fd(), + &destination_leaf, + RenameFlags::NOREPLACE, + ) + .map_err(io::Error::from) } #[cfg(not(target_os = "linux"))] { @@ -264,13 +244,7 @@ pub(super) fn rename_noreplace(source: &PinnedPath, destination: &PinnedPath) -> pub(super) fn remove_file(path: &PinnedPath) -> io::Result<()> { let leaf = os_str_c_string(&path.leaf)?; - // SAFETY: the parent fd and CString remain valid for the duration of the call. - let result = unsafe { libc::unlinkat(path.parent.fd(), leaf.as_ptr(), 0) }; - if result == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()) - } + rustix_fs::unlinkat(path.parent.fd(), &leaf, AtFlags::empty()).map_err(io::Error::from) } fn validate_leaf(leaf: &OsStr) -> io::Result<()> { @@ -288,14 +262,6 @@ fn os_str_c_string(value: &OsStr) -> io::Result { .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "path contains a NUL byte")) } -fn file_from_fd(fd: libc::c_int) -> io::Result { - if fd < 0 { - return Err(io::Error::last_os_error()); - } - // SAFETY: openat returned a new owned descriptor on success. - Ok(unsafe { File::from_raw_fd(fd) }) -} - fn file_identity(metadata: &fs::Metadata) -> RuntimeStateFileIdentity { RuntimeStateFileIdentity { device: metadata.dev(), diff --git a/src/runtime_ui_state/store/tests.rs b/src/runtime_ui_state/store/tests.rs index 3bde53fb..785a886a 100644 --- a/src/runtime_ui_state/store/tests.rs +++ b/src/runtime_ui_state/store/tests.rs @@ -1,6 +1,4 @@ -use std::ffi::CString; use std::fs; -use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::{PermissionsExt, symlink}; use std::time::{Duration, Instant}; @@ -257,9 +255,7 @@ fn target_symlink_is_rejected_and_neither_path_is_modified() { fn fifo_is_rejected_without_blocking_for_a_writer() { let temp = test_temp::tempdir().unwrap(); let path = temp.path().join("runtime-ui.toml"); - let c_path = CString::new(path.as_os_str().as_bytes()).unwrap(); - // SAFETY: the CString is NUL-terminated and remains live for the call. - assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, 0); + test_temp::make_fifo(&path); let started = Instant::now(); assert!(RuntimeUiStateStore::new(path).inspect().is_err()); assert!(started.elapsed() < Duration::from_secs(1)); diff --git a/src/session/artifacts/move_file.rs b/src/session/artifacts/move_file.rs index 35d93de2..74bee470 100644 --- a/src/session/artifacts/move_file.rs +++ b/src/session/artifacts/move_file.rs @@ -1,6 +1,8 @@ use std::ffi::CString; use anyhow::{Context, Result, anyhow}; +#[cfg(target_os = "linux")] +use rustix::fs::{self as rustix_fs, RenameFlags}; use std::ffi::{OsStr, OsString}; use std::fs::{self, File}; use std::io; @@ -386,23 +388,14 @@ fn rename_artifacts_with_rollback(artifacts: &[MoveArtifact]) -> Result<()> { pub(crate) fn rename_artifact_no_replace(source: &Path, target: &Path) -> io::Result<()> { let source = path_to_cstring(source)?; let target = path_to_cstring(target)?; - // SAFETY: The C strings are valid, NUL-terminated paths. AT_FDCWD makes both paths - // relative to the current process, matching std::fs::rename path semantics. - let result = unsafe { - libc::syscall( - libc::SYS_renameat2, - libc::AT_FDCWD, - source.as_ptr(), - libc::AT_FDCWD, - target.as_ptr(), - libc::RENAME_NOREPLACE, - ) - }; - if result == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()) - } + rustix_fs::renameat_with( + rustix_fs::CWD, + &source, + rustix_fs::CWD, + &target, + RenameFlags::NOREPLACE, + ) + .map_err(io::Error::from) } #[cfg(target_os = "linux")] diff --git a/src/session/options/tests.rs b/src/session/options/tests.rs index 9ad7d8f9..4ede8366 100644 --- a/src/session/options/tests.rs +++ b/src/session/options/tests.rs @@ -1,11 +1,7 @@ #[cfg(unix)] -use std::ffi::CString; -#[cfg(unix)] -use std::os::unix::ffi::OsStrExt; -#[cfg(unix)] use std::os::unix::fs::{PermissionsExt, symlink}; use std::path::{Path, PathBuf}; -use std::{fs, io, process}; +use std::{fs, process}; use crate::{test_env, test_temp}; @@ -250,7 +246,7 @@ fn named_file_validation_rejects_missing_directory_shaped_path() { fn named_file_validation_rejects_fifo_target() { let temp = test_temp::tempdir().unwrap(); let path = temp.path().join("session-fifo.wayscriber-session"); - make_fifo(&path); + test_temp::make_fifo(&path); for (label, result) in [ ( @@ -394,17 +390,3 @@ fn can_create_probe(parent: &Path) -> bool { Err(_) => false, } } - -#[cfg(unix)] -fn make_fifo(path: &Path) { - let raw_path = CString::new(path.as_os_str().as_bytes()).expect("fifo path has no NUL bytes"); - // SAFETY: raw_path is a valid, NUL-terminated filesystem path for this process. - let result = unsafe { libc::mkfifo(raw_path.as_ptr(), 0o600) }; - assert_eq!( - result, - 0, - "mkfifo {} failed: {}", - path.display(), - io::Error::last_os_error() - ); -} diff --git a/src/session/snapshot/generation/tests.rs b/src/session/snapshot/generation/tests.rs index 483dfd5d..30e945e1 100644 --- a/src/session/snapshot/generation/tests.rs +++ b/src/session/snapshot/generation/tests.rs @@ -13,7 +13,7 @@ use super::*; use crate::session::snapshot::tests::sample_snapshot; use crate::session::snapshot::{load, save}; use crate::session::{CompressionMode, SessionOptions}; -use crate::test_temp::tempdir; +use crate::test_temp::{self, tempdir}; use std::fs::{self, File}; use std::io::{Seek, Write}; use std::path::Path; @@ -427,7 +427,7 @@ fn save_as_and_gzip_preserve_the_allocated_header_generation() { fn fifo_marker_probe_never_blocks_and_oversized_generations_do_not_raise_the_sequence() { let temp = tempdir().unwrap(); let options = options(temp.path()); - super::super::tests::make_fifo(&options.clear_marker_file_path()); + test_temp::make_fifo(&options.clear_marker_file_path()); assert_eq!( ArtifactSetView::probe(&options) .next_generation(UnreadablePolicy::Refuse) diff --git a/src/session/snapshot/tests.rs b/src/session/snapshot/tests.rs index b75ceed5..46ece0f9 100644 --- a/src/session/snapshot/tests.rs +++ b/src/session/snapshot/tests.rs @@ -22,19 +22,15 @@ use crate::draw::frame::{MAX_COMPOUND_DEPTH, ShapeSnapshot, UndoAction}; use crate::draw::{ArrowStyle, Color, FontDescriptor, Frame, Shape}; use crate::input::EraserMode; use crate::session::options::{CompressionMode, SessionOptions}; -use crate::test_temp::tempdir; +use crate::test_temp::{make_fifo, tempdir}; use crate::time_utils::now_rfc3339; -#[cfg(unix)] -use std::ffi::CString; use std::fs::{File, Permissions}; #[cfg(unix)] -use std::os::unix::ffi::OsStrExt; -#[cfg(unix)] use std::os::unix::fs::symlink; use std::path::{Path, PathBuf}; use std::sync::mpsc; use std::time::{Duration, SystemTime}; -use std::{fs, io, thread}; +use std::{fs, thread}; fn sample_frame() -> Frame { let mut frame = Frame::new(); @@ -281,20 +277,6 @@ fn write_contentless_session(path: &Path) { .expect("contentless session write"); } -#[cfg(unix)] -pub(super) fn make_fifo(path: &Path) { - let raw_path = CString::new(path.as_os_str().as_bytes()).expect("fifo path has no NUL bytes"); - // SAFETY: raw_path is a valid, NUL-terminated filesystem path for this process. - let result = unsafe { libc::mkfifo(raw_path.as_ptr(), 0o600) }; - assert_eq!( - result, - 0, - "mkfifo {} failed: {}", - path.display(), - io::Error::last_os_error() - ); -} - pub(super) fn set_modified(path: &Path, modified: SystemTime) { File::options() .write(true) diff --git a/src/session/storage/tests.rs b/src/session/storage/tests.rs index 6a9e4e80..8f81f7ab 100644 --- a/src/session/storage/tests.rs +++ b/src/session/storage/tests.rs @@ -8,14 +8,9 @@ use crate::session::snapshot::{BoardPagesSnapshot, BoardSnapshot}; use crate::session::{ CompressionMode, SessionOptions, SessionSnapshot, ToolStateSnapshot, save_snapshot, }; -#[cfg(unix)] -use std::ffi::CString; -#[cfg(unix)] -use std::os::unix::ffi::OsStrExt; +use std::fs; #[cfg(unix)] use std::os::unix::fs::symlink; -use std::path::Path; -use std::{fs, io}; fn transparent_line_snapshot() -> SessionSnapshot { let mut frame = Frame::new(); @@ -632,7 +627,7 @@ fn inspect_named_file_rejects_special_lock_without_blocking() { save_snapshot(&snapshot, &options).expect("save_snapshot should succeed"); let lock_path = options.lock_file_path(); fs::remove_file(&lock_path).expect("remove named session lock"); - make_fifo(&lock_path); + test_temp::make_fifo(&lock_path); let err = inspect_session(&options).expect_err("special lock should be rejected"); @@ -722,17 +717,3 @@ fn clear_named_file_rejects_symlink_primary_without_removing_artifacts() { "clear must not remove selected sidecars after primary validation fails" ); } - -#[cfg(unix)] -fn make_fifo(path: &Path) { - let raw_path = CString::new(path.as_os_str().as_bytes()).expect("fifo path has no NUL bytes"); - // SAFETY: raw_path is a valid, NUL-terminated filesystem path for this process. - let result = unsafe { libc::mkfifo(raw_path.as_ptr(), 0o600) }; - assert_eq!( - result, - 0, - "mkfifo {} failed: {}", - path.display(), - io::Error::last_os_error() - ); -} diff --git a/src/test_temp.rs b/src/test_temp.rs index 15be825c..6ca085ce 100644 --- a/src/test_temp.rs +++ b/src/test_temp.rs @@ -1 +1,19 @@ +#[cfg(unix)] +use std::path::Path; + +#[cfg(unix)] +use rustix::fs::{self as rustix_fs, FileType, Mode}; pub(crate) use tempfile::{TempDir, tempdir}; + +/// Creates a FIFO that only the current user can open. +#[cfg(unix)] +pub(crate) fn make_fifo(path: &Path) { + rustix_fs::mknodat( + rustix_fs::CWD, + path, + FileType::Fifo, + Mode::RUSR | Mode::WUSR, + 0, + ) + .unwrap_or_else(|errno| panic!("mkfifo {} failed: {errno}", path.display())); +} From c115c907ec9ec0965f1836759aa37e013e23ce6b Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:48:52 +0100 Subject: [PATCH 09/13] refactor(daemon): use rustix for pidfds, timers, and inotify Move pidfd opening and waiting, the boot clock and timerfd, the command queue watcher, and the overlay watchdog to rustix, removing their unsafe blocks. Deadlines, retries, and failure handling stay the same. Three unsafe sites remain for a reason: the zero-signal pidfd check has no rustix equivalent, the inotify parser keeps its buffer across calls, and fail_stop exits without any cleanup. The watch mask drops two bits the kernel ignores, because those events always arrive. --- src/daemon/control/runtime.rs | 19 ++- src/daemon/protocol_v2/child.rs | 39 ++---- src/daemon/protocol_v2/linux.rs | 226 ++++++++++---------------------- 3 files changed, 92 insertions(+), 192 deletions(-) diff --git a/src/daemon/control/runtime.rs b/src/daemon/control/runtime.rs index 01923084..22b6331c 100644 --- a/src/daemon/control/runtime.rs +++ b/src/daemon/control/runtime.rs @@ -6,6 +6,8 @@ use crate::paths::{daemon_lock_file, daemon_pid_file}; use crate::session::try_lock_exclusive; use anyhow::{Context, Result, anyhow}; use log::warn; +#[cfg(unix)] +use rustix::process::{self as rustix_process, Pid, Signal}; use std::fs; use std::fs::{File, OpenOptions}; use std::io::ErrorKind; @@ -136,16 +138,13 @@ pub(super) fn signal_daemon_pid(pid: u32) -> Result<()> { return Err(anyhow!("invalid daemon pid {pid}")); } - // SAFETY: `pid` has been checked to be a positive Unix process id and - // `SIGUSR1` is a valid signal constant. - if unsafe { libc::kill(pid, libc::SIGUSR1) } != 0 { - return Err(anyhow!( - "failed to signal wayscriber daemon {}: {}", - pid, - io::Error::last_os_error() - )); - } - Ok(()) + let target = Pid::from_raw(pid).ok_or_else(|| anyhow!("invalid daemon pid {pid}"))?; + rustix_process::kill_process(target, Signal::USR1).map_err(|errno| { + anyhow!( + "failed to signal wayscriber daemon {pid}: {}", + io::Error::from(errno) + ) + }) } #[cfg(not(unix))] diff --git a/src/daemon/protocol_v2/child.rs b/src/daemon/protocol_v2/child.rs index 092ae279..42b20265 100644 --- a/src/daemon/protocol_v2/child.rs +++ b/src/daemon/protocol_v2/child.rs @@ -1,5 +1,5 @@ use std::io::ErrorKind; -use std::os::fd::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd}; +use std::os::fd::{AsFd, BorrowedFd, FromRawFd, OwnedFd}; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use std::thread::Builder; @@ -7,6 +7,8 @@ use std::time::Duration; use std::{env, fs, io, process, thread}; use anyhow::{Context, Result, anyhow, bail}; +use rustix::event::{self, PollFd, PollFlags}; +use rustix::io::{self as rustix_io, Errno, FdFlags}; use serde::{Deserialize, Serialize}; use crate::durable_io::{AtomicWriteOptions, OverwriteMode, PermissionPolicy, SymlinkPolicy}; @@ -535,11 +537,9 @@ pub(crate) fn start_daemon_watchdog_from_environment() -> Result<()> { // inherited descriptor. The environment marker is removed immediately so // no later exec can reinterpret the descriptor number. let descriptor = unsafe { OwnedFd::from_raw_fd(raw) }; - // SAFETY: descriptor is owned and F_SETFD changes only exec inheritance. - if unsafe { libc::fcntl(descriptor.as_raw_fd(), libc::F_SETFD, libc::FD_CLOEXEC) } != 0 { - return Err(io::Error::last_os_error()) - .context("failed to protect daemon watchdog descriptor"); - } + rustix_io::fcntl_setfd(&descriptor, FdFlags::CLOEXEC) + .map_err(io::Error::from) + .context("failed to protect daemon watchdog descriptor")?; super::linux::validate_pidfd(descriptor.as_fd()) .context("inherited daemon watchdog is not a live pidfd")?; // SAFETY: application startup has not launched runtime worker threads yet; @@ -550,27 +550,12 @@ pub(crate) fn start_daemon_watchdog_from_environment() -> Result<()> { Builder::new() .name("wayscriber-daemon-watchdog".into()) .spawn(move || { - let mut pollfd = libc::pollfd { - fd: descriptor.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - loop { - // SAFETY: pollfd points to one initialized entry and the - // descriptor remains owned by this thread. - let result = unsafe { libc::poll(&raw mut pollfd, 1, -1) }; - if result > 0 { - // SAFETY: loss of the owning daemon is an irrevocable - // fail-stop condition for its internal overlay child. - unsafe { super::linux::fail_stop(70) } - } - if result < 0 && io::Error::last_os_error().kind() == ErrorKind::Interrupted { - continue; - } - // SAFETY: an unusable parent-death channel cannot safely - // preserve overlay ownership. - unsafe { super::linux::fail_stop(70) } - } + let mut pollfds = [PollFd::new(&descriptor, PollFlags::IN)]; + while event::poll(&mut pollfds, None) == Err(Errno::INTR) {} + // SAFETY: readiness means the owning daemon is gone, and any other + // result is an unusable parent-death channel. Neither can safely + // preserve overlay ownership, so this is an irrevocable fail-stop. + unsafe { super::linux::fail_stop(70) } }) .context("failed to start daemon watchdog")?; Ok(()) diff --git a/src/daemon/protocol_v2/linux.rs b/src/daemon/protocol_v2/linux.rs index b2cb536f..d4cc9faf 100644 --- a/src/daemon/protocol_v2/linux.rs +++ b/src/daemon/protocol_v2/linux.rs @@ -1,12 +1,21 @@ use std::ffi::CString; use std::fs::{File, Metadata, OpenOptions, Permissions}; use std::io::{self, Read}; -use std::os::fd::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd}; +use std::os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd}; use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; use std::{fmt, fs, process, ptr}; +use rustix::event::{self, PollFd, PollFlags, Timespec}; +use rustix::fs::inotify::{self, CreateFlags, WatchFlags}; +use rustix::io::{self as rustix_io, Errno}; +use rustix::process::{self as rustix_process, Pid, PidfdFlags}; +use rustix::time::{ + self as rustix_time, DynamicClockId, Itimerspec, TimerfdClockId, TimerfdFlags, + TimerfdTimerFlags, +}; + const BOOT_ID_PATH: &str = "/proc/sys/kernel/random/boot_id"; const TIME_NAMESPACE_PATH: &str = "/proc/self/ns/time"; const PID_NAMESPACE_PATH: &str = "/proc/self/ns/pid"; @@ -171,8 +180,7 @@ pub(crate) fn read_bounded_private_file(path: &Path, cap: usize) -> io::Result bool { - // SAFETY: geteuid has no preconditions and cannot fail. - metadata.uid() == unsafe { libc::geteuid() } && metadata.mode() & 0o077 == 0 + metadata.uid() == rustix_process::geteuid().as_raw() && metadata.mode() & 0o077 == 0 } /// Creates `path` as a directory only this user may use, or makes an existing @@ -188,8 +196,7 @@ pub(crate) fn create_private_directory(path: &Path) -> anyhow::Result<()> { } let metadata = fs::symlink_metadata(path)?; - // SAFETY: geteuid has no preconditions and cannot fail. - if !metadata.is_dir() || metadata.uid() != unsafe { libc::geteuid() } { + if !metadata.is_dir() || metadata.uid() != rustix_process::geteuid().as_raw() { anyhow::bail!("{} is not a no-follow private directory", path.display()); } fs::set_permissions(path, Permissions::from_mode(0o700))?; @@ -362,15 +369,10 @@ pub(crate) fn process_start_ticks(pid: u32) -> io::Result { pub(crate) fn open_pidfd(pid: u32) -> io::Result { let pid = i32::try_from(pid) - .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "pid does not fit pid_t"))?; - // SAFETY: pidfd_open has no pointer arguments. The returned descriptor is - // owned by the caller on success. - let raw = unsafe { libc::syscall(libc::SYS_pidfd_open, pid, 0) }; - if raw < 0 { - return Err(io::Error::last_os_error()); - } - // SAFETY: raw is a new pidfd not wrapped elsewhere. - Ok(unsafe { OwnedFd::from_raw_fd(raw as i32) }) + .ok() + .and_then(Pid::from_raw) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "pid does not fit pid_t"))?; + Ok(rustix_process::pidfd_open(pid, PidfdFlags::empty())?) } /// Blocks until the process behind `fd` exits or `timeout` elapses, returning @@ -386,27 +388,14 @@ pub(crate) fn wait_for_pidfd_exit(fd: BorrowedFd<'_>, timeout: Duration) -> io:: if remaining.is_zero() { return Ok(false); } - let mut poll_fd = libc::pollfd { - fd: fd.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - let millis = i32::try_from(remaining.as_millis()).unwrap_or(i32::MAX); - // SAFETY: one owned pollfd, valid for the call; the descriptor is - // borrowed for at least this long. - let ready = unsafe { libc::poll(&raw mut poll_fd, 1, millis) }; - if ready < 0 { - let error = io::Error::last_os_error(); + let mut pollfds = [PollFd::from_borrowed_fd(fd, PollFlags::IN)]; + let remaining = Timespec::try_from(remaining).map_err(io::Error::other)?; + match event::poll(&mut pollfds, Some(&remaining)) { + Ok(ready) => return Ok(ready != 0), // A signal arriving mid-wait is expected on this thread. - if error.kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(error); + Err(Errno::INTR) => {} + Err(errno) => return Err(errno.into()), } - if ready == 0 { - return Ok(false); - } - return Ok(true); } } @@ -432,15 +421,7 @@ pub(crate) fn validate_pidfd(fd: BorrowedFd<'_>) -> io::Result<()> { impl BootClock { pub(crate) fn now() -> io::Result { - let mut value = libc::timespec { - tv_sec: 0, - tv_nsec: 0, - }; - // SAFETY: value is a valid writable timespec and CLOCK_BOOTTIME has no - // additional preconditions. - if unsafe { libc::clock_gettime(libc::CLOCK_BOOTTIME, &raw mut value) } != 0 { - return Err(io::Error::last_os_error()); - } + let value = rustix_time::clock_gettime_dynamic(DynamicClockId::Boottime)?; if value.tv_sec < 0 || !(0..1_000_000_000).contains(&value.tv_nsec) { return Err(io::Error::new( io::ErrorKind::InvalidData, @@ -481,17 +462,11 @@ impl BootDeadline { impl BootDeadlineSource { pub(crate) fn try_new() -> io::Result { - // SAFETY: timerfd_create returns a new owned descriptor on success. - let raw = unsafe { - libc::timerfd_create(libc::CLOCK_BOOTTIME, libc::TFD_NONBLOCK | libc::TFD_CLOEXEC) - }; - if raw < 0 { - return Err(io::Error::last_os_error()); - } - Ok(Self { - // SAFETY: raw is a newly created descriptor not wrapped elsewhere. - fd: unsafe { OwnedFd::from_raw_fd(raw) }, - }) + let fd = rustix_time::timerfd_create( + TimerfdClockId::Boottime, + TimerfdFlags::NONBLOCK | TimerfdFlags::CLOEXEC, + )?; + Ok(Self { fd }) } pub(crate) fn poll_fd(&self) -> BorrowedFd<'_> { @@ -501,82 +476,45 @@ impl BootDeadlineSource { pub(crate) fn arm(&self, deadline: BootDeadline) -> io::Result<()> { let seconds = deadline.0 / 1_000_000_000; let nanos = deadline.0 % 1_000_000_000; - let value = libc::itimerspec { - it_interval: libc::timespec { - tv_sec: 0, - tv_nsec: 0, - }, - it_value: libc::timespec { - tv_sec: libc::time_t::try_from(seconds).map_err(|_| { + let value = Itimerspec { + it_interval: Timespec::default(), + it_value: Timespec { + tv_sec: i64::try_from(seconds).map_err(|_| { io::Error::new(io::ErrorKind::InvalidInput, "deadline seconds overflow") })?, - tv_nsec: libc::c_long::try_from(nanos).map_err(|_| { + tv_nsec: i64::try_from(nanos).map_err(|_| { io::Error::new(io::ErrorKind::InvalidInput, "deadline nanos overflow") })?, }, }; - // SAFETY: value is a fully initialized itimerspec and fd owns a timerfd. - if unsafe { - libc::timerfd_settime( - self.fd.as_raw_fd(), - libc::TFD_TIMER_ABSTIME, - &raw const value, - ptr::null_mut(), - ) - } != 0 - { - return Err(io::Error::last_os_error()); - } + rustix_time::timerfd_settime(&self.fd, TimerfdTimerFlags::ABSTIME, &value)?; Ok(()) } pub(crate) fn disarm(&self) -> io::Result<()> { - let value = libc::itimerspec { - it_interval: libc::timespec { - tv_sec: 0, - tv_nsec: 0, - }, - it_value: libc::timespec { - tv_sec: 0, - tv_nsec: 0, - }, + let value = Itimerspec { + it_interval: Timespec::default(), + it_value: Timespec::default(), }; - // SAFETY: value is initialized and fd owns a timerfd. - if unsafe { - libc::timerfd_settime(self.fd.as_raw_fd(), 0, &raw const value, ptr::null_mut()) - } != 0 - { - return Err(io::Error::last_os_error()); - } + rustix_time::timerfd_settime(&self.fd, TimerfdTimerFlags::empty(), &value)?; Ok(()) } pub(crate) fn drain(&self) -> io::Result { - let mut expirations = 0_u64; + let mut expirations = [0_u8; size_of::()]; loop { - // SAFETY: expirations is writable and fd remains valid for the read. - let result = unsafe { - libc::read( - self.fd.as_raw_fd(), - (&raw mut expirations).cast(), - size_of::(), - ) - }; - if result == size_of::() as isize { - return Ok(true); + match rustix_io::read(&self.fd, &mut expirations) { + Ok(read) if read == expirations.len() => return Ok(true), + Ok(read) => { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + format!("timerfd returned a short read ({read} bytes)"), + )); + } + Err(Errno::INTR) => {} + Err(Errno::WOULDBLOCK) => return Ok(false), + Err(errno) => return Err(errno.into()), } - if result < 0 { - let error = io::Error::last_os_error(); - return match error.kind() { - io::ErrorKind::Interrupted => continue, - io::ErrorKind::WouldBlock => Ok(false), - _ => Err(error), - }; - } - return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, - format!("timerfd returned a short read ({result} bytes)"), - )); } } } @@ -584,27 +522,15 @@ impl BootDeadlineSource { impl CommandQueueWatcher { pub(crate) fn try_new(queue_path: &Path) -> io::Result { let (queue_fd, queue_identity) = open_nofollow_directory(queue_path)?; - // SAFETY: inotify_init1 returns a newly owned descriptor on success. - let raw = unsafe { libc::inotify_init1(libc::IN_NONBLOCK | libc::IN_CLOEXEC) }; - if raw < 0 { - return Err(io::Error::last_os_error()); - } - // SAFETY: raw is a new descriptor not wrapped elsewhere. - let fd = unsafe { OwnedFd::from_raw_fd(raw) }; + let fd = inotify::init(CreateFlags::NONBLOCK | CreateFlags::CLOEXEC)?; let path = CString::new(queue_path.as_os_str().as_encoded_bytes()) .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "queue path contains NUL"))?; - let mask = libc::IN_MOVED_TO - | libc::IN_CREATE - | libc::IN_DELETE_SELF - | libc::IN_MOVE_SELF - | libc::IN_IGNORED - | libc::IN_Q_OVERFLOW; - // SAFETY: path is NUL-terminated and fd owns an inotify instance. - let watch_descriptor = - unsafe { libc::inotify_add_watch(fd.as_raw_fd(), path.as_ptr(), mask) }; - if watch_descriptor < 0 { - return Err(io::Error::last_os_error()); - } + // IN_IGNORED and IN_Q_OVERFLOW always arrive, so they are not requested here. + let mask = WatchFlags::MOVED_TO + | WatchFlags::CREATE + | WatchFlags::DELETE_SELF + | WatchFlags::MOVE_SELF; + let watch_descriptor = inotify::add_watch(&fd, &path, mask)?; revalidate_path_identity(queue_path, queue_identity)?; Ok(Self { fd, @@ -630,30 +556,20 @@ impl CommandQueueWatcher { self.buffered_length = 0; self.buffered_offset = 0; loop { - // SAFETY: buffer is writable for its full fixed length and fd is - // a nonblocking inotify descriptor. - let read = unsafe { - libc::read( - self.fd.as_raw_fd(), - self.buffer.as_mut_ptr().cast(), - self.buffer.len(), - ) - }; - if read > 0 { - self.buffered_length = read as usize; - return Ok(true); - } - if read == 0 { - return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, - "inotify returned EOF", - )); - } - let error = io::Error::last_os_error(); - match error.kind() { - io::ErrorKind::Interrupted => continue, - io::ErrorKind::WouldBlock => return Ok(false), - _ => return Err(error), + match rustix_io::read(&self.fd, &mut self.buffer[..]) { + Ok(0) => { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "inotify returned EOF", + )); + } + Ok(read) => { + self.buffered_length = read; + return Ok(true); + } + Err(Errno::INTR) => {} + Err(Errno::WOULDBLOCK) => return Ok(false), + Err(errno) => return Err(errno.into()), } } } From 82a8d5e9c68cf3561b6ad8edef9ba602d5487dd1 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:57:33 +0100 Subject: [PATCH 10/13] refactor(broker): use rustix for descriptors and processes Replace the hand-built sendmsg and recvmsg control messages, the sealed memfd handling, and the polling, signalling, and child status calls with typed rustix calls. Broker functions now take borrowed descriptors instead of raw integers, so fd lifetimes are checked by the compiler. The packet and descriptor caps, the truncation checks, and the signal allow-list stay. Passed descriptors now also close when a packet with no data is rejected. One check narrows: rustix skips ancillary types it does not know instead of reporting them, so only descriptors and credentials are visible here, and credentials are still rejected. The skipped case is SCM_SECURITY, which needs SO_PASSSEC on the socket, and nothing here sets it. The post-clone child stub stays on raw syscalls by design, and the broker entry keeps one borrow of its inherited channels, which live until the process exits. --- src/daemon/overlay/lifecycle.rs | 4 +- src/process_broker/bootstrap.rs | 91 +++----- src/process_broker/client.rs | 68 +++--- src/process_broker/execution.rs | 115 ++++----- src/process_broker/server.rs | 220 +++++++----------- src/process_broker/tests.rs | 4 +- src/process_broker/transport.rs | 398 +++++++++++--------------------- src/process_broker/wire.rs | 7 +- 8 files changed, 332 insertions(+), 575 deletions(-) diff --git a/src/daemon/overlay/lifecycle.rs b/src/daemon/overlay/lifecycle.rs index faee91c4..7f673e72 100644 --- a/src/daemon/overlay/lifecycle.rs +++ b/src/daemon/overlay/lifecycle.rs @@ -1,4 +1,4 @@ -use std::os::fd::{AsRawFd, BorrowedFd}; +use std::os::fd::{AsFd, BorrowedFd}; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; @@ -108,7 +108,7 @@ impl OverlayLifecycle { &candidate.program, &launch.arguments, launch.environment, - daemon_watchdog.as_raw_fd(), + daemon_watchdog.as_fd(), )?; self.child.start(child)?; self.child diff --git a/src/process_broker/bootstrap.rs b/src/process_broker/bootstrap.rs index 0368352f..611e8ab5 100644 --- a/src/process_broker/bootstrap.rs +++ b/src/process_broker/bootstrap.rs @@ -1,5 +1,5 @@ use std::io; -use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; +use std::os::fd::{AsRawFd, OwnedFd}; use std::sync::atomic::AtomicBool; use std::sync::{Arc, Mutex}; @@ -17,6 +17,13 @@ use std::thread::Builder; use std::time::{Duration, Instant}; use anyhow::{Context, Result}; +#[cfg(not(test))] +use rustix::io as rustix_io; +use rustix::io::Errno; +use rustix::net::{self, AddressFamily, SocketFlags, SocketType}; +#[cfg(not(test))] +use rustix::process::Signal; +use rustix::process::{self as rustix_process, Pid, WaitOptions}; use crate::daemon::protocol_v2::ProtocolToken; #[cfg(not(test))] @@ -152,9 +159,8 @@ pub(super) fn start() -> Result { }), }; if let Err(error) = broker.request(BrokerOperation::Ping) { - // SAFETY: pid is the raw-clone broker child created above. - unsafe { - libc::kill(pid, libc::SIGKILL); + if let Some(broker_pid) = Pid::from_raw(pid) { + let _ = rustix_process::kill_process(broker_pid, Signal::KILL); } wait_for_broker_process(pid); return Err(error).context("process broker exec/authentication handshake failed"); @@ -164,37 +170,20 @@ pub(super) fn start() -> Result { #[cfg(not(test))] fn duplicate_for_exec(descriptor: &OwnedFd) -> Result { - // SAFETY: F_DUPFD_CLOEXEC duplicates the live descriptor at or above five. - let duplicate = unsafe { libc::fcntl(descriptor.as_raw_fd(), libc::F_DUPFD_CLOEXEC, 5) }; - if duplicate < 0 { - return Err(io::Error::last_os_error()).context("failed to stage broker descriptor"); - } - // SAFETY: fcntl returned a fresh owned descriptor. - Ok(unsafe { OwnedFd::from_raw_fd(duplicate) }) + rustix_io::fcntl_dupfd_cloexec(descriptor, 5) + .map_err(io::Error::from) + .context("failed to stage broker descriptor") } fn socket_pair(label: &str) -> Result<(OwnedFd, OwnedFd)> { - let mut sockets = [0; 2]; - // SAFETY: sockets has room for the returned descriptor pair. - if unsafe { - libc::socketpair( - libc::AF_UNIX, - libc::SOCK_SEQPACKET | libc::SOCK_CLOEXEC, - 0, - sockets.as_mut_ptr(), - ) - } != 0 - { - return Err(io::Error::last_os_error()) - .with_context(|| format!("failed to create {label} socketpair")); - } - // SAFETY: socketpair returned two new descriptors. - Ok(unsafe { - ( - OwnedFd::from_raw_fd(sockets[0]), - OwnedFd::from_raw_fd(sockets[1]), - ) - }) + net::socketpair( + AddressFamily::UNIX, + SocketType::SEQPACKET, + SocketFlags::CLOEXEC, + None, + ) + .map_err(io::Error::from) + .with_context(|| format!("failed to create {label} socketpair")) } /// Whether the broker process exits within `grace`, reaping it if it does. @@ -203,20 +192,17 @@ pub(super) fn broker_exits_within(child_pid: libc::pid_t, grace: Duration) -> bo return true; } + let Some(pid) = Pid::from_raw(child_pid) else { + return true; + }; let deadline = Instant::now() + grace; - let mut status = 0; loop { - // SAFETY: child_pid names the raw-clone broker child owned by its guard. - let result = unsafe { libc::waitpid(child_pid, &raw mut status, libc::WNOHANG) }; - if result == child_pid { - return true; - } - if result < 0 { - if io::Error::last_os_error().kind() == io::ErrorKind::Interrupted { - continue; - } + match rustix_process::waitpid(Some(pid), WaitOptions::NOHANG) { + Ok(Some(_)) => return true, + Ok(None) => {} + Err(Errno::INTR) => continue, // Already reaped: nothing is left to wait for. - return true; + Err(_) => return true, } if Instant::now() >= deadline { return false; @@ -229,16 +215,11 @@ pub(super) fn wait_for_broker_process(child_pid: libc::pid_t) { if child_pid <= 0 { return; } - let mut status = 0; - loop { - // SAFETY: child_pid names the raw-clone broker child owned by its guard. - let result = unsafe { libc::waitpid(child_pid, &raw mut status, 0) }; - if result == child_pid { - break; - } - if result < 0 && io::Error::last_os_error().kind() == io::ErrorKind::Interrupted { - continue; - } - break; - } + let Some(pid) = Pid::from_raw(child_pid) else { + return; + }; + while matches!( + rustix_process::waitpid(Some(pid), WaitOptions::empty()), + Err(Errno::INTR) + ) {} } diff --git a/src/process_broker/client.rs b/src/process_broker/client.rs index 0cfdd0a8..fbfc0313 100644 --- a/src/process_broker/client.rs +++ b/src/process_broker/client.rs @@ -2,7 +2,7 @@ use std::collections::VecDeque; use std::ffi::{OsStr, OsString}; use std::io; use std::io::ErrorKind; -use std::os::fd::{AsRawFd, OwnedFd, RawFd}; +use std::os::fd::{AsFd, BorrowedFd, OwnedFd}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex, MutexGuard, OnceLock, TryLockError, Weak}; #[cfg(test)] @@ -10,6 +10,9 @@ use std::thread::JoinHandle; use std::time::Duration; use anyhow::{Context, Result, anyhow, bail}; +use rustix::io::Errno; +use rustix::net::{self, SendFlags}; +use rustix::process::{self as rustix_process, Pid, Signal}; use crate::daemon::protocol_v2::ProtocolId; @@ -87,8 +90,8 @@ enum ExchangeWait { /// The transport policy and watchdog a spawn request carries, kept together so /// the private spawn path stays inside the argument-count lint. #[derive(Debug, Clone, Copy)] -struct SpawnOptions { - watchdog: Option, +struct SpawnOptions<'descriptor> { + watchdog: Option>, wait: ExchangeWait, } @@ -119,7 +122,7 @@ pub(crate) fn current() -> Result { impl Drop for ProcessBrokerGuard { fn drop(&mut self) { let was_healthy = self.broker.inner.healthy.swap(false, Ordering::AcqRel); - let asked_to_stop = signal_shutdown(self.broker.inner.shutdown.as_raw_fd()).is_ok(); + let asked_to_stop = signal_shutdown(self.broker.inner.shutdown.as_fd()).is_ok(); // A broker whose exchange failed may be stuck and never read the // shutdown request. Waiting on it without a bound would hang the very // exit that lets the service manager start a fresh daemon. @@ -129,11 +132,11 @@ impl Drop for ProcessBrokerGuard { self.broker.inner.child_pid, UNHEALTHY_BROKER_EXIT_GRACE, )); - if must_kill && self.broker.inner.child_pid > 0 { - // SAFETY: child_pid is the broker process owned by this guard. - unsafe { - libc::kill(self.broker.inner.child_pid, libc::SIGKILL); - } + if must_kill + && self.broker.inner.child_pid > 0 + && let Some(pid) = Pid::from_raw(self.broker.inner.child_pid) + { + let _ = rustix_process::kill_process(pid, Signal::KILL); } #[cfg(test)] if let Some(thread) = self @@ -163,29 +166,20 @@ impl Drop for ProcessBrokerGuard { } } -fn signal_shutdown(descriptor: RawFd) -> io::Result<()> { +fn signal_shutdown(descriptor: BorrowedFd<'_>) -> io::Result<()> { let byte = [GRACEFUL_SHUTDOWN_BYTE]; loop { - // SAFETY: descriptor is the broker shutdown socket and byte is readable. - let written = unsafe { - libc::send( - descriptor, - byte.as_ptr().cast(), - byte.len(), - libc::MSG_NOSIGNAL, - ) - }; - if written == 1 { - return Ok(()); + match net::send(descriptor, &byte, SendFlags::NOSIGNAL) { + Ok(1) => return Ok(()), + Ok(_) => { + return Err(io::Error::new( + ErrorKind::WriteZero, + "short broker shutdown write", + )); + } + Err(Errno::INTR) => {} + Err(errno) => return Err(errno.into()), } - if written < 0 && io::Error::last_os_error().kind() == ErrorKind::Interrupted { - continue; - } - return Err(if written < 0 { - io::Error::last_os_error() - } else { - io::Error::new(ErrorKind::WriteZero, "short broker shutdown write") - }); } } @@ -253,7 +247,7 @@ impl ProcessBroker { fn request_with_descriptors( &self, operation: BrokerOperation, - descriptors: &[RawFd], + descriptors: &[BorrowedFd<'_>], wait: ExchangeWait, ) -> Result<(BrokerOutcome, Vec)> { let request_id = ProtocolId::generate()?.to_string(); @@ -271,9 +265,9 @@ impl ProcessBroker { bail!("process broker transport is no longer usable"); } let exchange = (|| -> Result<(BrokerResponse, Vec)> { - set_socket_timeout(self.inner.socket.as_raw_fd(), exchange_timeout)?; - send_packet(self.inner.socket.as_raw_fd(), &packet, descriptors)?; - let (packet, descriptors) = recv_packet(self.inner.socket.as_raw_fd())?; + set_socket_timeout(self.inner.socket.as_fd(), exchange_timeout)?; + send_packet(self.inner.socket.as_fd(), &packet, descriptors)?; + let (packet, descriptors) = recv_packet(self.inner.socket.as_fd())?; let response: BrokerResponse = serde_json::from_slice(&packet)?; if response.request_id != request_id { bail!("broker response identity mismatch"); @@ -368,7 +362,7 @@ impl ProcessBroker { let (input, input_descriptor) = encode_blob(input, MAX_INPUT_BYTES)?; let request_descriptors = input_descriptor .as_ref() - .map(|descriptor| vec![descriptor.as_raw_fd()]) + .map(|descriptor| vec![descriptor.as_fd()]) .unwrap_or_default(); let (outcome, descriptors) = self.request_with_descriptors( BrokerOperation::Run { @@ -428,7 +422,7 @@ impl ProcessBroker { let (input, input_descriptor) = encode_blob(input, super::manifest::input_cap(kind))?; let request_descriptors = input_descriptor .as_ref() - .map(|descriptor| vec![descriptor.as_raw_fd()]) + .map(|descriptor| vec![descriptor.as_fd()]) .unwrap_or_default(); let (outcome, descriptors) = self.request_with_descriptors( BrokerOperation::Publish { @@ -530,7 +524,7 @@ impl ProcessBroker { program: &OsStr, arguments: I, environment: Vec<(OsString, Option)>, - watchdog: RawFd, + watchdog: BorrowedFd<'_>, ) -> Result where I: IntoIterator, @@ -556,7 +550,7 @@ impl ProcessBroker { program: &OsStr, arguments: I, environment: Vec<(OsString, Option)>, - options: SpawnOptions, + options: SpawnOptions<'_>, ) -> Result where I: IntoIterator, diff --git a/src/process_broker/execution.rs b/src/process_broker/execution.rs index 7700d216..0444ceb6 100644 --- a/src/process_broker/execution.rs +++ b/src/process_broker/execution.rs @@ -1,7 +1,6 @@ use std::collections::BTreeMap; use std::io::{self, Read, Write}; -use std::mem::MaybeUninit; -use std::os::fd::{AsRawFd, RawFd}; +use std::os::fd::{AsFd, BorrowedFd}; use std::os::unix::process::CommandExt; use std::process::{Child, ChildStdin, Command, ExitStatus, Stdio}; use std::sync::Arc; @@ -10,6 +9,10 @@ use std::time::Duration; use std::{mem, thread}; use anyhow::{Context, Result, anyhow}; +use rustix::event::{self, PollFd, PollFlags, Timespec}; +use rustix::fs::{self as rustix_fs, OFlags}; +use rustix::io::Errno; +use rustix::process::{self as rustix_process, Pid, Signal, WaitId, WaitIdOptions}; use crate::daemon::protocol_v2::{BootClock, BootDeadline}; @@ -25,7 +28,10 @@ use super::wire::{HelperKind, MAX_STDERR_BYTES, OutputMode, STDOUT_CAP_EXCEEDED} const PIPE_DRAIN_GRACE: Duration = Duration::from_secs(2); /// How often a pipe thread waiting on an idle pipe checks whether to give up. -const PIPE_POLL_INTERVAL_MS: libc::c_int = 20; +const PIPE_POLL_INTERVAL: Timespec = Timespec { + tv_sec: 0, + tv_nsec: 20_000_000, +}; const PIPE_HELD_OPEN: &str = "broker helper pipe is still open after the helper exited"; @@ -63,12 +69,9 @@ pub(super) fn terminate_owned_children(children: &mut BTreeMap) { } pub(super) fn kill_child_process_group(child: &mut Child) { - let pid = child.id(); - if let Ok(group) = i32::try_from(pid) { - // SAFETY: broker children use a fresh process group equal to their PID. - if unsafe { libc::kill(-group, libc::SIGKILL) } == 0 { - return; - } + // Broker children use a fresh process group equal to their PID. + if rustix_process::kill_process_group(Pid::from_child(child), Signal::KILL).is_ok() { + return; } let _ = child.kill(); } @@ -145,48 +148,28 @@ impl Drop for OwnedProcess { } fn child_status_unreaped(child: &Child) -> io::Result> { - let mut info = MaybeUninit::::zeroed(); // WNOWAIT pins the zombie leader and therefore its process-group identity. - // SAFETY: info is writable and child is owned by this broker. - if unsafe { - libc::waitid( - libc::P_PID, - child.id(), - info.as_mut_ptr(), - libc::WEXITED | libc::WNOHANG | libc::WNOWAIT, - ) - } != 0 - { - return Err(io::Error::last_os_error()); - } - // SAFETY: waitid initialized info after success. - let info = unsafe { info.assume_init() }; - // SAFETY: info was zeroed before the call and waitid only writes a SIGCHLD - // record into it, so the pid field it reads is initialized; it stays zero - // when WNOHANG found no waitable child. - if unsafe { info.si_pid() } == 0 { + let options = WaitIdOptions::EXITED | WaitIdOptions::NOHANG | WaitIdOptions::NOWAIT; + let Some(status) = rustix_process::waitid(WaitId::Pid(Pid::from_child(child)), options)? else { return Ok(None); - } - // SAFETY: CLD terminal records initialize si_status. - let detail = unsafe { info.si_status() }; - let status = match info.si_code { - libc::CLD_EXITED => detail, - libc::CLD_KILLED | libc::CLD_DUMPED => 128_i32.saturating_add(detail), - _ => { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "waitid returned a nonterminal child status", - )); - } }; - Ok(Some(status)) + if let Some(code) = status.exit_status() { + return Ok(Some(code)); + } + if let Some(signal) = status.terminating_signal() { + return Ok(Some(128_i32.saturating_add(signal))); + } + Err(io::Error::new( + io::ErrorKind::InvalidData, + "waitid returned a nonterminal child status", + )) } pub(super) fn publish_bounded( mut command: Command, input: Vec, timeout: Duration, - shutdown_fd: RawFd, + shutdown_fd: BorrowedFd<'static>, ) -> Result { if shutdown_requested(shutdown_fd)? { return Err(anyhow!("broker publication cancelled during shutdown")); @@ -254,17 +237,10 @@ fn write_input_until( mut stdin: ChildStdin, input: &[u8], deadline: BootDeadline, - shutdown_fd: RawFd, + shutdown_fd: BorrowedFd<'static>, ) -> io::Result<()> { - let descriptor = stdin.as_raw_fd(); - // SAFETY: fcntl reads and updates descriptor-local flags. - let flags = unsafe { libc::fcntl(descriptor, libc::F_GETFL) }; - // SAFETY: `stdin` keeps the descriptor open, and F_SETFL only adds - // O_NONBLOCK to the flags just read from it. - if flags < 0 || unsafe { libc::fcntl(descriptor, libc::F_SETFL, flags | libc::O_NONBLOCK) } != 0 - { - return Err(io::Error::last_os_error()); - } + let flags = rustix_fs::fcntl_getfl(&stdin)?; + rustix_fs::fcntl_setfl(&stdin, flags | OFlags::NONBLOCK)?; let mut offset = 0; while offset < input.len() { if shutdown_requested(shutdown_fd)? { @@ -298,7 +274,7 @@ pub(super) fn run_bounded( timeout: Duration, output_cap: usize, output_mode: OutputMode, - shutdown_fd: RawFd, + shutdown_fd: BorrowedFd<'static>, ) -> Result { if shutdown_requested(shutdown_fd)? { return Err(anyhow!("broker helper cancelled during shutdown")); @@ -421,7 +397,7 @@ struct AbandonablePipe

{ abandon: Arc, } -impl AbandonablePipe

{ +impl AbandonablePipe

{ fn new(pipe: P, abandon: &Arc) -> Self { Self { pipe, @@ -429,42 +405,31 @@ impl AbandonablePipe

{ } } - fn wait_until_ready(&self, events: libc::c_short) -> io::Result<()> { - let mut pollfd = libc::pollfd { - fd: self.pipe.as_raw_fd(), - events, - revents: 0, - }; + fn wait_until_ready(&self, events: PollFlags) -> io::Result<()> { + let mut pollfds = [PollFd::new(&self.pipe, events)]; loop { if self.abandon.load(Ordering::Acquire) { return Err(io::Error::new(io::ErrorKind::TimedOut, PIPE_HELD_OPEN)); } - // SAFETY: pollfd points to one initialized entry, and the pipe - // stays open for as long as `self` owns it. - let result = unsafe { libc::poll(&raw mut pollfd, 1, PIPE_POLL_INTERVAL_MS) }; - if result > 0 { - return Ok(()); - } - if result < 0 { - let error = io::Error::last_os_error(); - if error.kind() != io::ErrorKind::Interrupted { - return Err(error); - } + match event::poll(&mut pollfds, Some(&PIPE_POLL_INTERVAL)) { + Ok(0) | Err(Errno::INTR) => {} + Ok(_) => return Ok(()), + Err(errno) => return Err(errno.into()), } } } } -impl Read for AbandonablePipe

{ +impl Read for AbandonablePipe

{ fn read(&mut self, buffer: &mut [u8]) -> io::Result { - self.wait_until_ready(libc::POLLIN)?; + self.wait_until_ready(PollFlags::IN)?; self.pipe.read(buffer) } } -impl Write for AbandonablePipe

{ +impl Write for AbandonablePipe

{ fn write(&mut self, buffer: &[u8]) -> io::Result { - self.wait_until_ready(libc::POLLOUT)?; + self.wait_until_ready(PollFlags::OUT)?; // A writable pipe has room for at least PIPE_BUF bytes, so a write of // no more than that cannot block on a reader that stopped reading. self.pipe.write(&buffer[..buffer.len().min(libc::PIPE_BUF)]) diff --git a/src/process_broker/server.rs b/src/process_broker/server.rs index a9ab880e..a000f40e 100644 --- a/src/process_broker/server.rs +++ b/src/process_broker/server.rs @@ -1,12 +1,17 @@ use std::collections::{BTreeMap, VecDeque}; -use std::os::fd::{AsRawFd, OwnedFd, RawFd}; +use std::os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd, RawFd}; use std::os::unix::process::CommandExt; use std::process::{Child, ExitCode, Stdio}; use std::thread::Builder; use std::time::Duration; -use std::{env, io, mem}; +use std::{env, io}; use anyhow::{Context, Result, anyhow, bail}; +use rustix::event::{self, PollFd, PollFlags}; +use rustix::io::{self as rustix_io, Errno, FdFlags}; +use rustix::net::SocketType; +use rustix::net::sockopt; +use rustix::process::{self as rustix_process, Pid, Signal}; use crate::daemon::protocol_v2::ProtocolId; use crate::env_vars; @@ -33,18 +38,24 @@ pub(crate) fn run_internal_broker_if_requested() -> Option { .parse::() .ok()?; let token = env::var(BROKER_TOKEN_ENV).ok()?; - if fd != BROKER_FD - || shutdown_fd != BROKER_SHUTDOWN_FD - || !canonical_lower_hex(&token, 64) - || validate_broker_socket(fd).is_err() - || validate_broker_socket(shutdown_fd).is_err() - { + if fd != BROKER_FD || shutdown_fd != BROKER_SHUTDOWN_FD || !canonical_lower_hex(&token, 64) { + return Some(ExitCode::from(126)); + } + // SAFETY: the fixed numbers are never -1, the socket checks below reject a + // number that is not an open broker channel before anything else uses it, + // and nothing in this process closes those channels before it exits. + let (fd, shutdown_fd) = unsafe { + ( + BorrowedFd::borrow_raw(fd), + BorrowedFd::borrow_raw(shutdown_fd), + ) + }; + if validate_broker_socket(fd).is_err() || validate_broker_socket(shutdown_fd).is_err() { return Some(ExitCode::from(126)); } // Restore CLOEXEC before any runtime helper can inherit broker internals. for descriptor in [fd, shutdown_fd] { - // SAFETY: both descriptors were validated as inherited broker channels. - if unsafe { libc::fcntl(descriptor, libc::F_SETFD, libc::FD_CLOEXEC) } != 0 { + if rustix_io::fcntl_setfd(descriptor, FdFlags::CLOEXEC).is_err() { return Some(ExitCode::from(126)); } } @@ -60,23 +71,8 @@ pub(crate) fn run_internal_broker_if_requested() -> Option { }) } -fn validate_broker_socket(fd: RawFd) -> io::Result<()> { - let mut socket_type = 0_i32; - let mut length = mem::size_of::() as libc::socklen_t; - // SAFETY: the output slots are valid for getsockopt. - if unsafe { - libc::getsockopt( - fd, - libc::SOL_SOCKET, - libc::SO_TYPE, - (&raw mut socket_type).cast(), - &raw mut length, - ) - } != 0 - { - return Err(io::Error::last_os_error()); - } - if socket_type != libc::SOCK_SEQPACKET { +fn validate_broker_socket(fd: BorrowedFd<'_>) -> io::Result<()> { + if sockopt::socket_type(fd)? != SocketType::SEQPACKET { return Err(io::Error::new( io::ErrorKind::InvalidInput, "broker descriptor is not SOCK_SEQPACKET", @@ -85,7 +81,11 @@ fn validate_broker_socket(fd: RawFd) -> io::Result<()> { Ok(()) } -fn broker_loop(socket: RawFd, shutdown_fd: RawFd, token: &str) -> Result<()> { +fn broker_loop( + socket: BorrowedFd<'static>, + shutdown_fd: BorrowedFd<'static>, + token: &str, +) -> Result<()> { let mut ownership = BrokerOwnership::default(); loop { if wait_for_request(socket, shutdown_fd)? == BrokerWake::Shutdown { @@ -139,7 +139,7 @@ fn broker_loop(socket: RawFd, shutdown_fd: RawFd, token: &str) -> Result<()> { let response_descriptors = wire_response .descriptors .iter() - .map(AsRawFd::as_raw_fd) + .map(AsFd::as_fd) .collect::>(); send_packet(socket, &bytes, &response_descriptors)?; if take_graceful_shutdown_signal(shutdown_fd)? { @@ -155,39 +155,27 @@ enum BrokerWake { Shutdown, } -fn wait_for_request(socket: RawFd, shutdown_fd: RawFd) -> io::Result { +fn wait_for_request(socket: BorrowedFd<'_>, shutdown_fd: BorrowedFd<'_>) -> io::Result { let mut descriptors = [ - libc::pollfd { - fd: socket, - events: libc::POLLIN, - revents: 0, - }, - libc::pollfd { - fd: shutdown_fd, - events: libc::POLLIN, - revents: 0, - }, + PollFd::from_borrowed_fd(socket, PollFlags::IN), + PollFd::from_borrowed_fd(shutdown_fd, PollFlags::IN), ]; + let unusable = PollFlags::HUP | PollFlags::ERR | PollFlags::NVAL; loop { - // SAFETY: descriptors points to two initialized pollfd entries. - let result = unsafe { libc::poll(descriptors.as_mut_ptr(), descriptors.len() as _, -1) }; - if result < 0 { - let error = io::Error::last_os_error(); - if error.kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(error); + match event::poll(&mut descriptors, None) { + Ok(_) => {} + Err(Errno::INTR) => continue, + Err(errno) => return Err(errno.into()), } - if descriptors[1].revents & libc::POLLIN != 0 && take_graceful_shutdown_signal(shutdown_fd)? - { + let socket_events = descriptors[0].revents(); + let shutdown_events = descriptors[1].revents(); + if shutdown_events.contains(PollFlags::IN) && take_graceful_shutdown_signal(shutdown_fd)? { return Ok(BrokerWake::Shutdown); } - if descriptors[0].revents & libc::POLLIN != 0 { + if socket_events.contains(PollFlags::IN) { return Ok(BrokerWake::Request); } - if descriptors[0].revents & (libc::POLLHUP | libc::POLLERR | libc::POLLNVAL) != 0 - || descriptors[1].revents & (libc::POLLHUP | libc::POLLERR | libc::POLLNVAL) != 0 - { + if socket_events.intersects(unusable) || shutdown_events.intersects(unusable) { return Err(io::Error::new( io::ErrorKind::BrokenPipe, "broker control channel became unusable", @@ -198,6 +186,14 @@ fn wait_for_request(socket: RawFd, shutdown_fd: RawFd) -> io::Result #[cfg(test)] pub(super) fn run_loop_for_test(socket: RawFd, shutdown_fd: RawFd, token: &str) -> Result<()> { + // SAFETY: the test broker thread owns both descriptors and keeps them open + // until this loop returns. + let (socket, shutdown_fd) = unsafe { + ( + BorrowedFd::borrow_raw(socket), + BorrowedFd::borrow_raw(shutdown_fd), + ) + }; broker_loop(socket, shutdown_fd, token) } @@ -242,7 +238,7 @@ fn handle_operation( operation: BrokerOperation, descriptors: &mut VecDeque, ownership: &mut BrokerOwnership, - shutdown_fd: RawFd, + shutdown_fd: BorrowedFd<'static>, ) -> Result { if shutdown_requested(shutdown_fd)? { bail!("broker operation cancelled during shutdown"); @@ -350,20 +346,20 @@ fn handle_operation( ), BrokerOperation::Signal { handle, signal } => { reject_descriptors(descriptors)?; - if !matches!( - signal, - libc::SIGUSR1 | libc::SIGUSR2 | libc::SIGTERM | libc::SIGKILL - ) { - bail!("signal is not allowed by broker manifest"); - } + let signal = match signal { + libc::SIGUSR1 => Signal::USR1, + libc::SIGUSR2 => Signal::USR2, + libc::SIGTERM => Signal::TERM, + libc::SIGKILL => Signal::KILL, + _ => bail!("signal is not allowed by broker manifest"), + }; let child = ownership .children .get(&handle) .ok_or_else(|| anyhow!("unknown broker child handle"))?; - // SAFETY: the broker retains the exact unreaped child handle. - if unsafe { libc::kill(child.id() as i32, signal) } != 0 { - return Err(io::Error::last_os_error()).context("broker child signal failed"); - } + rustix_process::kill_process(Pid::from_child(child), signal) + .map_err(io::Error::from) + .context("broker child signal failed")?; Ok(wire_outcome(BrokerOutcome::Acknowledged)) } BrokerOperation::TryWait { handle } => { @@ -409,7 +405,7 @@ fn spawn_helper( request: SpawnRequest, descriptors: &mut VecDeque, children: &mut BTreeMap, - shutdown_fd: RawFd, + shutdown_fd: BorrowedFd<'static>, ) -> Result { let SpawnRequest { kind, @@ -504,22 +500,17 @@ fn reject_descriptors(descriptors: &VecDeque) -> Result<()> { } fn set_cloexec(descriptor: &OwnedFd, enabled: bool) -> Result<()> { - // SAFETY: fcntl reads and updates descriptor-local flags. - let current = unsafe { libc::fcntl(descriptor.as_raw_fd(), libc::F_GETFD) }; - if current < 0 { - return Err(io::Error::last_os_error()).context("failed to read descriptor flags"); - } + let current = rustix_io::fcntl_getfd(descriptor) + .map_err(io::Error::from) + .context("failed to read descriptor flags")?; let updated = if enabled { - current | libc::FD_CLOEXEC + current | FdFlags::CLOEXEC } else { - current & !libc::FD_CLOEXEC + current - FdFlags::CLOEXEC }; - // SAFETY: `descriptor` is borrowed open for the call, and `updated` only - // changes FD_CLOEXEC in the flags just read from it. - if unsafe { libc::fcntl(descriptor.as_raw_fd(), libc::F_SETFD, updated) } != 0 { - return Err(io::Error::last_os_error()).context("failed to update descriptor flags"); - } - Ok(()) + rustix_io::fcntl_setfd(descriptor, updated) + .map_err(io::Error::from) + .context("failed to update descriptor flags") } fn wire_outcome(outcome: BrokerOutcome) -> BrokerWireResponse { @@ -551,30 +542,18 @@ fn canonical_lower_hex(value: &str, length: usize) -> bool { mod tests { use std::process::Command; + use rustix::net::{self, AddressFamily, SendFlags, SocketFlags}; + use super::*; - use std::os::fd::FromRawFd; fn test_socket_pair() -> (OwnedFd, OwnedFd) { - let mut sockets = [0; 2]; - assert_eq!( - // SAFETY: sockets has room for both returned descriptors. - unsafe { - libc::socketpair( - libc::AF_UNIX, - libc::SOCK_SEQPACKET | libc::SOCK_CLOEXEC, - 0, - sockets.as_mut_ptr(), - ) - }, - 0 - ); - // SAFETY: socketpair returned two fresh owned descriptors. - unsafe { - ( - OwnedFd::from_raw_fd(sockets[0]), - OwnedFd::from_raw_fd(sockets[1]), - ) - } + net::socketpair( + AddressFamily::UNIX, + SocketType::SEQPACKET, + SocketFlags::CLOEXEC, + None, + ) + .unwrap() } #[test] @@ -583,8 +562,7 @@ mod tests { let (shutdown_writer, shutdown_reader) = test_socket_pair(); drop(shutdown_writer); - let Err(error) = wait_for_request(control_reader.as_raw_fd(), shutdown_reader.as_raw_fd()) - else { + let Err(error) = wait_for_request(control_reader.as_fd(), shutdown_reader.as_fd()) else { panic!("hangup must unwind through destructive broker cleanup"); }; @@ -597,21 +575,13 @@ mod tests { let (shutdown_writer, shutdown_reader) = test_socket_pair(); let signal = [super::super::transport::GRACEFUL_SHUTDOWN_BYTE]; assert_eq!( - // SAFETY: both the descriptor and one-byte buffer are valid. - unsafe { - libc::send( - shutdown_writer.as_raw_fd(), - signal.as_ptr().cast(), - signal.len(), - libc::MSG_NOSIGNAL, - ) - }, + net::send(&shutdown_writer, &signal, SendFlags::NOSIGNAL).unwrap(), 1 ); drop(shutdown_writer); assert!(matches!( - wait_for_request(control_reader.as_raw_fd(), shutdown_reader.as_raw_fd()), + wait_for_request(control_reader.as_fd(), shutdown_reader.as_fd()), Ok(BrokerWake::Shutdown) )); } @@ -622,20 +592,11 @@ mod tests { let (shutdown_writer, shutdown_reader) = test_socket_pair(); let invalid = [2_u8]; assert_eq!( - // SAFETY: both the descriptor and one-byte buffer are valid. - unsafe { - libc::send( - shutdown_writer.as_raw_fd(), - invalid.as_ptr().cast(), - invalid.len(), - libc::MSG_NOSIGNAL, - ) - }, + net::send(&shutdown_writer, &invalid, SendFlags::NOSIGNAL).unwrap(), 1 ); - let Err(error) = wait_for_request(control_reader.as_raw_fd(), shutdown_reader.as_raw_fd()) - else { + let Err(error) = wait_for_request(control_reader.as_fd(), shutdown_reader.as_fd()) else { panic!("invalid packet must not authorize ownership release"); }; @@ -648,20 +609,11 @@ mod tests { let (shutdown_writer, shutdown_reader) = test_socket_pair(); let invalid = [super::super::transport::GRACEFUL_SHUTDOWN_BYTE; 2]; assert_eq!( - // SAFETY: both the descriptor and two-byte buffer are valid. - unsafe { - libc::send( - shutdown_writer.as_raw_fd(), - invalid.as_ptr().cast(), - invalid.len(), - libc::MSG_NOSIGNAL, - ) - }, + net::send(&shutdown_writer, &invalid, SendFlags::NOSIGNAL).unwrap(), 2 ); - let Err(error) = wait_for_request(control_reader.as_raw_fd(), shutdown_reader.as_raw_fd()) - else { + let Err(error) = wait_for_request(control_reader.as_fd(), shutdown_reader.as_fd()) else { panic!("oversized packet must not authorize ownership release"); }; diff --git a/src/process_broker/tests.rs b/src/process_broker/tests.rs index acd5cd15..b801f27d 100644 --- a/src/process_broker/tests.rs +++ b/src/process_broker/tests.rs @@ -1,5 +1,5 @@ use std::ffi::OsStr; -use std::os::fd::AsRawFd; +use std::os::fd::{AsFd, AsRawFd}; use std::os::unix::process::CommandExt; use std::path::Path; use std::process::Command; @@ -778,7 +778,7 @@ fn owned_child_inherits_daemon_pidfd_without_leaking_broker_copy() { OsStr::new("sleep"), [OsStr::new("30")], Vec::new(), - watchdog.as_raw_fd(), + watchdog.as_fd(), ) .unwrap(); let inherited_pidfd = fs::read_dir(format!("/proc/{}/fd", child.id())) diff --git a/src/process_broker/transport.rs b/src/process_broker/transport.rs index d3dca23b..a21048dd 100644 --- a/src/process_broker/transport.rs +++ b/src/process_broker/transport.rs @@ -1,10 +1,18 @@ use std::collections::VecDeque; +use std::io::{self, IoSlice, IoSliceMut}; use std::mem::MaybeUninit; -use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd}; +use std::os::fd::{BorrowedFd, OwnedFd}; use std::time::Duration; -use std::{io, mem, ptr}; use anyhow::{Context, Result, anyhow, bail}; +use rustix::event::{self, PollFd, PollFlags, Timespec}; +use rustix::fs::{self as rustix_fs, FileType, MemfdFlags}; +use rustix::io::{self as rustix_io, Errno}; +use rustix::net::sockopt::{self, Timeout}; +use rustix::net::{ + self, RecvAncillaryBuffer, RecvAncillaryMessage, RecvFlags, ReturnFlags, SendAncillaryBuffer, + SendAncillaryMessage, SendFlags, +}; use super::wire::{ BlobWire, INLINE_BLOB_BYTES, MAX_OUTPUT_BYTES, MAX_PACKET_BYTES, MAX_PACKET_DESCRIPTORS, @@ -13,80 +21,47 @@ use super::wire::{ pub(super) const GRACEFUL_SHUTDOWN_BYTE: u8 = 1; -pub(super) fn set_socket_timeout(fd: RawFd, timeout: Duration) -> io::Result<()> { +pub(super) fn set_socket_timeout(fd: BorrowedFd<'_>, timeout: Duration) -> io::Result<()> { let timeout = timeout.max(Duration::from_millis(1)); - let value = libc::timeval { - tv_sec: libc::time_t::try_from(timeout.as_secs()) - .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "socket timeout overflow"))?, - tv_usec: libc::suseconds_t::from(timeout.subsec_micros()), - }; - for option in [libc::SO_SNDTIMEO, libc::SO_RCVTIMEO] { - // SAFETY: value is initialized and setsockopt copies it. - if unsafe { - libc::setsockopt( - fd, - libc::SOL_SOCKET, - option, - (&raw const value).cast(), - mem::size_of::() as libc::socklen_t, - ) - } != 0 - { - return Err(io::Error::last_os_error()); - } + for option in [Timeout::Send, Timeout::Recv] { + sockopt::set_socket_timeout(fd, option, Some(timeout))?; } Ok(()) } -pub(super) fn shutdown_requested(descriptor: RawFd) -> io::Result { - let mut pollfd = libc::pollfd { - fd: descriptor, - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: pollfd points to one initialized entry. - let result = unsafe { libc::poll(&raw mut pollfd, 1, 0) }; - if result < 0 { - return Err(io::Error::last_os_error()); - } - if pollfd.revents & libc::POLLNVAL != 0 { +pub(super) fn shutdown_requested(descriptor: BorrowedFd<'_>) -> io::Result { + let mut pollfds = [PollFd::from_borrowed_fd(descriptor, PollFlags::IN)]; + event::poll(&mut pollfds, Some(&Timespec::default()))?; + let revents = pollfds[0].revents(); + if revents.contains(PollFlags::NVAL) { return Err(io::Error::new( io::ErrorKind::BrokenPipe, "broker shutdown channel became invalid", )); } - Ok(pollfd.revents & (libc::POLLIN | libc::POLLHUP | libc::POLLERR) != 0) + Ok(revents.intersects(PollFlags::IN | PollFlags::HUP | PollFlags::ERR)) } -pub(super) fn take_graceful_shutdown_signal(descriptor: RawFd) -> io::Result { - let mut pollfd = libc::pollfd { - fd: descriptor, - events: libc::POLLIN, - revents: 0, - }; +pub(super) fn take_graceful_shutdown_signal(descriptor: BorrowedFd<'_>) -> io::Result { + let mut pollfds = [PollFd::from_borrowed_fd(descriptor, PollFlags::IN)]; loop { - // SAFETY: pollfd points to one initialized entry. - let result = unsafe { libc::poll(&raw mut pollfd, 1, 0) }; - if result < 0 { - let error = io::Error::last_os_error(); - if error.kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(error); + match event::poll(&mut pollfds, Some(&Timespec::default())) { + Ok(0) => return Ok(false), + Ok(_) => {} + Err(Errno::INTR) => continue, + Err(errno) => return Err(errno.into()), } - if result == 0 { - return Ok(false); - } - if pollfd.revents & libc::POLLNVAL != 0 { + let revents = pollfds[0].revents(); + if revents.contains(PollFlags::NVAL) { return Err(io::Error::new( io::ErrorKind::BrokenPipe, "broker shutdown channel became invalid", )); } - if pollfd.revents & libc::POLLIN != 0 { + if revents.contains(PollFlags::IN) { return receive_graceful_shutdown_signal(descriptor); } - if pollfd.revents & (libc::POLLHUP | libc::POLLERR) != 0 { + if revents.intersects(PollFlags::HUP | PollFlags::ERR) { return Err(io::Error::new( io::ErrorKind::BrokenPipe, "broker shutdown channel closed without a graceful signal", @@ -96,27 +71,17 @@ pub(super) fn take_graceful_shutdown_signal(descriptor: RawFd) -> io::Result io::Result { - let mut byte = 0_u8; +fn receive_graceful_shutdown_signal(descriptor: BorrowedFd<'_>) -> io::Result { + let mut byte = [0_u8]; loop { - // MSG_TRUNC makes an oversized SOCK_SEQPACKET message report its full length. - // SAFETY: byte is a valid one-byte destination for this live broker socket. - let received = unsafe { - libc::recv( - descriptor, - (&raw mut byte).cast(), - 1, - libc::MSG_DONTWAIT | libc::MSG_TRUNC, - ) + // TRUNC makes an oversized SOCK_SEQPACKET message report its full length. + let flags = RecvFlags::DONTWAIT | RecvFlags::TRUNC; + let received = match net::recv(descriptor, &mut byte, flags) { + Ok((_, received)) => received, + Err(Errno::INTR) => continue, + Err(errno) => return Err(errno.into()), }; - if received < 0 { - let error = io::Error::last_os_error(); - if error.kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(error); - } - if received == 1 && byte == GRACEFUL_SHUTDOWN_BYTE { + if received == 1 && byte[0] == GRACEFUL_SHUTDOWN_BYTE { return Ok(true); } return Err(io::Error::new( @@ -166,28 +131,11 @@ pub(super) fn decode_blob( let mut bytes = vec![0_u8; length]; let mut offset = 0; while offset < bytes.len() { - // SAFETY: the destination range is writable and the validated - // memfd has exactly the declared immutable length. - let read = unsafe { - libc::pread( - descriptor.as_raw_fd(), - bytes[offset..].as_mut_ptr().cast(), - bytes.len() - offset, - offset as libc::off_t, - ) - }; - if read > 0 { - offset += read as usize; - } else if read < 0 - && io::Error::last_os_error().kind() == io::ErrorKind::Interrupted - { - continue; - } else { - return Err(if read == 0 { - anyhow!("broker memfd ended before its declared length") - } else { - io::Error::last_os_error().into() - }); + match rustix_io::pread(&descriptor, &mut bytes[offset..], offset as u64) { + Ok(0) => bail!("broker memfd ended before its declared length"), + Ok(read) => offset += read, + Err(Errno::INTR) => {} + Err(errno) => return Err(io::Error::from(errno).into()), } } Ok(bytes) @@ -196,206 +144,120 @@ pub(super) fn decode_blob( } fn sealed_memfd(bytes: &[u8]) -> Result { - let name = c"wayscriber-broker-payload"; - // SAFETY: name is a valid C string and flags request a private, sealable fd. - let raw = - unsafe { libc::memfd_create(name.as_ptr(), libc::MFD_CLOEXEC | libc::MFD_ALLOW_SEALING) }; - if raw < 0 { - return Err(io::Error::last_os_error()).context("broker memfd creation failed"); - } - // SAFETY: memfd_create returned a new owned descriptor. - let descriptor = unsafe { OwnedFd::from_raw_fd(raw) }; + let descriptor = rustix_fs::memfd_create( + c"wayscriber-broker-payload", + MemfdFlags::CLOEXEC | MemfdFlags::ALLOW_SEALING, + ) + .map_err(io::Error::from) + .context("broker memfd creation failed")?; let mut offset = 0; while offset < bytes.len() { - // SAFETY: the source range is readable for the requested length. - let written = unsafe { - libc::write( - descriptor.as_raw_fd(), - bytes[offset..].as_ptr().cast(), - bytes.len() - offset, - ) - }; - if written > 0 { - offset += written as usize; - } else if written < 0 && io::Error::last_os_error().kind() == io::ErrorKind::Interrupted { - continue; - } else { - return Err(if written == 0 { - anyhow!("short write while creating broker memfd") - } else { - io::Error::last_os_error().into() - }); + match rustix_io::write(&descriptor, &bytes[offset..]) { + Ok(0) => bail!("short write while creating broker memfd"), + Ok(written) => offset += written, + Err(Errno::INTR) => {} + Err(errno) => return Err(io::Error::from(errno).into()), } } - // SAFETY: fcntl changes only seal metadata on the owned memfd. - if unsafe { - libc::fcntl( - descriptor.as_raw_fd(), - libc::F_ADD_SEALS, - REQUIRED_MEMFD_SEALS, - ) - } < 0 - { - return Err(io::Error::last_os_error()).context("failed to seal broker memfd"); - } + rustix_fs::fcntl_add_seals(&descriptor, REQUIRED_MEMFD_SEALS) + .map_err(io::Error::from) + .context("failed to seal broker memfd")?; Ok(descriptor) } fn validate_sealed_memfd(descriptor: &OwnedFd, length: usize) -> Result<()> { - let mut stat = MaybeUninit::::uninit(); - // SAFETY: stat points to writable storage for fstat. - if unsafe { libc::fstat(descriptor.as_raw_fd(), stat.as_mut_ptr()) } != 0 { - return Err(io::Error::last_os_error()).context("failed to inspect broker memfd"); - } - // SAFETY: fstat initialized stat after success. - let stat = unsafe { stat.assume_init() }; - if (stat.st_mode & libc::S_IFMT) != libc::S_IFREG || stat.st_size != length as libc::off_t { + let stat = rustix_fs::fstat(descriptor) + .map_err(io::Error::from) + .context("failed to inspect broker memfd")?; + if FileType::from_raw_mode(stat.st_mode) != FileType::RegularFile + || u64::try_from(stat.st_size).ok() != Some(length as u64) + { bail!("broker memfd shape does not match its declaration"); } - // SAFETY: F_GET_SEALS reads descriptor metadata. - let seals = unsafe { libc::fcntl(descriptor.as_raw_fd(), libc::F_GET_SEALS) }; - if seals < 0 || seals & REQUIRED_MEMFD_SEALS != REQUIRED_MEMFD_SEALS { + let sealed = rustix_fs::fcntl_get_seals(descriptor) + .is_ok_and(|seals| seals.contains(REQUIRED_MEMFD_SEALS)); + if !sealed { bail!("broker memfd is not immutably sealed"); } Ok(()) } -pub(super) fn send_packet(fd: RawFd, packet: &[u8], descriptors: &[RawFd]) -> io::Result<()> { +pub(super) fn send_packet( + fd: BorrowedFd<'_>, + packet: &[u8], + descriptors: &[BorrowedFd<'_>], +) -> io::Result<()> { if descriptors.len() > MAX_PACKET_DESCRIPTORS { return Err(io::Error::new( io::ErrorKind::InvalidInput, "too many broker packet descriptors", )); } + let mut space = [MaybeUninit::uninit(); rustix::cmsg_space!(ScmRights(MAX_PACKET_DESCRIPTORS))]; + let mut control = SendAncillaryBuffer::new(&mut space); + if !descriptors.is_empty() && !control.push(SendAncillaryMessage::ScmRights(descriptors)) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "broker packet descriptors do not fit their control buffer", + )); + } + let payload = [IoSlice::new(packet)]; loop { - let mut iovec = libc::iovec { - iov_base: packet.as_ptr().cast_mut().cast(), - iov_len: packet.len(), - }; - let control_len = if descriptors.is_empty() { - 0 - } else { - // SAFETY: CMSG_SPACE only performs arithmetic on its argument. - unsafe { libc::CMSG_SPACE(mem::size_of_val(descriptors) as libc::c_uint) as usize } - }; - let mut control = vec![0_usize; control_len.div_ceil(mem::size_of::())]; - // SAFETY: zero is a valid initial state for msghdr. - let mut message: libc::msghdr = unsafe { mem::zeroed() }; - message.msg_iov = &raw mut iovec; - message.msg_iovlen = 1; - if !descriptors.is_empty() { - message.msg_control = control.as_mut_ptr().cast(); - message.msg_controllen = control_len; - // SAFETY: the control buffer has CMSG_SPACE bytes. - unsafe { - let header = libc::CMSG_FIRSTHDR(&raw const message); - (*header).cmsg_level = libc::SOL_SOCKET; - (*header).cmsg_type = libc::SCM_RIGHTS; - (*header).cmsg_len = - libc::CMSG_LEN(mem::size_of_val(descriptors) as libc::c_uint) as usize; - ptr::copy_nonoverlapping( - descriptors.as_ptr(), - libc::CMSG_DATA(header).cast::(), - descriptors.len(), - ); + match net::sendmsg(fd, &payload, &mut control, SendFlags::NOSIGNAL) { + Ok(sent) if sent == packet.len() => return Ok(()), + Ok(_) => { + return Err(io::Error::new( + io::ErrorKind::WriteZero, + "short seqpacket send", + )); } + Err(Errno::INTR) => {} + Err(errno) => return Err(errno.into()), } - // SAFETY: message references live packet/control buffers. - let sent = unsafe { libc::sendmsg(fd, &raw const message, libc::MSG_NOSIGNAL) }; - if sent == packet.len() as isize { - return Ok(()); - } - if sent < 0 && io::Error::last_os_error().kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(if sent < 0 { - io::Error::last_os_error() - } else { - io::Error::new(io::ErrorKind::WriteZero, "short seqpacket send") - }); } } -pub(super) fn recv_packet(fd: RawFd) -> io::Result<(Vec, Vec)> { +pub(super) fn recv_packet(fd: BorrowedFd<'_>) -> io::Result<(Vec, Vec)> { let mut buffer = vec![0_u8; MAX_PACKET_BYTES + 1]; - loop { - let mut iovec = libc::iovec { - iov_base: buffer.as_mut_ptr().cast(), - iov_len: buffer.len(), - }; - // SAFETY: CMSG_SPACE only performs arithmetic on its argument. - let control_len = unsafe { - libc::CMSG_SPACE((MAX_PACKET_DESCRIPTORS * mem::size_of::()) as libc::c_uint) - as usize - }; - let mut control = vec![0_usize; control_len.div_ceil(mem::size_of::())]; - // SAFETY: zero is a valid initial state for msghdr. - let mut message: libc::msghdr = unsafe { mem::zeroed() }; - message.msg_iov = &raw mut iovec; - message.msg_iovlen = 1; - message.msg_control = control.as_mut_ptr().cast(); - message.msg_controllen = control_len; - // SAFETY: message references writable packet/control buffers. - let read = unsafe { libc::recvmsg(fd, &raw mut message, libc::MSG_CMSG_CLOEXEC) }; - if read > 0 { - let read = read as usize; - let mut descriptors = Vec::new(); - // SAFETY: recvmsg initialized ancillary headers in the buffer. - unsafe { - let mut header = libc::CMSG_FIRSTHDR(&raw const message); - while !header.is_null() { - if (*header).cmsg_level != libc::SOL_SOCKET - || (*header).cmsg_type != libc::SCM_RIGHTS - { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "unexpected broker packet ancillary data", - )); - } - let base_len = libc::CMSG_LEN(0) as usize; - if (*header).cmsg_len < base_len { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "malformed broker descriptor payload", - )); - } - let data_len = (*header).cmsg_len - base_len; - if !data_len.is_multiple_of(mem::size_of::()) { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "misaligned broker descriptor payload", - )); - } - let count = data_len / mem::size_of::(); - let data = libc::CMSG_DATA(header).cast::(); - for index in 0..count { - descriptors.push(OwnedFd::from_raw_fd(*data.add(index))); - } - header = libc::CMSG_NXTHDR(&raw const message, header); - } - } - if read > MAX_PACKET_BYTES - || message.msg_flags & (libc::MSG_TRUNC | libc::MSG_CTRUNC) != 0 - || descriptors.len() > MAX_PACKET_DESCRIPTORS - { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "broker packet exceeds its data or descriptor cap", - )); - } - buffer.truncate(read); - return Ok((buffer, descriptors)); + let mut space = [MaybeUninit::uninit(); rustix::cmsg_space!(ScmRights(MAX_PACKET_DESCRIPTORS))]; + let mut control = RecvAncillaryBuffer::new(&mut space); + let received = loop { + let mut payload = [IoSliceMut::new(&mut buffer)]; + match net::recvmsg(fd, &mut payload, &mut control, RecvFlags::CMSG_CLOEXEC) { + Ok(received) => break received, + Err(Errno::INTR) => {} + Err(errno) => return Err(errno.into()), } - if read == 0 { - return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, - "broker socket closed", - )); - } - let error = io::Error::last_os_error(); - if error.kind() == io::ErrorKind::Interrupted { - continue; - } - return Err(error); + }; + if received.bytes == 0 { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "broker socket closed", + )); } + // Descriptors left in `control` close when it drops, so every rejection + // below releases what the packet carried. + let mut descriptors = Vec::new(); + for message in control.drain() { + let RecvAncillaryMessage::ScmRights(passed) = message else { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "unexpected broker packet ancillary data", + )); + }; + descriptors.extend(passed); + } + if received.bytes > MAX_PACKET_BYTES + || received + .flags + .intersects(ReturnFlags::TRUNC | ReturnFlags::CTRUNC) + || descriptors.len() > MAX_PACKET_DESCRIPTORS + { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "broker packet exceeds its data or descriptor cap", + )); + } + buffer.truncate(received.bytes); + Ok((buffer, descriptors)) } diff --git a/src/process_broker/wire.rs b/src/process_broker/wire.rs index a920ac3e..262c0296 100644 --- a/src/process_broker/wire.rs +++ b/src/process_broker/wire.rs @@ -3,6 +3,7 @@ use std::os::fd::OwnedFd; use std::os::unix::ffi::OsStrExt; use anyhow::{Result, bail}; +use rustix::fs::SealFlags; use serde::{Deserialize, Serialize}; pub(super) const BROKER_FD: i32 = 3; @@ -19,8 +20,10 @@ pub(super) const MAX_OUTPUT_BYTES: usize = 256 * 1024 * 1024; pub(super) const MAX_STDERR_BYTES: usize = 64 * 1024; pub(super) const MAX_OWNED_CHILDREN: usize = 64; pub(super) const MAX_PACKET_DESCRIPTORS: usize = 2; -pub(super) const REQUIRED_MEMFD_SEALS: i32 = - libc::F_SEAL_WRITE | libc::F_SEAL_GROW | libc::F_SEAL_SHRINK | libc::F_SEAL_SEAL; +pub(super) const REQUIRED_MEMFD_SEALS: SealFlags = SealFlags::WRITE + .union(SealFlags::GROW) + .union(SealFlags::SHRINK) + .union(SealFlags::SEAL); /// Failure text used when a `Complete`-mode helper writes past its stdout cap. /// From 66e9259f6f701e5eabd9f7519735953a504fa037 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:03:17 +0100 Subject: [PATCH 11/13] refactor: finish moving kernel calls off raw libc Detach from the terminal with rustix and std, and drop the remaining unsafe process, poll, and shutdown calls from the tests through small shared helpers. Prune the rustix features nothing ended up using, and record which call sites stay on libc and why in the agent guidance. --- AGENTS.md | 1 + Cargo.toml | 2 +- src/app/mod.rs | 24 ++++++---- src/capture/tests/manager.rs | 19 +++----- src/daemon/protocol_v2/tests.rs | 38 ++++++--------- src/process_broker/server.rs | 3 +- src/process_broker/tests.rs | 83 +++++++++++++-------------------- src/test_fake_helper.rs | 5 +- src/unix_signals/listener.rs | 9 ++-- 9 files changed, 80 insertions(+), 104 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2f013d49..b6994c8e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,6 +40,7 @@ - Never reach inside a poisoned lock. Use `expect` when the critical section cannot panic, or a fallback that leaves the data alone. Tests that serialize on process-wide state use `test_env::lock()` or a `test_env::Lease`. - `catch_unwind` is reserved for the reviewed worker-isolation sites that carry an `#[expect]` reason. Tests observe panics through a joined scoped thread. - Getters are `foo()`/`foo_mut()`, a fallible constructor is `try_new`, and type lifetimes have descriptive names. Every `unsafe` block has a `// SAFETY:` comment that says why it is sound. +- Make kernel calls through `std` or `rustix`, which need no `unsafe`, and rename on import when a module name clashes with `std` (`use rustix::fs as rustix_fs`). Use raw `libc` calls only where neither fits: signal handlers, the post-`clone` broker stub, `fail_stop`, the zero-signal pidfd check, the inotify header parser, `dup2` onto a fixed descriptor number, and `localtime_r`. `libc` constants stay fine. - Convert numbers losslessly with `From` (`f64::from(width)`) wherever it can be called; `const` initializers keep `as`. Take the middle of two values with `midpoint` (`f64::midpoint(a, b)`), which cannot overflow. - Inline named format arguments: `format!("{name}: {err}")`. - Use `map_or`, `map_or_else`, `is_some_and`, or `is_ok_and` instead of `map(..).unwrap_or(..)`. Use `let ... else` for a `match` that only unwraps or leaves, and `if let ... else` for a `match` with one pattern and a fallback. diff --git a/Cargo.toml b/Cargo.toml index f6b9622d..50f2d279 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -94,7 +94,7 @@ serde_ignored = "0.1" schemars = { version = "1.2", features = ["derive"], optional = true } libc = "0.2" # Safe wrappers for the kernel interfaces that the std library does not cover. -rustix = { version = "1", features = ["event", "fs", "net", "pipe", "process", "stdio", "termios", "thread", "time"] } +rustix = { version = "1", features = ["event", "fs", "net", "pipe", "process", "thread", "time"] } # Kernel random bytes for portal handle tokens and daemon protocol identifiers. getrandom = { version = "0.4", features = ["std"] } # Securely created, self-deleting temporary files for the OCR engine handoff. diff --git a/src/app/mod.rs b/src/app/mod.rs index 58e531bb..584d3991 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -22,10 +22,14 @@ use crate::{ }; use anyhow::Context; use env::env_flag_enabled; +#[cfg(unix)] +use rustix::process as rustix_process; use session::run_session_cli_commands; use std::ffi::OsString; use std::fs::{File, OpenOptions}; use std::io::ErrorKind; +#[cfg(unix)] +use std::io::IsTerminal; use std::path::{Path, PathBuf}; use usage::{log_overlay_controls, print_usage}; @@ -170,16 +174,17 @@ fn run_update_check() -> anyhow::Result<()> { #[cfg(unix)] fn detach_from_tty() { // Start a new session to drop the controlling terminal (prevents stuck shells). - // SAFETY: setsid takes no arguments; a failure only leaves the session as is. - unsafe { - let _ = libc::setsid(); - } + // A failure only leaves the session as is. + let _ = rustix_process::setsid(); // Point stdio that still refers to a TTY at /dev/null. Closing it instead // frees the descriptor number for the next open, and the logger's stderr // writes would then land in whatever file that open returned. - for fd in [libc::STDIN_FILENO, libc::STDOUT_FILENO, libc::STDERR_FILENO] { - // SAFETY: isatty only inspects the descriptor number it is given. - let is_tty = unsafe { libc::isatty(fd) } == 1; + let stdio = [ + (libc::STDIN_FILENO, io::stdin().is_terminal()), + (libc::STDOUT_FILENO, io::stdout().is_terminal()), + (libc::STDERR_FILENO, io::stderr().is_terminal()), + ]; + for (fd, is_tty) in stdio { if is_tty && let Err(error) = redirect_to_dev_null(fd) { log::warn!("Failed to detach descriptor {fd} from the terminal: {error}"); } @@ -414,6 +419,8 @@ mod tests { use std::io::Write; use std::os::fd::AsRawFd; + use rustix::io as rustix_io; + let temp = test_temp::tempdir().unwrap(); let path = temp.path().join("was-a-terminal"); let mut file = File::create(&path).unwrap(); @@ -422,8 +429,7 @@ mod tests { redirect_to_dev_null(file.as_raw_fd()).unwrap(); file.write_all(b"after").unwrap(); - // SAFETY: F_GETFD only inspects the descriptor the file still owns. - assert!(unsafe { libc::fcntl(file.as_raw_fd(), libc::F_GETFD) } >= 0); + rustix_io::fcntl_getfd(&file).unwrap(); assert_eq!(fs::read(&path).unwrap(), b"before "); } diff --git a/src/capture/tests/manager.rs b/src/capture/tests/manager.rs index e1edace2..623e9545 100644 --- a/src/capture/tests/manager.rs +++ b/src/capture/tests/manager.rs @@ -1,4 +1,3 @@ -use std::os::fd::AsRawFd; use std::path::PathBuf; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex, mpsc}; @@ -163,23 +162,19 @@ async fn completion_wakes_an_already_blocked_runtime_poll_after_publication() { .unwrap(); started.notified().await; - let wake_fd = runtime_wake.poll_fd().as_raw_fd(); let (polling_tx, polling_rx) = mpsc::sync_channel(0); let poller = thread::spawn(move || { - let mut pollfd = libc::pollfd { - fd: wake_fd, - events: libc::POLLIN, - revents: 0, - }; polling_tx.send(()).unwrap(); - // SAFETY: the runtime wake source outlives this bounded poll and owns - // the descriptor for the entire test. - assert_eq!(unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }, 1); - assert_ne!(pollfd.revents & libc::POLLIN, 0); + assert!( + runtime_wake + .is_readable_within(Duration::from_secs(1)) + .unwrap() + ); + runtime_wake }); polling_rx.recv().unwrap(); release.notify_one(); - poller.join().unwrap(); + let runtime_wake = poller.join().unwrap(); runtime_wake.drain().unwrap(); assert!(matches!( diff --git a/src/daemon/protocol_v2/tests.rs b/src/daemon/protocol_v2/tests.rs index 4598d6f9..18231e3f 100644 --- a/src/daemon/protocol_v2/tests.rs +++ b/src/daemon/protocol_v2/tests.rs @@ -1,13 +1,22 @@ -use std::os::fd::AsRawFd; +use std::os::fd::{AsFd, BorrowedFd}; use std::process::Command; use std::time::{Duration, Instant}; use std::{env, process}; +use rustix::event::{self, PollFd, PollFlags, Timespec}; + use crate::tray_action::TrayAction; use crate::{env_vars, test_env, test_temp}; use super::*; +fn is_readable(fd: BorrowedFd<'_>, timeout: Duration) -> bool { + let mut pollfds = [PollFd::from_borrowed_fd(fd, PollFlags::IN)]; + let timeout = Timespec::try_from(timeout).unwrap(); + event::poll(&mut pollfds, Some(&timeout)).unwrap() == 1 + && pollfds[0].revents().contains(PollFlags::IN) +} + #[test] fn production_mode_is_closed_to_published_v2_after_cutover() { assert_eq!( @@ -71,15 +80,7 @@ fn boottime_deadline_source_arms_absolutely_and_drains_once() { .unwrap(); source.arm(deadline).unwrap(); - let mut poll_fd = libc::pollfd { - fd: source.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: poll_fd remains valid for this bounded poll. - let ready = unsafe { libc::poll(&raw mut poll_fd, 1, 500) }; - assert_eq!(ready, 1); - assert_ne!(poll_fd.revents & libc::POLLIN, 0); + assert!(is_readable(source.poll_fd(), Duration::from_millis(500))); assert!(source.drain().unwrap()); assert!(!source.drain().unwrap()); source.disarm().unwrap(); @@ -128,14 +129,7 @@ fn pidfd_wait_times_out_while_alive_and_returns_once_exited() { #[test] fn self_pidfd_is_not_readable_while_process_is_alive() { let pidfd = linux::open_self_pidfd().unwrap(); - let mut poll_fd = libc::pollfd { - fd: pidfd.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: poll_fd remains valid for the zero-timeout observation. - let ready = unsafe { libc::poll(&raw mut poll_fd, 1, 0) }; - assert_eq!(ready, 0); + assert!(!is_readable(pidfd.as_fd(), Duration::ZERO)); } #[test] @@ -189,13 +183,7 @@ fn command_queue_watch_reports_atomic_publication_and_rejects_replacement() { ) .unwrap(); - let mut poll_fd = libc::pollfd { - fd: watcher.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: watcher owns the descriptor throughout the bounded poll. - assert_eq!(unsafe { libc::poll(&raw mut poll_fd, 1, 500) }, 1); + assert!(is_readable(watcher.poll_fd(), Duration::from_millis(500))); assert!(watcher.drain().unwrap().scan_pending); fs::rename(&queue, temp.path().join("old-queue")).unwrap(); diff --git a/src/process_broker/server.rs b/src/process_broker/server.rs index a000f40e..5b4646b2 100644 --- a/src/process_broker/server.rs +++ b/src/process_broker/server.rs @@ -633,7 +633,6 @@ mod tests { drop(ownership); - // SAFETY: signal zero only probes the test-owned provider after teardown. - assert_ne!(unsafe { libc::kill(pid, 0) }, 0); + assert!(rustix_process::test_kill_process(Pid::from_raw(pid).unwrap()).is_err()); } } diff --git a/src/process_broker/tests.rs b/src/process_broker/tests.rs index b801f27d..15005482 100644 --- a/src/process_broker/tests.rs +++ b/src/process_broker/tests.rs @@ -1,5 +1,5 @@ use std::ffi::OsStr; -use std::os::fd::{AsFd, AsRawFd}; +use std::os::fd::AsFd; use std::os::unix::process::CommandExt; use std::path::Path; use std::process::Command; @@ -7,10 +7,23 @@ use std::sync::mpsc; use std::time::{Duration, Instant}; use std::{env, fs, iter, thread}; +use rustix::net::{self, Shutdown}; +use rustix::process::{self as rustix_process, Pid, Signal}; + use crate::daemon::protocol_v2; use crate::test_fake_helper::Role; use crate::{env_vars, process_broker, test_env, test_fake_helper, test_temp}; +fn process_exists(pid: libc::pid_t) -> bool { + Pid::from_raw(pid).is_some_and(|pid| rustix_process::test_kill_process(pid).is_ok()) +} + +fn kill_for_cleanup(pid: libc::pid_t) { + if let Some(pid) = Pid::from_raw(pid) { + let _ = rustix_process::kill_process(pid, Signal::KILL); + } +} + use super::*; fn wire_arguments(arguments: &[&str]) -> Vec { @@ -28,10 +41,7 @@ fn release_test_provider(release_path: &Path, proof_path: &Path, provider_pid: i return; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its provider. - unsafe { - libc::kill(provider_pid, libc::SIGKILL); - } + kill_for_cleanup(provider_pid); panic!("successful provider could not act after normal broker shutdown"); } thread::sleep(Duration::from_millis(5)); @@ -540,8 +550,7 @@ fn process_group_guard_cleans_up_before_ownership_transfer() { drop(child); - // SAFETY: signal zero only probes the test-owned helper PID after guard cleanup. - assert_ne!(unsafe { libc::kill(pid, 0) }, 0); + assert!(!process_exists(pid)); } #[test] @@ -762,8 +771,7 @@ fn broker_guard_preempts_an_active_operation_and_kills_its_group() { "broker shutdown waited for the active operation" ); assert!(run.join().unwrap().is_err()); - // SAFETY: signal zero only probes the test-owned helper PID. - assert_ne!(unsafe { libc::kill(helper_pid, 0) }, 0); + assert!(!process_exists(helper_pid)); } #[test] @@ -891,10 +899,7 @@ fn a_descendant_outside_the_helper_group_cannot_hold_the_broker_on_its_pipes() { .ok() .and_then(|pid| pid.trim().parse::().ok()) { - // SAFETY: cleanup of the descendant this test started in its own session. - unsafe { - libc::kill(pid, libc::SIGKILL); - } + kill_for_cleanup(pid); } let error = result.expect_err("output still held open must not read as complete"); @@ -950,8 +955,7 @@ fn normal_broker_shutdown_releases_successful_provider_descendant() { .trim() .parse::() .unwrap(); - // SAFETY: signal zero only checks the test-owned provider. - assert_eq!(unsafe { libc::kill(provider_pid, 0) }, 0); + assert!(process_exists(provider_pid)); drop(guard); @@ -984,28 +988,19 @@ fn shutdown_channel_peer_loss_kills_retained_provider() { .trim() .parse::() .unwrap(); - // SAFETY: signal zero only checks the test-owned provider. - assert_eq!(unsafe { libc::kill(provider_pid, 0) }, 0); + assert!(process_exists(provider_pid)); // Simulate abrupt parent loss without writing the graceful-shutdown packet. - assert_eq!( - // SAFETY: the descriptor belongs to this test's live broker guard. - unsafe { libc::shutdown(guard.broker().inner.shutdown.as_raw_fd(), libc::SHUT_RDWR) }, - 0 - ); + net::shutdown(&guard.broker().inner.shutdown, Shutdown::Both).unwrap(); drop(guard); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test provider PID. - if unsafe { libc::kill(provider_pid, 0) } != 0 { + if !process_exists(provider_pid) { break; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its provider. - unsafe { - libc::kill(provider_pid, libc::SIGKILL); - } + kill_for_cleanup(provider_pid); panic!("provider survived abnormal broker channel loss"); } thread::sleep(Duration::from_millis(5)); @@ -1069,22 +1064,17 @@ fn retained_publication_replacement_disposes_the_previous_provider() { .unwrap(); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test provider PID. - if unsafe { libc::kill(first_pid, 0) } != 0 { + if !process_exists(first_pid) { break; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its helpers. - unsafe { - libc::kill(first_pid, libc::SIGKILL); - libc::kill(second_pid, libc::SIGKILL); - } + kill_for_cleanup(first_pid); + kill_for_cleanup(second_pid); panic!("replaced publication provider remained alive"); } thread::sleep(Duration::from_millis(5)); } - // SAFETY: signal zero only checks the current test-owned provider. - assert_eq!(unsafe { libc::kill(second_pid, 0) }, 0); + assert!(process_exists(second_pid)); drop(guard); release_test_provider(&second_release_path, &second_proof_path, second_pid); @@ -1146,20 +1136,15 @@ fn failed_publication_replacement_preserves_the_current_provider() { .trim() .parse::() .unwrap(); - // SAFETY: signal zero only checks the current test-owned provider. - assert_eq!(unsafe { libc::kill(current_pid, 0) }, 0); + assert!(process_exists(current_pid)); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the failed test provider PID. - if unsafe { libc::kill(failed_pid, 0) } != 0 { + if !process_exists(failed_pid) { break; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its helpers. - unsafe { - libc::kill(current_pid, libc::SIGKILL); - libc::kill(failed_pid, libc::SIGKILL); - } + kill_for_cleanup(current_pid); + kill_for_cleanup(failed_pid); panic!("failed replacement provider survived cleanup"); } thread::sleep(Duration::from_millis(5)); @@ -1205,8 +1190,7 @@ fn retained_publication_kills_failed_or_input_stalled_provider_groups() { } let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test child PID. - if unsafe { libc::kill(provider_pid, 0) } != 0 { + if !process_exists(provider_pid) { break; } assert!( @@ -1307,8 +1291,7 @@ fn broker_shutdown_preempts_retained_publication_stdin_writer() { release_test_provider(¤t_release_path, ¤t_proof_path, current_pid); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test child PID. - if unsafe { libc::kill(provider_pid, 0) } != 0 { + if !process_exists(provider_pid) { break; } assert!( diff --git a/src/test_fake_helper.rs b/src/test_fake_helper.rs index 6eac2008..61f7c441 100644 --- a/src/test_fake_helper.rs +++ b/src/test_fake_helper.rs @@ -15,6 +15,8 @@ use std::os::unix::fs as unix_fs; use std::path::{Path, PathBuf}; use std::{env, fs, io, process}; +use rustix::process as rustix_process; + use anyhow::{Context, Result}; use crate::durable_io; @@ -124,8 +126,7 @@ fn play(arguments: &[OsString]) -> Result<()> { let report: Vec = match role { Role::Exit => return Ok(()), Role::ReportProcessGroup => { - // SAFETY: getpgrp has no preconditions and cannot fail. - let leader = unsafe { libc::getpgrp() } == process::id() as libc::pid_t; + let leader = rustix_process::getpgrp().as_raw_pid() == process::id() as libc::pid_t; if leader { "process-group-leader" } else { diff --git a/src/unix_signals/listener.rs b/src/unix_signals/listener.rs index a0328dd4..919e0004 100644 --- a/src/unix_signals/listener.rs +++ b/src/unix_signals/listener.rs @@ -7,6 +7,9 @@ use std::sync::{Arc, Mutex}; use std::thread::{self, JoinHandle}; use std::{fmt, io}; +#[cfg(test)] +use rustix::thread as rustix_thread; + use super::{ SIGNAL_WRITE_FD, begin_listener_teardown, clear_registered_signals, create_pipe, dispatch_pending_signals, finish_listener_teardown, install_handler, publish_listener_active, @@ -259,9 +262,9 @@ impl Drop for SignalListener { fn listener_thread(read_fd: RawFd, shared: &ListenerShared) { #[cfg(test)] { - // SAFETY: gettid has no preconditions and is used only by tests to - // observe that this exact thread reached its blocking read. - let tid = unsafe { libc::syscall(libc::SYS_gettid) as libc::pid_t }; + // Used only by tests to observe that this exact thread reached its + // blocking read. + let tid = rustix_thread::gettid().as_raw_pid(); shared.thread_tid.store(tid, Ordering::Release); } From b7bfbd6df6ae9af9d5985eeb57bb2f493e4834f1 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 22:53:29 +0100 Subject: [PATCH 12/13] fix(broker): validate inherited descriptors before borrowing them Borrowing a descriptor requires it to be open at that moment, so the broker entry cannot check a raw number through a borrow. Check both numbers with getsockopt first, as main did, which keeps the rejection of closed descriptors and makes the borrow's safety claim true. The entry exits 126 either way, so this only restores the contract. Pin that rejection with an integration test against the real binary. --- AGENTS.md | 2 +- src/process_broker/server.rs | 42 +++++++++++++++++++++++++----------- tests/cli.rs | 13 +++++++++++ 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b6994c8e..8194e042 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,7 +40,7 @@ - Never reach inside a poisoned lock. Use `expect` when the critical section cannot panic, or a fallback that leaves the data alone. Tests that serialize on process-wide state use `test_env::lock()` or a `test_env::Lease`. - `catch_unwind` is reserved for the reviewed worker-isolation sites that carry an `#[expect]` reason. Tests observe panics through a joined scoped thread. - Getters are `foo()`/`foo_mut()`, a fallible constructor is `try_new`, and type lifetimes have descriptive names. Every `unsafe` block has a `// SAFETY:` comment that says why it is sound. -- Make kernel calls through `std` or `rustix`, which need no `unsafe`, and rename on import when a module name clashes with `std` (`use rustix::fs as rustix_fs`). Use raw `libc` calls only where neither fits: signal handlers, the post-`clone` broker stub, `fail_stop`, the zero-signal pidfd check, the inotify header parser, `dup2` onto a fixed descriptor number, and `localtime_r`. `libc` constants stay fine. +- Make kernel calls through `std` or `rustix`, which need no `unsafe`, and rename on import when a module name clashes with `std` (`use rustix::fs as rustix_fs`). Use raw `libc` calls only where neither fits: signal handlers, the post-`clone` broker stub, `fail_stop`, the zero-signal pidfd check, validating the broker's inherited descriptors, the inotify header parser, `dup2` onto a fixed descriptor number, and `localtime_r`. `libc` constants stay fine. - Convert numbers losslessly with `From` (`f64::from(width)`) wherever it can be called; `const` initializers keep `as`. Take the middle of two values with `midpoint` (`f64::midpoint(a, b)`), which cannot overflow. - Inline named format arguments: `format!("{name}: {err}")`. - Use `map_or`, `map_or_else`, `is_some_and`, or `is_ok_and` instead of `map(..).unwrap_or(..)`. Use `let ... else` for a `match` that only unwraps or leaves, and `if let ... else` for a `match` with one pattern and a fallback. diff --git a/src/process_broker/server.rs b/src/process_broker/server.rs index 5b4646b2..1b41bb21 100644 --- a/src/process_broker/server.rs +++ b/src/process_broker/server.rs @@ -4,13 +4,11 @@ use std::os::unix::process::CommandExt; use std::process::{Child, ExitCode, Stdio}; use std::thread::Builder; use std::time::Duration; -use std::{env, io}; +use std::{env, io, mem}; use anyhow::{Context, Result, anyhow, bail}; use rustix::event::{self, PollFd, PollFlags}; use rustix::io::{self as rustix_io, Errno, FdFlags}; -use rustix::net::SocketType; -use rustix::net::sockopt; use rustix::process::{self as rustix_process, Pid, Signal}; use crate::daemon::protocol_v2::ProtocolId; @@ -38,21 +36,22 @@ pub(crate) fn run_internal_broker_if_requested() -> Option { .parse::() .ok()?; let token = env::var(BROKER_TOKEN_ENV).ok()?; - if fd != BROKER_FD || shutdown_fd != BROKER_SHUTDOWN_FD || !canonical_lower_hex(&token, 64) { + if fd != BROKER_FD + || shutdown_fd != BROKER_SHUTDOWN_FD + || !canonical_lower_hex(&token, 64) + || validate_broker_socket(fd).is_err() + || validate_broker_socket(shutdown_fd).is_err() + { return Some(ExitCode::from(126)); } - // SAFETY: the fixed numbers are never -1, the socket checks below reject a - // number that is not an open broker channel before anything else uses it, - // and nothing in this process closes those channels before it exits. + // SAFETY: both numbers were just checked to be open broker sockets, and + // nothing in this process closes them before it exits. let (fd, shutdown_fd) = unsafe { ( BorrowedFd::borrow_raw(fd), BorrowedFd::borrow_raw(shutdown_fd), ) }; - if validate_broker_socket(fd).is_err() || validate_broker_socket(shutdown_fd).is_err() { - return Some(ExitCode::from(126)); - } // Restore CLOEXEC before any runtime helper can inherit broker internals. for descriptor in [fd, shutdown_fd] { if rustix_io::fcntl_setfd(descriptor, FdFlags::CLOEXEC).is_err() { @@ -71,8 +70,25 @@ pub(crate) fn run_internal_broker_if_requested() -> Option { }) } -fn validate_broker_socket(fd: BorrowedFd<'_>) -> io::Result<()> { - if sockopt::socket_type(fd)? != SocketType::SEQPACKET { +fn validate_broker_socket(fd: RawFd) -> io::Result<()> { + let mut socket_type = 0_i32; + let mut length = mem::size_of::() as libc::socklen_t; + // SAFETY: the output slots are valid for getsockopt, which also rejects a + // number that is not an open descriptor. A borrowed descriptor cannot be + // used here because it requires the number to be open already. + if unsafe { + libc::getsockopt( + fd, + libc::SOL_SOCKET, + libc::SO_TYPE, + (&raw mut socket_type).cast(), + &raw mut length, + ) + } != 0 + { + return Err(io::Error::last_os_error()); + } + if socket_type != libc::SOCK_SEQPACKET { return Err(io::Error::new( io::ErrorKind::InvalidInput, "broker descriptor is not SOCK_SEQPACKET", @@ -542,7 +558,7 @@ fn canonical_lower_hex(value: &str, length: usize) -> bool { mod tests { use std::process::Command; - use rustix::net::{self, AddressFamily, SendFlags, SocketFlags}; + use rustix::net::{self, AddressFamily, SendFlags, SocketFlags, SocketType}; use super::*; diff --git a/tests/cli.rs b/tests/cli.rs index 00838a2e..f47a35a5 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -343,6 +343,19 @@ fn clustered_help_and_version_exit_zero_without_runtime_startup() { .stdout_starts_with("wayscriber "); } +#[cfg(unix)] +#[test] +fn internal_broker_entry_rejects_descriptors_that_are_not_broker_sockets() { + run_command( + wayscriber_cmd() + .env("WAYSCRIBER_INTERNAL_PROCESS_BROKER_FD", "3") + .env("WAYSCRIBER_INTERNAL_PROCESS_BROKER_SHUTDOWN_FD", "4") + .env("WAYSCRIBER_INTERNAL_PROCESS_BROKER_TOKEN", "a".repeat(64)), + ) + .failure() + .code(126); +} + #[test] fn invalid_arguments_exit_two() { for (args, diagnostic) in [ From 5966f2d7b81872282eb2b59a32e6bd260fa97389 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 22:54:45 +0100 Subject: [PATCH 13/13] test: treat non-wake readiness as a failure in wake assertions The old assertions required poll to report nothing, but the shared helpers only looked for readable input, so a hangup or a bad descriptor could satisfy a test that expects no wake. Reject any other readiness in both helpers and pin that with a hung-up pipe. --- src/backend/wayland/backend/runtime_wake.rs | 20 +++++++++++++++++++- src/daemon/protocol_v2/tests.rs | 12 ++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/backend/wayland/backend/runtime_wake.rs b/src/backend/wayland/backend/runtime_wake.rs index 80e4df96..5a57c8ab 100644 --- a/src/backend/wayland/backend/runtime_wake.rs +++ b/src/backend/wayland/backend/runtime_wake.rs @@ -142,6 +142,8 @@ impl RuntimeWakeSource { } /// Reports whether a wake arrives within `timeout`, without draining it. + /// Any other readiness is an error, so a broken descriptor cannot pass for + /// silence in a test that expects no wake. #[cfg(test)] pub(crate) fn is_readable_within(&self, timeout: Duration) -> io::Result { let readable = poll_with_retry(Some(timeout), |timeout_ms| { @@ -149,8 +151,14 @@ impl RuntimeWakeSource { if poll_for_ms(&mut pollfds, timeout_ms)? == 0 { return Ok(None); } - Ok(Some(pollfds[0].revents().contains(PollFlags::IN))) + validate_poll_readiness( + pollfds[0].revents(), + "runtime wake", + TerminalReadinessPolicy::Reject, + ) + .map(Some) })?; + Ok(readable == Some(true)) } } @@ -194,6 +202,7 @@ mod tests { use std::time::{Duration, Instant}; use std::{fs, thread}; + use rustix::pipe; use rustix::process::Pid; use rustix::thread as rustix_thread; @@ -274,6 +283,15 @@ mod tests { assert!(!source.drain().unwrap()); } + #[test] + fn readiness_without_a_wake_is_an_error_rather_than_silence() { + let (read, write) = pipe::pipe().unwrap(); + drop(write); + let source = RuntimeWakeSource { fd: Arc::new(read) }; + + assert!(source.is_readable_within(Duration::ZERO).is_err()); + } + #[test] fn wait_readable_times_out_without_a_wake() { let source = RuntimeWakeSource::try_new().unwrap(); diff --git a/src/daemon/protocol_v2/tests.rs b/src/daemon/protocol_v2/tests.rs index 18231e3f..ae44eef4 100644 --- a/src/daemon/protocol_v2/tests.rs +++ b/src/daemon/protocol_v2/tests.rs @@ -10,11 +10,19 @@ use crate::{env_vars, test_env, test_temp}; use super::*; +/// Polls for input. Any other readiness fails the test, so a broken descriptor +/// cannot pass for silence. fn is_readable(fd: BorrowedFd<'_>, timeout: Duration) -> bool { let mut pollfds = [PollFd::from_borrowed_fd(fd, PollFlags::IN)]; let timeout = Timespec::try_from(timeout).unwrap(); - event::poll(&mut pollfds, Some(&timeout)).unwrap() == 1 - && pollfds[0].revents().contains(PollFlags::IN) + let ready = event::poll(&mut pollfds, Some(&timeout)).unwrap(); + let revents = pollfds[0].revents(); + assert!( + ready == 0 || revents == PollFlags::IN, + "unexpected readiness {:#x}", + revents.bits() + ); + ready == 1 } #[test]