From fc85d0ee7ed36129864d7d66f5b4a866622834dc Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 26 Sep 2026 19:39:50 +0200 Subject: [PATCH] hive-priv: publish config files by rename, bound the toplevel build Every file hive-priv writes for another reader now goes through one StagedFile: a temp with a unique dot-prefixed `.partial` name in the destination directory, created O_EXCL|O_NOFOLLOW at its final mode (and owner, where one is set), fsynced, renamed onto the final name relative to a directory fd, then the directory fsynced. Dropping it unpublished unlinks the temp. - write_resource_limits wrote its systemd drop-in in place, so a concurrent daemon-reload could load a truncated file. - sync_agent_tmpfiles staged through a fixed `.tmp` name, so two overlapping syncs shared one inode and one could publish the other's bytes, or a mix. - write_nspawn_flags rewrote /etc/nixos-containers/.conf in place; it now keeps the file's existing owner and mode. - write_agent_dir_file (agent tokens, sidecars, pause marker) already renamed, but through a fixed temp opened O_TRUNC, with no fsync. - write_bridge_dns_marker_in truncated and wrote the marker in place; a leaf the container planted (symlink, FIFO) is now replaced by the rename instead of refused. - register_ci_runner must keep writing in place (nspawn pins the bind-mounted inode); it now creates the file 0600 when the tmpfiles seed is missing, instead of at the umask's mode until the chmod. nix_build_toplevel now runs nix as its own process-group leader and, after one hour, SIGKILLs the group and fails with "timed out after 3600s". One hour is four times CI's observed cold-cache flake check. Refs #4723 --- hive-priv/src/main.rs | 786 +++++++++++++++++++++++++++++------------- 1 file changed, 553 insertions(+), 233 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 66358cb6..66a15987 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -864,6 +864,127 @@ fn toplevel_attr(name: &str) -> String { format!("{META_DIR}#nixosConfigurations.{name}.config.system.build.toplevel") } +/// Wall-clock bound on [`nix_build_toplevel`]. Four times CI's observed +/// cold-cache `nix flake check` (~15 min, `.forgejo/workflows/ci.yml`), which +/// compiles this workspace's crates, the part of an agent toplevel no binary +/// cache holds; the rebuild path has usually warmed this exact attr already +/// (`hive-c0re`'s `prebuild_toplevel`). A stalled download is bounded by nix's +/// own `stalled-download-timeout`; this covers the rest, such as a wedged +/// nix-daemon holding the caller's build slot forever. +const NIX_BUILD_TIMEOUT: std::time::Duration = std::time::Duration::from_hours(1); + +/// How a [`run_bounded`] child ended. +enum BoundedRun { + Exited { + status: std::process::ExitStatus, + stdout: String, + stderr: String, + }, + /// Still running at the limit; its whole process group was killed. + TimedOut, +} + +/// Run `cmd` to completion or until `limit`, draining both pipes: stdout is +/// captured silently, stderr is logged and, when `writer` is set, forwarded +/// live as [`PrivEvent::Line`]s. +/// +/// The child leads its own process group and a timeout `SIGKILL`s the whole +/// group, so a grandchild still holding the pipes (or the daemon connection) +/// dies with it instead of outliving the error. +async fn run_bounded( + mut cmd: Command, + limit: std::time::Duration, + mut writer: Option<&mut OwnedWriteHalf>, +) -> std::io::Result { + use tokio::io::AsyncBufReadExt as _; + + let mut child = cmd + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .process_group(0) + .spawn()?; + // Group leader, so its pid is the pgid. + let pgid = child.id().and_then(|pid| i32::try_from(pid).ok()); + + let stdout = child.stdout.take().expect("stdout piped"); + let stderr = child.stderr.take().expect("stderr piped"); + let run = async { + let mut stdout_lines = BufReader::new(stdout).lines(); + let mut stderr_lines = BufReader::new(stderr).lines(); + let mut stdout_buf = String::new(); + let mut stderr_buf = String::new(); + + // ⚠️ Both pipes are drained concurrently even though only one is + // streamed: reading stderr alone would let stdout fill its pipe + // buffer and deadlock the child on a build with enough stdout output + // to fill it. + // + // ⚠️ Each stream's EOF is tracked separately rather than breaking on + // the first `None`: `next_line()` on a closed stream returns + // `Ok(None)` immediately and forever, so a loop that keeps polling a + // finished stream spins hot until the other one ends too. + let mut stdout_done = false; + let mut stderr_done = false; + while !(stdout_done && stderr_done) { + tokio::select! { + line = stdout_lines.next_line(), if !stdout_done => { + match line { + // Captured, never streamed — this is the store path. + Ok(Some(l)) => { + stdout_buf.push_str(&l); + stdout_buf.push('\n'); + } + Ok(None) => stdout_done = true, + Err(e) => { + tracing::warn!(error = %e, "nix build stdout read error"); + stdout_done = true; + } + } + } + line = stderr_lines.next_line(), if !stderr_done => { + match line { + // Streamed as it arrives, so the dashboard and + // `journalctl -f` show build progress live. + Ok(Some(l)) => { + tracing::info!(target: "nix-build-toplevel", "{l}"); + if let Some(w) = writer.as_deref_mut() { + write_line_event(w, PrivStream::Stderr, &l).await; + } + if !stderr_buf.is_empty() { + stderr_buf.push('\n'); + } + stderr_buf.push_str(&l); + } + Ok(None) => stderr_done = true, + Err(e) => { + tracing::warn!(error = %e, "nix build stderr read error"); + stderr_done = true; + } + } + } + } + } + let status = child.wait().await?; + Ok::<_, std::io::Error>(BoundedRun::Exited { + status, + stdout: stdout_buf, + stderr: stderr_buf, + }) + }; + if let Ok(exited) = tokio::time::timeout(limit, run).await { + return exited; + } + if let Some(pgid) = pgid { + // SAFETY: plain `kill(2)`; `-pgid` targets the child's own process + // group, which `process_group(0)` made it lead. + unsafe { + libc::kill(-pgid, libc::SIGKILL); + } + } + child.wait().await?; + Ok(BoundedRun::TimedOut) +} + /// Build `nixosConfigurations..config.system.build.toplevel` and /// return the resulting store path, so `create`/`update` can hand /// `nixos-container` an explicit `--system-path` instead of letting its @@ -876,91 +997,35 @@ fn toplevel_attr(name: &str) -> String { /// /// `writer` is `None` for the non-streaming call shape (`stream: false`); /// stderr still logs to journald either way, just without the -/// `PrivEvent::Line` forwarding. -async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) -> Result { - use tokio::io::AsyncBufReadExt as _; - +/// `PrivEvent::Line` forwarding. Bounded by [`NIX_BUILD_TIMEOUT`]. +async fn nix_build_toplevel(name: &str, writer: Option<&mut OwnedWriteHalf>) -> Result { let attr = toplevel_attr(name); - let args = [ + let mut cmd = Command::new("nix"); + cmd.args([ "--extra-experimental-features", "nix-command flakes", "build", "--no-link", "--print-out-paths", &attr, - ]; - let mut child = Command::new("nix") - .args(args) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - .with_context(|| format!("invoke nix build {attr}"))?; - - let stdout = child.stdout.take().expect("stdout piped"); - let stderr = child.stderr.take().expect("stderr piped"); - let mut stdout_lines = BufReader::new(stdout).lines(); - let mut stderr_lines = BufReader::new(stderr).lines(); - - let mut stdout_buf = String::new(); - let mut stderr_buf = String::new(); - - // ⚠️ Both pipes are drained concurrently even though only one is - // streamed: reading stderr alone would let stdout fill its pipe - // buffer and deadlock the child on a build with enough stdout output - // to fill it. - // - // ⚠️ Each stream's EOF is tracked separately rather than breaking on - // the first `None`: `next_line()` on a closed stream returns - // `Ok(None)` immediately and forever, so a loop that keeps polling a - // finished stream spins hot until the other one ends too. - let mut stdout_done = false; - let mut stderr_done = false; - while !(stdout_done && stderr_done) { - tokio::select! { - line = stdout_lines.next_line(), if !stdout_done => { - match line { - // Captured, never streamed — this is the store path. - Ok(Some(l)) => { - stdout_buf.push_str(&l); - stdout_buf.push('\n'); - } - Ok(None) => stdout_done = true, - Err(e) => { - tracing::warn!(error = %e, "nix build stdout read error"); - stdout_done = true; - } - } - } - line = stderr_lines.next_line(), if !stderr_done => { - match line { - // Streamed as it arrives — the progress the dashboard - // and `journalctl -f` were missing. - Ok(Some(l)) => { - tracing::info!(target: "nix-build-toplevel", "{l}"); - if let Some(w) = writer.as_deref_mut() { - write_line_event(w, PrivStream::Stderr, &l).await; - } - if !stderr_buf.is_empty() { - stderr_buf.push('\n'); - } - stderr_buf.push_str(&l); - } - Ok(None) => stderr_done = true, - Err(e) => { - tracing::warn!(error = %e, "nix build stderr read error"); - stderr_done = true; - } - } - } - } - } + ]); + let (status, stdout_buf, stderr_buf) = match run_bounded(cmd, NIX_BUILD_TIMEOUT, writer) + .await + .with_context(|| format!("run nix build {attr}"))? + { + BoundedRun::Exited { + status, + stdout, + stderr, + } => (status, stdout, stderr), + BoundedRun::TimedOut => bail!( + "nix build {attr} timed out after {}s; killed it", + NIX_BUILD_TIMEOUT.as_secs() + ), + }; // ⚠️ Success is decided by the exit status, not by "we parsed a // path" — a build can print to stdout and still fail. - let status = child - .wait() - .await - .with_context(|| format!("wait nix build {attr}"))?; if !status.success() { bail!( "nix build {attr} failed ({status}): {}", @@ -1102,7 +1167,9 @@ fn write_resource_limits( std::fs::create_dir_all(&dir).with_context(|| format!("create {dir}"))?; let path = format!("{dir}/hyperhive-limits.conf"); let content = limits_dropin_body(&runtime_dir, memory_max, cpu_quota, cpu_weight, io_weight); - std::fs::write(&path, content).with_context(|| format!("write {path}"))?; + // Published whole: a `daemon-reload` between truncate and write would + // otherwise load the unit with its limits and start guard missing. + publish_file(Path::new(&path), content.as_bytes(), 0o644, None)?; Ok((String::new(), String::new())) } @@ -1248,7 +1315,8 @@ fn clear_runner_credentials(path: &str) -> Result<()> { /// the registration token c0re passes here is written, and it lands on a host /// path bind-mounted read-only into hive-ci. async fn register_ci_runner(token: &str) -> Result<(String, String)> { - use std::os::unix::fs::PermissionsExt as _; + use std::io::Write as _; + use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; // Reject anything that could corrupt the `KEY=VALUE` env-file or smuggle a // second line — a forge registration token is an opaque single-line string. if token.is_empty() || token.contains(['\n', '\r', '\0']) { @@ -1259,8 +1327,15 @@ async fn register_ci_runner(token: &str) -> Result<(String, String)> { // `echo > $FILE`), NOT a temp+rename: nspawn pins this file's inode into // hive-ci at container start, so a rename would leave the running runner // reading the old content. Format + perms match the tmpfiles seed and the - // prefetch: `TOKEN=`, mode 0600, root-owned. - std::fs::write(token_path, format!("TOKEN={token}\n")) + // prefetch: `TOKEN=`, mode 0600, root-owned. Created 0600 when the + // seed is missing, so the token is never on disk at a wider mode. + std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(token_path) + .and_then(|mut f| f.write_all(format!("TOKEN={token}\n").as_bytes())) .with_context(|| format!("write {token_path}"))?; std::fs::set_permissions(token_path, std::fs::Permissions::from_mode(0o600)) .with_context(|| format!("chmod {token_path}"))?; @@ -1364,44 +1439,166 @@ fn matrix_token_filename(account: Option<&str>) -> String { } } -/// Name the content is written under before being renamed onto `filename`. -/// The leading dot is load-bearing rather than tidy: `nix/agent-modules/ -/// matrix.nix` starts the agent's matrix daemon on the glob `matrix-token*`, -/// which a `.partial` suffix would match — waking it on precisely the -/// empty file the rename exists to hide. +/// Name the content is written under before being renamed onto `filename`, +/// unique per call so two concurrent writes of one file never share a temp. +/// +/// The leading dot and the `.partial` suffix are load-bearing: +/// `nix/agent-modules/matrix.nix` starts the agent's matrix daemon on the glob +/// `matrix-token*`, and systemd reads only `*.conf` out of `tmpfiles.d` and +/// drop-in dirs, so none of them can pick up a half-written temp. fn partial_name(filename: &str) -> String { - format!(".{filename}.partial") + use std::sync::atomic::{AtomicU64, Ordering}; + static SEQ: AtomicU64 = AtomicU64::new(0); + let n = SEQ.fetch_add(1, Ordering::Relaxed); + format!(".{filename}.{}-{n}.partial", std::process::id()) } -/// Create/overwrite `dir/filename` at 0600 without following a symlink at the -/// leaf, returning the open fd for the caller to `fchown`. `filename` must be a -/// single plain component (no `/`, `.`, `..`) — the leaf sits in an -/// agent-writable dir, so `O_NOFOLLOW` refuses a planted symlink (`ELOOP`) -/// instead of letting this root-privileged write/chmod be redirected at another -/// file; `O_WRONLY` refuses a directory leaf (`EISDIR`); `O_TRUNC` keeps the -/// overwrite semantics for an existing regular file. `.mode(0o600)` sets the -/// create mode; the explicit `fchmod` after (on the fd, not a re-resolved path) -/// tightens an already-existing file and dodges umask. The returned fd is the -/// exact inode the write hit, so the caller's `fchown` is TOCTOU-immune. -fn write_state_file_nofollow(dir: &Path, filename: &str, content: &str) -> Result { - use std::io::Write as _; - use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; +/// Open `path` as a directory fd for [`StagedFile`] to work relative to. +fn open_dir(path: &Path) -> Result { + use std::os::unix::fs::OpenOptionsExt as _; + std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_DIRECTORY) + .open(path) + .with_context(|| format!("open directory {}", path.display())) +} - ensure_plain_filename("write_state_file_nofollow", filename)?; - let path = dir.join(filename); - let mut file = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .custom_flags(libc::O_NOFOLLOW) - .open(&path) - .with_context(|| format!("open (no-follow) {}", path.display()))?; - file.write_all(content.as_bytes()) - .with_context(|| format!("write {}", path.display()))?; - file.set_permissions(std::fs::Permissions::from_mode(0o600)) - .with_context(|| format!("chmod 600 {}", path.display()))?; - Ok(file) +/// A file written under a [`partial_name`] in its destination directory and +/// then renamed onto its final name, so a reader sees either the old file or +/// the complete new one, never a partial or wrong-mode one. Mode, and owner +/// when the caller `fchown`s [`file`](Self::file), are on the inode before the +/// rename makes it visible. +/// +/// Every step is relative to the `dir` fd, so it is safe in a directory an +/// agent or container controls: the temp is created `O_EXCL|O_NOFOLLOW`, which +/// refuses any entry already planted at its name, and `rename` replaces +/// whatever sits at the final name without following it. +/// +/// Dropped without [`publish`](Self::publish), it unlinks the temp, so a +/// failure between the two leaves the old file untouched and no residue. +struct StagedFile<'a> { + dir: &'a std::fs::File, + tmp: std::ffi::CString, + dest: std::ffi::CString, + dest_path: PathBuf, + file: std::fs::File, + published: bool, +} + +impl<'a> StagedFile<'a> { + /// Create the temp at `mode` and write `content` to it. The mode is set + /// again on the fd after the write, so the umask cannot change it. + fn write( + dir: &'a std::fs::File, + dir_path: &Path, + filename: &str, + content: &[u8], + mode: u32, + ) -> Result { + use std::io::Write as _; + use std::os::unix::fs::PermissionsExt as _; + + ensure_plain_filename("StagedFile::write", filename)?; + let dest_path = dir_path.join(filename); + let tmp_name = partial_name(filename); + let tmp = std::ffi::CString::new(tmp_name.as_str()) + .with_context(|| format!("NUL in filename {filename:?}"))?; + let dest = std::ffi::CString::new(filename) + .with_context(|| format!("NUL in filename {filename:?}"))?; + // SAFETY: `dir` is an open directory fd and `tmp` is NUL-terminated; + // the result is checked before it is wrapped. + let fd = unsafe { + libc::openat( + dir.as_raw_fd(), + tmp.as_ptr(), + libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL | libc::O_NOFOLLOW | libc::O_CLOEXEC, + mode, + ) + }; + if fd < 0 { + return Err(std::io::Error::last_os_error()) + .with_context(|| format!("create {}", dir_path.join(&tmp_name).display())); + } + // SAFETY: `fd` is a fresh, valid fd that nothing else owns. + let file = std::fs::File::from(unsafe { OwnedFd::from_raw_fd(fd) }); + let mut staged = Self { + dir, + tmp, + dest, + dest_path, + file, + published: false, + }; + staged + .file + .write_all(content) + .with_context(|| format!("write {}", staged.dest_path.display()))?; + staged + .file + .set_permissions(std::fs::Permissions::from_mode(mode)) + .with_context(|| format!("chmod {mode:o} {}", staged.dest_path.display()))?; + Ok(staged) + } + + /// The temp's open fd, for an `fchown` that must land before publishing. + fn file(&self) -> &std::fs::File { + &self.file + } + + /// Flush the content, rename the temp onto the final name, and flush the + /// directory: without the last step a crash can lose the rename itself. + fn publish(mut self) -> Result<()> { + self.file + .sync_all() + .with_context(|| format!("fsync {}", self.dest_path.display()))?; + let fd = self.dir.as_raw_fd(); + // SAFETY: `fd` is an open directory fd; both names are NUL-terminated. + if unsafe { libc::renameat(fd, self.tmp.as_ptr(), fd, self.dest.as_ptr()) } != 0 { + return Err(std::io::Error::last_os_error()) + .with_context(|| format!("publishing {}", self.dest_path.display())); + } + self.published = true; + self.dir + .sync_all() + .with_context(|| format!("fsync directory of {}", self.dest_path.display())) + } +} + +impl Drop for StagedFile<'_> { + fn drop(&mut self) { + if self.published { + return; + } + // SAFETY: `self.dir` is an open directory fd; `self.tmp` is + // NUL-terminated. + if unsafe { libc::unlinkat(self.dir.as_raw_fd(), self.tmp.as_ptr(), 0) } != 0 { + tracing::warn!( + dest = %self.dest_path.display(), + error = %std::io::Error::last_os_error(), + "failed to remove unpublished temp" + ); + } + } +} + +/// Publish `content` at `path` through a [`StagedFile`], at `mode` and, when +/// given, owned by `owner` (`(uid, gid)`). +fn publish_file(path: &Path, content: &[u8], mode: u32, owner: Option<(u32, u32)>) -> Result<()> { + let (Some(dir_path), Some(filename)) = + (path.parent(), path.file_name().and_then(|n| n.to_str())) + else { + bail!( + "publish_file: {} has no directory or file name", + path.display() + ); + }; + let dir = open_dir(dir_path)?; + let staged = StagedFile::write(&dir, dir_path, filename, content, mode)?; + if let Some((uid, gid)) = owner { + std::os::unix::fs::fchown(staged.file(), Some(uid), Some(gid)) + .with_context(|| format!("chown {}", path.display()))?; + } + staged.publish() } /// Sidecar written alongside an extra matrix account's token @@ -1469,8 +1666,7 @@ fn set_agent_paused(agent_name: &str, paused: bool) -> Result<(String, String)> /// /// `remove_file` unlinks the leaf itself and never follows a symlink, so an /// agent-planted link at the marker path cannot redirect this root unlink -/// at another file — the same threat `write_state_file_nofollow` closes on -/// the create side. +/// at another file — the same threat [`StagedFile`] closes on the create side. fn remove_marker_in(dir: &Path, filename: &str) -> Result<()> { let path = dir.join(filename); match std::fs::remove_file(&path) { @@ -1486,10 +1682,10 @@ fn remove_marker_in(dir: &Path, filename: &str) -> Result<()> { /// `harness/`) — both write into a directory owned by the agent, which is /// precisely why they need hive-priv at all. /// -/// The file is published by `rename`, so a reader woken by its appearance -/// cannot catch it empty or half-written: several of these paths have a -/// `systemd.path` unit watching them, and one of those triggers on the file -/// existing rather than changing. +/// The file is published through a [`StagedFile`], so a reader woken by its +/// appearance cannot catch it empty or half-written: several of these paths +/// have a `systemd.path` unit watching them, and one of those triggers on the +/// file existing rather than changing. fn write_agent_dir_file( agent_name: &str, dir: &Path, @@ -1499,8 +1695,7 @@ fn write_agent_dir_file( use std::os::fd::AsRawFd as _; use std::os::unix::fs::MetadataExt as _; - // The leaf is validated here as well as in `write_state_file_nofollow`: - // that call now sees the temp name, which is plain whatever `filename` is. + // Checked before `create_dir_all`, so a bad leaf does no filesystem work. ensure_plain_filename("write_agent_dir_file", filename)?; let state_dir = dir.to_path_buf(); @@ -1514,24 +1709,24 @@ fn write_agent_dir_file( std::fs::create_dir_all(&state_dir) .with_context(|| format!("create state dir {}", state_dir.display()))?; - // Security-critical: refuses a symlink the (state/-owning) agent may have - // planted at the leaf, so this root-privileged create/write/chmod/chown - // can't be redirected at an arbitrary file. See `write_state_file_nofollow`. + // Security-critical: the (state/-owning) agent may plant a symlink or + // hardlink in this dir, and `StagedFile` neither follows nor reuses one, + // so this root-privileged create/write/chmod/chown can't be redirected at + // an arbitrary file. let path = state_dir.join(filename); - let tmp_name = partial_name(filename); - let tmp_path = state_dir.join(&tmp_name); - let file = write_state_file_nofollow(&state_dir, &tmp_name, content)?; + let dir_fd = open_dir(&state_dir)?; + let staged = StagedFile::write(&dir_fd, &state_dir, filename, content.as_bytes(), 0o600)?; // Chown to the state dir's owner so the agent process can read the file. - // fchown on the same fd — TOCTOU-immune (the inode the write hit, never a + // fchown on the temp's fd — TOCTOU-immune (the inode the write hit, never a // swapped path). If stat fails (e.g. dir just created, owner is root), the // file stays root-owned and 0600 — still unreadable by others, just not // agent-readable. Log a warning so operators can diagnose. - match std::fs::metadata(&state_dir) { + match dir_fd.metadata() { Ok(meta) => { - // SAFETY: `file` is an open, owned fd live for the whole call; - // `fchown` only mutates that inode's uid/gid. - let rc = unsafe { libc::fchown(file.as_raw_fd(), meta.uid(), meta.gid()) }; + // SAFETY: the temp's fd is open and owned by `staged` for the whole + // call; `fchown` only mutates that inode's uid/gid. + let rc = unsafe { libc::fchown(staged.file().as_raw_fd(), meta.uid(), meta.gid()) }; if rc != 0 { let e = std::io::Error::last_os_error(); tracing::warn!( @@ -1551,12 +1746,9 @@ fn write_agent_dir_file( } } // After the chown, never before: the file must never be visible under its - // final name while still root-owned. Leaving the temp behind on failure - // would also leave a credential readable by nobody but root, so it goes. - if let Err(e) = std::fs::rename(&tmp_path, &path) { - let _ = std::fs::remove_file(&tmp_path); - return Err(e).with_context(|| format!("publishing {}", path.display())); - } + // final name while still root-owned. A failed publish drops `staged`, + // which removes the temp and the credential in it. + staged.publish()?; tracing::info!( agent = %agent_name, @@ -2966,6 +3158,7 @@ fn write_nspawn_flags( load_credentials: &[CredentialMount], ) -> Result<()> { use std::fmt::Write as _; + use std::os::unix::fs::MetadataExt as _; let path = format!("/etc/nixos-containers/{container}.conf"); let original = std::fs::read_to_string(&path).with_context(|| format!("read {path}"))?; let lines: Vec<&str> = original @@ -3025,7 +3218,15 @@ fn write_nspawn_flags( } let flags_joined = flags.join(" "); let _ = writeln!(out, "EXTRA_NSPAWN_FLAGS=\"{flags_joined}\""); - std::fs::write(&path, out).with_context(|| format!("write {path}"))?; + // Keeps the owner and mode `nixos-container` created the file with; this + // helper only rewrites its lines. + let meta = std::fs::metadata(&path).with_context(|| format!("stat {path}"))?; + publish_file( + Path::new(&path), + out.as_bytes(), + meta.mode() & 0o7777, + Some((meta.uid(), meta.gid())), + )?; // DNS marker for the in-container resolver oneshot: the oneshot only // rewrites the container's resolv.conf when this marker exists, and the @@ -3039,7 +3240,7 @@ fn write_nspawn_flags( } /// Filename of the in-container DNS marker, in the container's own `/etc`. -const BRIDGE_DNS_MARKER: &std::ffi::CStr = c"hyperhive-bridge-dns"; +const BRIDGE_DNS_MARKER: &str = "hyperhive-bridge-dns"; /// Write the bridge-DNS marker the `hyperhive-isolated-dns` oneshot keys /// off. The marker file contains just the gateway IP. Always written: @@ -3054,24 +3255,20 @@ fn write_bridge_dns_marker(container: &str, isolation: &NetworkIsolation) -> Res /// /// Root inside the container owns its `/etc`, and this write runs as host /// root, where an absolute symlink planted in the container resolves against -/// the host's `/`. So this opens `etc` with `O_DIRECTORY|O_NOFOLLOW` and the -/// leaf relative to that fd with `O_NOFOLLOW`: a symlink at either step fails -/// the write (`ELOOP`) instead of redirecting it. It also opens the leaf -/// `O_NONBLOCK` and requires a regular file, so a FIFO cannot hang the helper -/// and a device node cannot take the write. +/// the host's `/`. So this opens `etc` with `O_DIRECTORY|O_NOFOLLOW` (a +/// symlinked `etc` fails with `ELOOP`) and publishes the marker through a +/// [`StagedFile`] relative to that fd: whatever the container planted at the +/// leaf, symlink or FIFO or device node, is replaced by the rename, never +/// written through or opened. /// /// On a fresh install the container's `/etc` may not exist yet (nixos-container /// materialises the rootfs on the first start), so this creates it first; the /// dir and the marker persist through that start. Mode 0644: the gateway IP -/// is not a secret, and 0644 matches what the default 0022 umask gave the -/// earlier plain write. +/// is not a secret. fn write_bridge_dns_marker_in(rootfs: &Path, gateway_ip: &str) -> Result<()> { - use std::io::Write as _; - use std::os::unix::ffi::OsStrExt as _; - use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; + use std::os::unix::fs::OpenOptionsExt as _; let etc_path = rootfs.join("etc"); - let path = etc_path.join(std::ffi::OsStr::from_bytes(BRIDGE_DNS_MARKER.to_bytes())); // The rootfs dir is the container's `/`, which the container cannot // replace, so creating it by path is safe. std::fs::create_dir_all(rootfs) @@ -3102,41 +3299,15 @@ fn write_bridge_dns_marker_in(rootfs: &Path, gateway_ip: &str) -> Result<()> { .with_context(|| format!("open (no-follow) {}", etc_path.display())); } // SAFETY: `etc_fd` is a fresh, valid fd that nothing else owns. - let etc = unsafe { OwnedFd::from_raw_fd(etc_fd) }; - // SAFETY: `etc` is an open directory fd and the name a NUL-terminated - // literal; this checks the result before wrapping it. - let leaf_fd = unsafe { - libc::openat( - etc.as_raw_fd(), - BRIDGE_DNS_MARKER.as_ptr(), - libc::O_WRONLY - | libc::O_CREAT - | libc::O_TRUNC - | libc::O_NOFOLLOW - | libc::O_NONBLOCK - | libc::O_CLOEXEC, - 0o644 as libc::c_uint, - ) - }; - if leaf_fd < 0 { - return Err(std::io::Error::last_os_error()) - .with_context(|| format!("open (no-follow) {}", path.display())); - } - // SAFETY: `leaf_fd` is a fresh, valid fd that nothing else owns. - let mut file = std::fs::File::from(unsafe { OwnedFd::from_raw_fd(leaf_fd) }); - let is_file = file - .metadata() - .with_context(|| format!("stat {}", path.display()))? - .file_type() - .is_file(); - if !is_file { - bail!("{} is not a regular file", path.display()); - } - file.write_all(format!("{gateway_ip}\n").as_bytes()) - .with_context(|| format!("write bridge-DNS marker {}", path.display()))?; - file.set_permissions(std::fs::Permissions::from_mode(0o644)) - .with_context(|| format!("chmod 644 {}", path.display()))?; - Ok(()) + let etc = std::fs::File::from(unsafe { OwnedFd::from_raw_fd(etc_fd) }); + StagedFile::write( + &etc, + &etc_path, + BRIDGE_DNS_MARKER, + format!("{gateway_ip}\n").as_bytes(), + 0o644, + )? + .publish() } /// `SyncAgentTmpfiles` — write `/etc/tmpfiles.d/hyperhive-agents.conf` for @@ -3232,12 +3403,9 @@ async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, S } let content = agent_tmpfiles_content(agents); - // Atomic write: write to a tmp file then rename so a concurrent reader - // always sees a complete file. - let tmp = format!("{TMPFILES_PATH}.tmp"); - std::fs::write(&tmp, &content).with_context(|| format!("write {tmp}"))?; - std::fs::rename(&tmp, TMPFILES_PATH) - .with_context(|| format!("rename {TMPFILES_PATH}.tmp -> {TMPFILES_PATH}"))?; + // Overlapping syncs each stage their own temp, so the last rename wins + // with one complete roster rather than a mix of two. + publish_file(Path::new(TMPFILES_PATH), content.as_bytes(), 0o644, None)?; tracing::info!(agents = agents.len(), "tmpfiles.d: wrote {TMPFILES_PATH}"); // Apply immediately so dirs exist on the running host, not just after next boot. @@ -3259,13 +3427,13 @@ async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, S #[cfg(test)] mod tests { use super::{ - AgentTmpfilesEntry, BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, - agent_tmpfiles_content, check_fd_agreement, clear_runner_credentials, + AgentTmpfilesEntry, BindMount, BoundedRun, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, + StagedFile, agent_tmpfiles_content, check_fd_agreement, clear_runner_credentials, contains_secret_shaped_run, describe_forge_admin, ensure_plain_filename, git_overlay_flags, - limits_dropin_body, matrix_token_filename, open_export_dest, partial_name, - redact_secret_line, remove_marker_in, single_output_path, toplevel_attr, - validate_account_name, validate_credential_name, validate_snapshot_name, - write_agent_dir_file, write_bridge_dns_marker_in, write_state_file_nofollow, + limits_dropin_body, matrix_token_filename, open_dir, open_export_dest, partial_name, + publish_file, redact_secret_line, remove_marker_in, run_bounded, single_output_path, + toplevel_attr, validate_account_name, validate_credential_name, validate_snapshot_name, + write_agent_dir_file, write_bridge_dns_marker_in, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -3669,17 +3837,16 @@ mod tests { #[test] fn rejects_non_plain_filenames() { let dir = scratch(); + let dir_fd = open_dir(&dir).unwrap(); for bad in ["", ".", "..", "a/b", "/etc/passwd", "../escape", "sub/tok"] { assert!( - write_state_file_nofollow(&dir, bad, "x").is_err(), + StagedFile::write(&dir_fd, &dir, bad, b"x", 0o600).is_err(), "must reject filename {bad:?}" ); - // The wrapper needs its own check: the only name it hands to - // `write_state_file_nofollow` is the temp, which is plain whatever - // the caller passed. Pointing it at a directory that does not exist - // yet is what makes this arm bite -- a bad leaf fails eventually - // either way, at the rename, so the property worth pinning is that - // it fails BEFORE any root-privileged filesystem work. + // The wrapper's own check runs before its `create_dir_all`. + // Pointing it at a directory that does not exist yet is what makes + // this arm bite: the property worth pinning is that a bad leaf + // fails BEFORE any root-privileged filesystem work. let absent = dir.join("never-created"); assert!( write_agent_dir_file("a", &absent, bad, "x").is_err(), @@ -3786,65 +3953,143 @@ mod tests { "must fail AT the publish -- failing earlier leaves no temp to \ clean up and makes the next assertion vacuous. got: {err:#}" ); - assert!( - !dir.join(partial_name("forge-token")).exists(), + assert_eq!( + entries(&dir), + ["forge-token"], "temp still holds the secret after a failed publish" ); std::fs::remove_dir_all(&dir).ok(); } + /// Names in `dir`, sorted — the residue check every publish test ends on. + fn entries(dir: &PathBuf) -> Vec { + let mut v: Vec = std::fs::read_dir(dir) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + v.sort(); + v + } + + /// The agent plants a symlink where the token goes. The rename replaces + /// the link; the root-privileged write never reaches its target. #[test] - fn refuses_symlink_leaf_and_leaves_target_untouched() { + fn a_symlink_at_the_name_is_replaced_not_followed() { let dir = scratch(); let target = dir.join("target"); std::fs::write(&target, "original").unwrap(); - // Agent plants a symlink where the token would be written. std::os::unix::fs::symlink(&target, dir.join("forge-token")).unwrap(); - let res = write_state_file_nofollow(&dir, "forge-token", "PWNED"); - assert!(res.is_err(), "O_NOFOLLOW must refuse a symlink leaf"); - // The root-privileged write must NOT have followed the link. + write_agent_dir_file("a", &dir, "forge-token", "SECRET").unwrap(); assert_eq!( std::fs::read_to_string(&target).unwrap(), "original", "symlink target must be untouched" ); + let meta = std::fs::symlink_metadata(dir.join("forge-token")).unwrap(); + assert!(meta.file_type().is_file(), "the link must be replaced"); + assert_eq!( + std::fs::read_to_string(dir.join("forge-token")).unwrap(), + "SECRET" + ); std::fs::remove_dir_all(&dir).ok(); } + /// Any failure between staging and publishing drops the `StagedFile`. + /// Until the rename a reader sees only the old file, and afterwards the + /// old file is still whole and the temp is gone. #[test] - fn writes_plain_file_0600() { - use std::os::unix::fs::PermissionsExt as _; + fn a_failed_write_leaves_the_old_file_intact() { let dir = scratch(); - write_state_file_nofollow(&dir, "forge-token", "secret").unwrap(); - let path = dir.join("forge-token"); - assert_eq!(std::fs::read_to_string(&path).unwrap(), "secret"); - let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o600, "token file must be 0600"); - // Overwrite truncates cleanly (O_TRUNC), no residue. - write_state_file_nofollow(&dir, "forge-token", "new").unwrap(); - assert_eq!(std::fs::read_to_string(&path).unwrap(), "new"); + let dest = dir.join("hyperhive-limits.conf"); + std::fs::write(&dest, "OLD").unwrap(); + let dir_fd = open_dir(&dir).unwrap(); + + let staged = + StagedFile::write(&dir_fd, &dir, "hyperhive-limits.conf", b"NEW", 0o644).unwrap(); + assert_eq!(std::fs::read_to_string(&dest).unwrap(), "OLD"); + assert_eq!( + entries(&dir).len(), + 2, + "control: the temp exists while staged" + ); + drop(staged); + + assert_eq!(std::fs::read_to_string(&dest).unwrap(), "OLD"); + assert_eq!(entries(&dir), ["hyperhive-limits.conf"], "temp left behind"); std::fs::remove_dir_all(&dir).ok(); } - /// The container plants a symlink where the bridge-DNS marker goes. + /// Two overlapping writes of one file each own a temp, so whichever + /// publishes last lands whole — never the other's bytes, never a mix. #[test] - fn bridge_dns_marker_refuses_symlink_leaf() { + fn overlapping_writes_of_one_file_never_share_a_temp() { + let dir = scratch(); + let dir_fd = open_dir(&dir).unwrap(); + let name = "hyperhive-agents.conf"; + + let first = StagedFile::write(&dir_fd, &dir, name, b"roster A\n", 0o644).unwrap(); + let second = StagedFile::write(&dir_fd, &dir, name, b"roster B, longer\n", 0o644).unwrap(); + second.publish().unwrap(); + first.publish().unwrap(); + + assert_eq!( + std::fs::read_to_string(dir.join(name)).unwrap(), + "roster A\n" + ); + assert_eq!(entries(&dir), [name], "temp left behind"); + std::fs::remove_dir_all(&dir).ok(); + } + + /// The mode lands exactly as asked, whatever the umask would have made + /// of it (0666 under the usual 022 would otherwise come out 0644), and + /// the requested owner is on the file. + #[test] + fn publish_file_applies_mode_and_owner() { + use std::os::unix::fs::{MetadataExt as _, PermissionsExt as _}; + let dir = scratch(); + let path = dir.join("c.conf"); + let owner = std::fs::metadata(&dir).unwrap(); + + publish_file(&path, b"x\n", 0o666, Some((owner.uid(), owner.gid()))).unwrap(); + let meta = std::fs::metadata(&path).unwrap(); + assert_eq!(meta.permissions().mode() & 0o7777, 0o666); + assert_eq!((meta.uid(), meta.gid()), (owner.uid(), owner.gid())); + + publish_file(&path, b"y\n", 0o600, None).unwrap(); + let meta = std::fs::metadata(&path).unwrap(); + assert_eq!(meta.permissions().mode() & 0o7777, 0o600); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "y\n"); + assert_eq!(entries(&dir), ["c.conf"], "temp left behind"); + std::fs::remove_dir_all(&dir).ok(); + } + + /// The container plants a symlink where the bridge-DNS marker goes; the + /// rename replaces it without writing through it. + #[test] + fn bridge_dns_marker_replaces_symlink_leaf() { let dir = scratch(); let rootfs = dir.join("rootfs"); std::fs::create_dir_all(rootfs.join("etc")).unwrap(); let target = dir.join("target"); std::fs::write(&target, "original").unwrap(); - std::os::unix::fs::symlink(&target, rootfs.join("etc/hyperhive-bridge-dns")).unwrap(); + let leaf = rootfs.join("etc/hyperhive-bridge-dns"); + std::os::unix::fs::symlink(&target, &leaf).unwrap(); - let res = write_bridge_dns_marker_in(&rootfs, "10.0.0.1"); - assert!(res.is_err(), "O_NOFOLLOW must refuse a symlink leaf"); + write_bridge_dns_marker_in(&rootfs, "10.0.0.1").unwrap(); assert_eq!( std::fs::read_to_string(&target).unwrap(), "original", "symlink target must be untouched" ); + assert!( + std::fs::symlink_metadata(&leaf) + .unwrap() + .file_type() + .is_file() + ); + assert_eq!(std::fs::read_to_string(&leaf).unwrap(), "10.0.0.1\n"); std::fs::remove_dir_all(&dir).ok(); } @@ -3871,9 +4116,10 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } - /// A FIFO at the leaf fails the write rather than blocking the helper. + /// A FIFO at the leaf is replaced, never opened, so it cannot block the + /// helper. #[test] - fn bridge_dns_marker_refuses_fifo_leaf() { + fn bridge_dns_marker_replaces_fifo_leaf() { use std::os::unix::ffi::OsStrExt as _; let dir = scratch(); let rootfs = dir.join("rootfs"); @@ -3883,13 +4129,19 @@ mod tests { // SAFETY: `c_fifo` is a valid NUL-terminated path. assert_eq!(unsafe { libc::mkfifo(c_fifo.as_ptr(), 0o644) }, 0); - let res = write_bridge_dns_marker_in(&rootfs, "10.0.0.1"); - assert!(res.is_err(), "a FIFO leaf must be refused"); + write_bridge_dns_marker_in(&rootfs, "10.0.0.1").unwrap(); + assert!( + std::fs::symlink_metadata(&fifo) + .unwrap() + .file_type() + .is_file() + ); + assert_eq!(std::fs::read_to_string(&fifo).unwrap(), "10.0.0.1\n"); std::fs::remove_dir_all(&dir).ok(); } /// The control: a plain rootfs with no `etc` yet (a fresh install) gets - /// the dir and a 0644 marker, and a second write truncates. + /// the dir and a 0644 marker, and a second write replaces it. #[test] fn bridge_dns_marker_written_into_plain_etc() { use std::os::unix::fs::PermissionsExt as _; @@ -3908,6 +4160,74 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// Whether `pid` is gone, or only a zombie awaiting its reaper: either + /// way it is no longer running. + fn is_dead(pid: &str) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/stat")) { + Err(_) => true, + // The state letter follows the parenthesised command name. + Ok(stat) => stat + .rsplit_once(") ") + .is_some_and(|(_, rest)| rest.starts_with('Z')), + } + } + + /// A child still running at the limit comes back `TimedOut`, and the kill + /// reaches its whole process group: the backgrounded grandchild dies too, + /// not just the shell that started it. + #[tokio::test] + async fn a_build_past_its_limit_is_killed_with_its_group() { + use std::time::{Duration, Instant}; + let dir = scratch(); + let pidfile = dir.join("grandchild"); + let mut cmd = tokio::process::Command::new("sh"); + cmd.arg("-c") + .arg(format!("sleep 600 & echo $! > {}; wait", pidfile.display())); + + let started = Instant::now(); + let run = run_bounded(cmd, Duration::from_secs(2), None) + .await + .unwrap(); + assert!(matches!(run, BoundedRun::TimedOut), "must time out"); + assert!( + started.elapsed() < Duration::from_mins(1), + "the limit did not bound the run" + ); + + let pid = std::fs::read_to_string(&pidfile).unwrap(); + let pid = pid.trim(); + let deadline = Instant::now() + Duration::from_secs(5); + while !is_dead(pid) && Instant::now() < deadline { + tokio::time::sleep(Duration::from_millis(50)).await; + } + assert!(is_dead(pid), "grandchild {pid} outlived the timeout"); + std::fs::remove_dir_all(&dir).ok(); + } + + /// The control: a child that exits inside its limit is `Exited`, with its + /// status and both streams kept apart. + #[tokio::test] + async fn a_build_inside_its_limit_returns_its_output() { + let mut cmd = tokio::process::Command::new("sh"); + cmd.arg("-c") + .arg("echo progress >&2; echo /nix/store/abc-toplevel"); + match run_bounded(cmd, std::time::Duration::from_mins(1), None) + .await + .unwrap() + { + BoundedRun::Exited { + status, + stdout, + stderr, + } => { + assert!(status.success()); + assert_eq!(stdout, "/nix/store/abc-toplevel\n"); + assert_eq!(stderr, "progress"); + } + BoundedRun::TimedOut => panic!("a fast child must not time out"), + } + } + /// The runner-credential clear, in all three states that matter. The /// PRESENCE arm is the load-bearing one: an implementation that did nothing /// at all would pass the "absent is fine" arm perfectly, and the whole point @@ -3972,7 +4292,7 @@ mod tests { let path = dir.join(PAUSED_MARKER_FILE); for _ in 0..2 { - write_state_file_nofollow(&dir, PAUSED_MARKER_FILE, "").unwrap(); + write_agent_dir_file("a", &dir, PAUSED_MARKER_FILE, "").unwrap(); assert!(path.exists(), "marker must exist after pause"); assert_eq!(std::fs::read_to_string(&path).unwrap(), ""); }