From af476e613eca90e7da17de0b36e7d3748e67c4c2 Mon Sep 17 00:00:00 2001 From: YanLien <128586861+YanLien@users.noreply.github.com> Date: Thu, 23 Jul 2026 15:40:00 +0800 Subject: [PATCH] fix(ostool): harden local U-Boot TFTP staging lifecycle --- README.en.md | 24 ++ README.md | 21 ++ ostool/src/run/tftp.rs | 726 +++++++++++++++++++++++++++++++++++----- ostool/src/run/uboot.rs | 387 +++++++++++++++++++-- 4 files changed, 1045 insertions(+), 113 deletions(-) diff --git a/README.en.md b/README.en.md index 4def5406..2c3a74ee 100644 --- a/README.en.md +++ b/README.en.md @@ -501,6 +501,30 @@ ostool --workdir /path/to/kernel run qemu sudo setcap cap_net_bind_service=+eip $(which ostool) ``` +When using a system TFTP root on Linux (`/srv/tftp` by default), ostool stages +FIT images under `/srv/tftp/ostool` and removes the staging directory with the +regular user's permissions after each run. Configure a dedicated group for +this directory: + +```bash +# One-time setup +sudo groupadd ostool +sudo usermod -aG ostool "$USER" +sudo install -d -o root -g ostool -m 2775 /srv/tftp/ostool +``` + +After setup, log in again. Alternatively, start a shell with the new group in +the current terminal: + +```bash +newgrp ostool +``` + +Skip `groupadd` if the `ostool` group already exists. Run `newgrp ostool`, log in +again to activate the new group; restarting `tftpd-hpa` is not required. ostool +does not use `sudo rmdir` after a run; cleanup returns an error when the +directory does not grant group write access. + ### Debug Configuration ```toml diff --git a/README.md b/README.md index 2aa7dba1..b1e36702 100644 --- a/README.md +++ b/README.md @@ -498,6 +498,27 @@ ostool --workdir /path/to/kernel run qemu sudo setcap cap_net_bind_service=+eip $(which ostool) ``` +在 Linux 上使用系统 TFTP 根目录（默认 `/srv/tftp`）时，ostool 会在 +`/srv/tftp/ostool` 下暂存 FIT 镜像，并在任务结束后使用普通用户权限删除暂存目录。 +请为该目录配置专用用户组： + +```bash +# 首次配置 +sudo groupadd ostool +sudo usermod -aG ostool "$USER" +sudo install -d -o root -g ostool -m 2775 /srv/tftp/ostool +``` + +配置完成后，重新登录；也可以在当前终端启动一个已应用新用户组的 shell： + +```bash +newgrp ostool +``` + +如果 `ostool` 组已经存在，可以跳过 `groupadd`。执行 `newgrp ostool` 或重新登录后， +新用户组权限才会生效；不需要重启 `tftpd-hpa`。ostool 不会在任务结束时使用 +`sudo rmdir`；目录缺少组写权限时，清理操作会返回错误。 + ### 调试配置 ```toml diff --git a/ostool/src/run/tftp.rs b/ostool/src/run/tftp.rs index bd22ef81..41dbd48e 100644 --- a/ostool/src/run/tftp.rs +++ b/ostool/src/run/tftp.rs @@ -10,6 +10,7 @@ use std::{ net::{IpAddr, Ipv4Addr}, path::{Component, Path, PathBuf}, process::{Command, Stdio}, + sync::atomic::{AtomicU64, Ordering}, time::{SystemTime, UNIX_EPOCH}, }; @@ -21,13 +22,30 @@ use crate::{artifact::state::OutputArtifacts, utils::PathResultExt}; const TFTP_HPA_CONFIG_PATH: &str = "/etc/default/tftpd-hpa"; const DEFAULT_TFTP_DIRECTORY: &str = "/srv/tftp"; +const TFTP_NAMESPACE: &str = "ostool"; + +static STAGING_SEQUENCE: AtomicU64 = AtomicU64::new(0); #[derive(Debug, Clone, PartialEq, Eq)] pub struct LinuxTftpPrepared { - pub tftp_root: PathBuf, - pub target_dir: PathBuf, - pub absolute_fit_path: PathBuf, - pub relative_filename: String, + tftp_root: PathBuf, + target_dir: PathBuf, + absolute_fit_path: PathBuf, + relative_filename: String, +} + +impl LinuxTftpPrepared { + pub fn target_dir(&self) -> &Path { + &self.target_dir + } + + pub fn absolute_fit_path(&self) -> &Path { + &self.absolute_fit_path + } + + pub fn relative_filename(&self) -> &str { + &self.relative_filename + } } #[derive(Debug, Clone, PartialEq, Eq)] @@ -48,6 +66,12 @@ enum DistroKind { Unsupported, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LeafClaimOutcome { + Claimed, + Collision, +} + #[derive(Debug, Clone, PartialEq, Eq)] struct CommandSpec { program: String, @@ -140,22 +164,74 @@ pub fn stage_linux_fit_image( fitimage: &Path, tftp_root: &Path, ) -> anyhow::Result { - let prepared = prepare_linux_tftp_paths(fitimage, tftp_root)?; - ensure_tftp_target_dir(&prepared.target_dir)?; - fs::copy(fitimage, &prepared.absolute_fit_path).with_path("failed to copy file", fitimage)?; + validate_tftp_root(tftp_root)?; + let file_name = fitimage + .file_name() + .ok_or_else(|| anyhow!("invalid FIT image filename: {}", fitimage.display()))?; + let target_dir = claim_tftp_namespace(tftp_root)?; + let relative_dir = target_dir + .strip_prefix(tftp_root) + .with_context(|| { + format!( + "staged TFTP directory {} is outside root {}", + target_dir.display(), + tftp_root.display() + ) + })? + .to_path_buf(); + let relative_path = relative_dir.join(file_name); + let prepared = LinuxTftpPrepared { + tftp_root: tftp_root.to_path_buf(), + target_dir, + absolute_fit_path: tftp_root.join(&relative_path), + relative_filename: relative_path.to_string_lossy().replace('\\', "/"), + }; + if let Err(err) = fs::copy(fitimage, &prepared.absolute_fit_path) { + let copy_err = anyhow!(err).context(format!("failed to copy file {}", fitimage.display())); + if let Err(rollback_err) = cleanup_linux_tftp_staging(&prepared) { + return Err(copy_err.context(format!( + "failed to roll back TFTP staging directory {}: {rollback_err:#}", + prepared.target_dir.display() + ))); + } + return Err(copy_err); + } Ok(prepared) } -pub fn relative_tftp_filename(fitimage: &Path) -> anyhow::Result { - let artifact_dir = fitimage - .parent() - .ok_or_else(|| anyhow!("invalid FIT image path: {}", fitimage.display()))?; - let relative_path = relative_tftp_directory(artifact_dir)?.join( - fitimage - .file_name() - .ok_or_else(|| anyhow!("invalid FIT image filename: {}", fitimage.display()))?, - ); - Ok(relative_path.to_string_lossy().replace('\\', "/")) +pub fn cleanup_linux_tftp_staging(prepared: &LinuxTftpPrepared) -> anyhow::Result<()> { + validate_linux_tftp_prepared(prepared)?; + + match fs::remove_file(&prepared.absolute_fit_path) { + Ok(()) => {} + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(err) => { + return Err(err).with_path( + "failed to remove staged TFTP file", + &prepared.absolute_fit_path, + ); + } + } + + handle_tftp_staging_directory_removal( + &prepared.target_dir, + fs::remove_dir(&prepared.target_dir), + ) +} + +fn handle_tftp_staging_directory_removal( + target_dir: &Path, + result: io::Result<()>, +) -> anyhow::Result<()> { + match result { + Ok(()) => Ok(()), + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(()), + Err(err) if err.kind() == io::ErrorKind::PermissionDenied => Err(err).context(format!( + "failed to remove TFTP staging directory {}; configure group write access on its parent directory", + target_dir.display() + )), + Err(err) => Err(err).with_path("failed to remove TFTP staging directory", target_dir), + } } /// Starts a built-in TFTP server serving files from the build output directory. @@ -439,79 +515,271 @@ fn temp_file_path(prefix: &str) -> PathBuf { env::temp_dir().join(format!("{prefix}-{}-{nanos}.tmp", std::process::id())) } -fn prepare_linux_tftp_paths( - fitimage: &Path, - tftp_root: &Path, -) -> anyhow::Result { - let relative_filename = relative_tftp_filename(fitimage)?; - let relative_path = PathBuf::from(&relative_filename); - let relative_dir = relative_path - .parent() - .ok_or_else(|| anyhow!("invalid relative TFTP path: {}", relative_path.display()))? - .to_path_buf(); - let target_dir = tftp_root.join(&relative_dir); - let absolute_fit_path = tftp_root.join(&relative_path); - - Ok(LinuxTftpPrepared { - tftp_root: tftp_root.to_path_buf(), - target_dir, - absolute_fit_path, - relative_filename, - }) +fn validate_tftp_root(tftp_root: &Path) -> anyhow::Result<()> { + if !tftp_root.is_absolute() { + bail!("TFTP root must be absolute: {}", tftp_root.display()); + } + if tftp_root + .components() + .any(|component| component == Component::ParentDir) + { + bail!( + "TFTP root must not contain parent segments: {}", + tftp_root.display() + ); + } + Ok(()) } -fn relative_tftp_directory(artifact_dir: &Path) -> anyhow::Result { - if !artifact_dir.is_absolute() { +fn validate_linux_tftp_prepared(prepared: &LinuxTftpPrepared) -> anyhow::Result<()> { + validate_tftp_root(&prepared.tftp_root)?; + let namespace_parent = prepared.tftp_root.join(TFTP_NAMESPACE); + validate_tftp_namespace_parent(&namespace_parent)?; + if prepared.target_dir.parent() != Some(namespace_parent.as_path()) { bail!( - "artifact directory must be absolute for Linux system TFTP: {}", - artifact_dir.display() + "invalid TFTP staging directory outside generated namespace: {}", + prepared.target_dir.display() ); } + let leaf = prepared + .target_dir + .file_name() + .and_then(|value| value.to_str()) + .ok_or_else(|| { + anyhow!( + "invalid TFTP staging directory name: {}", + prepared.target_dir.display() + ) + })?; + if !is_generated_namespace_leaf(leaf) { + bail!("invalid generated TFTP staging directory name: {leaf}"); + } + if prepared.absolute_fit_path.parent() != Some(prepared.target_dir.as_path()) { + bail!( + "staged TFTP file is outside its generated directory: {}", + prepared.absolute_fit_path.display() + ); + } + let expected_relative = PathBuf::from(TFTP_NAMESPACE) + .join(leaf) + .join( + prepared + .absolute_fit_path + .file_name() + .ok_or_else(|| anyhow!("staged TFTP file has no filename"))?, + ) + .to_string_lossy() + .replace('\\', "/"); + if prepared.relative_filename != expected_relative { + bail!( + "staged TFTP relative filename does not match generated path: {}", + prepared.relative_filename + ); + } + match fs::symlink_metadata(&prepared.target_dir) { + Ok(metadata) if metadata.file_type().is_symlink() => bail!( + "refusing to clean symlinked TFTP staging directory: {}", + prepared.target_dir.display() + ), + Ok(metadata) if !metadata.is_dir() => bail!( + "TFTP staging target is not a directory: {}", + prepared.target_dir.display() + ), + Ok(_) => {} + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(err) => { + return Err(err).with_path( + "failed to inspect TFTP staging directory", + &prepared.target_dir, + ); + } + } + Ok(()) +} + +fn validate_tftp_namespace_parent(namespace_parent: &Path) -> anyhow::Result<()> { + match fs::symlink_metadata(namespace_parent) { + Ok(metadata) if metadata.file_type().is_symlink() => bail!( + "refusing to use symlinked TFTP namespace parent: {}", + namespace_parent.display() + ), + Ok(metadata) if !metadata.is_dir() => bail!( + "TFTP namespace parent is not a directory: {}", + namespace_parent.display() + ), + Ok(_) => Ok(()), + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(err).with_path("failed to inspect TFTP namespace parent", namespace_parent), + } +} + +fn is_generated_namespace_leaf(leaf: &str) -> bool { + let mut parts = leaf.split('-'); + let Some(pid) = parts.next() else { + return false; + }; + let Some(timestamp) = parts.next() else { + return false; + }; + let Some(sequence) = parts.next() else { + return false; + }; + parts.next().is_none() + && !pid.is_empty() + && pid.bytes().all(|byte| byte.is_ascii_digit()) + && !timestamp.is_empty() + && timestamp.bytes().all(|byte| byte.is_ascii_hexdigit()) + && !sequence.is_empty() + && sequence.bytes().all(|byte| byte.is_ascii_hexdigit()) +} + +fn staging_namespace_candidate() -> String { + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + let sequence = STAGING_SEQUENCE.fetch_add(1, Ordering::Relaxed); + format!("{}-{nanos:x}-{sequence:x}", std::process::id()) +} + +fn claim_tftp_namespace(tftp_root: &Path) -> anyhow::Result { + claim_tftp_namespace_with( + tftp_root, + std::iter::repeat_with(staging_namespace_candidate), + |command| run_privileged_command(command, false), + ) +} - let mut relative = PathBuf::from("ostool"); - for component in artifact_dir.components() { - match component { - Component::RootDir => {} - Component::Normal(part) => relative.push(part), - Component::CurDir => {} - Component::ParentDir => { - bail!( - "artifact directory must not contain parent segments: {}", - artifact_dir.display() - ) +fn claim_tftp_namespace_with( + tftp_root: &Path, + candidates: I, + mut run_privileged: F, +) -> anyhow::Result +where + I: IntoIterator, + F: FnMut(&CommandSpec) -> anyhow::Result<()>, +{ + validate_tftp_root(tftp_root)?; + let namespace_parent = tftp_root.join(TFTP_NAMESPACE); + ensure_tftp_namespace_parent(&namespace_parent, &mut run_privileged)?; + + for candidate in candidates { + validate_tftp_namespace_parent(&namespace_parent)?; + let target_dir = namespace_parent.join(candidate); + let outcome = match fs::create_dir(&target_dir) { + Ok(()) => LeafClaimOutcome::Claimed, + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => LeafClaimOutcome::Collision, + Err(err) if err.kind() == io::ErrorKind::PermissionDenied => { + claim_privileged_leaf_with_user(&target_dir, &mut run_privileged, effective_user)? + } + Err(err) => { + return Err(err).with_path("failed to claim TFTP staging directory", &target_dir); } - Component::Prefix(prefix) => relative.push(prefix.as_os_str()), + }; + if outcome == LeafClaimOutcome::Collision { + continue; } + return Ok(target_dir); } - Ok(relative) + + bail!("unable to claim a unique TFTP staging directory") } -fn ensure_tftp_target_dir(target_dir: &Path) -> anyhow::Result<()> { - match fs::create_dir_all(target_dir) { - Ok(()) => return Ok(()), +fn ensure_tftp_namespace_parent( + namespace_parent: &Path, + run_privileged: &mut F, +) -> anyhow::Result<()> +where + F: FnMut(&CommandSpec) -> anyhow::Result<()>, +{ + validate_tftp_namespace_parent(namespace_parent)?; + match fs::create_dir_all(namespace_parent) { + Ok(()) => return validate_tftp_namespace_parent(namespace_parent), Err(err) if err.kind() == io::ErrorKind::PermissionDenied => {} - Err(err) => return Err(err).with_path("failed to create directory", target_dir), + Err(err) => return Err(err).with_path("failed to create directory", namespace_parent), } - let user = effective_user()?; let mkdir = CommandSpec { program: "mkdir".into(), - args: vec!["-p".into(), target_dir.display().to_string()], + args: vec!["-p".into(), namespace_parent.display().to_string()], }; - run_privileged_command(&mkdir, false) - .with_context(|| format!("failed to create directory {}", target_dir.display()))?; + run_privileged(&mkdir).with_context(|| { + format!( + "failed to create TFTP namespace parent {}", + namespace_parent.display() + ) + })?; + validate_tftp_namespace_parent(namespace_parent) +} + +fn claim_privileged_leaf_with_user( + target_dir: &Path, + run_privileged: &mut F, + effective_user_lookup: U, +) -> anyhow::Result +where + F: FnMut(&CommandSpec) -> anyhow::Result<()>, + U: FnOnce() -> anyhow::Result, +{ + let user = effective_user_lookup().with_context(|| { + format!( + "failed to resolve effective user before claiming {}", + target_dir.display() + ) + })?; + let outcome = classify_privileged_leaf_claim(target_dir, &mut *run_privileged)?; + if outcome == LeafClaimOutcome::Collision { + return Ok(outcome); + } let chown = CommandSpec { program: "chown".into(), args: vec![ - "-R".into(), format!("{}:{}", user.name, user.group), target_dir.display().to_string(), ], }; - run_privileged_command(&chown, false) - .with_context(|| format!("failed to change ownership for {}", target_dir.display()))?; - Ok(()) + if let Err(err) = run_privileged(&chown) { + let primary = err.context(format!( + "failed to change ownership for {}", + target_dir.display() + )); + let rmdir = CommandSpec { + program: "rmdir".into(), + args: vec![target_dir.display().to_string()], + }; + return match run_privileged(&rmdir) { + Ok(()) => Err(primary), + Err(rollback_err) => Err(primary.context(format!( + "failed to roll back privileged TFTP directory {}: {rollback_err:#}", + target_dir.display() + ))), + }; + } + Ok(LeafClaimOutcome::Claimed) +} + +fn classify_privileged_leaf_claim( + target_dir: &Path, + mut run_privileged: F, +) -> anyhow::Result +where + F: FnMut(&CommandSpec) -> anyhow::Result<()>, +{ + let mkdir = CommandSpec { + program: "mkdir".into(), + args: vec![target_dir.display().to_string()], + }; + match run_privileged(&mkdir) { + Ok(()) => Ok(LeafClaimOutcome::Claimed), + Err(err) => match fs::symlink_metadata(target_dir) { + Ok(_) => Ok(LeafClaimOutcome::Collision), + Err(metadata_err) if metadata_err.kind() == io::ErrorKind::NotFound => Err(err), + Err(metadata_err) => { + Err(metadata_err).with_path("failed to inspect TFTP staging directory", target_dir) + } + }, + } } fn effective_user() -> anyhow::Result { @@ -653,6 +921,291 @@ impl DistroKind { mod tests { use super::*; + fn prepared_record(tftp_root: &Path, leaf: &str) -> LinuxTftpPrepared { + let target_dir = tftp_root.join(TFTP_NAMESPACE).join(leaf); + let absolute_fit_path = target_dir.join("image.fit"); + LinuxTftpPrepared { + tftp_root: tftp_root.to_path_buf(), + target_dir, + absolute_fit_path, + relative_filename: format!("{TFTP_NAMESPACE}/{leaf}/image.fit"), + } + } + + #[test] + fn stage_linux_fit_image_rejects_relative_tftp_root() { + let err = validate_tftp_root(Path::new("relative-tftp-root")).unwrap_err(); + + assert!(err.to_string().contains("TFTP root must be absolute")); + } + + #[test] + fn stage_linux_fit_image_rejects_parent_segments_in_tftp_root() { + let err = validate_tftp_root(Path::new("/tmp/tftp/../escape")).unwrap_err(); + + assert!(err.to_string().contains("must not contain parent segments")); + } + + #[test] + fn namespace_claim_retries_collision() { + let temp = tempfile::tempdir().unwrap(); + let namespace_parent = temp.path().join("ostool"); + fs::create_dir_all(namespace_parent.join("123-abc-0")).unwrap(); + + let claimed = claim_tftp_namespace_with( + temp.path(), + ["123-abc-0".to_string(), "123-abc-1".to_string()], + |_| Ok(()), + ) + .unwrap(); + + assert_eq!(claimed, namespace_parent.join("123-abc-1")); + assert!(namespace_parent.join("123-abc-0").is_dir()); + } + + #[test] + fn namespace_claim_classifies_privileged_collision() { + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("ostool").join("123-abc-0"); + fs::create_dir_all(target.parent().unwrap()).unwrap(); + + let outcome = classify_privileged_leaf_claim(&target, |command| { + assert_eq!(command.program, "mkdir"); + assert_eq!(command.args, vec![target.display().to_string()]); + fs::create_dir(&target).unwrap(); + Err(anyhow!("simulated mkdir collision")) + }) + .unwrap(); + + assert_eq!(outcome, LeafClaimOutcome::Collision); + } + + #[test] + fn namespace_claim_preserves_privileged_failure_without_leaf() { + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("ostool").join("123-abc-0"); + fs::create_dir_all(target.parent().unwrap()).unwrap(); + + let err = + classify_privileged_leaf_claim(&target, |_| Err(anyhow!("simulated mkdir failure"))) + .unwrap_err(); + + assert!(err.to_string().contains("simulated mkdir failure")); + assert!(!target.exists()); + } + + #[test] + fn namespace_claim_user_lookup_failure_happens_before_privileged_mkdir() { + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("ostool").join("123-abc-0"); + fs::create_dir_all(target.parent().unwrap()).unwrap(); + let mut command_calls = 0; + + let err = claim_privileged_leaf_with_user( + &target, + &mut |_| { + command_calls += 1; + Ok(()) + }, + || -> anyhow::Result { Err(anyhow!("simulated user lookup failure")) }, + ) + .unwrap_err(); + + assert!(format!("{err:#}").contains("simulated user lookup failure")); + assert_eq!(command_calls, 0); + assert!(!target.exists()); + } + + #[test] + fn namespace_claim_reports_chown_and_rollback_failures() { + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("ostool").join("123-abc-0"); + fs::create_dir_all(target.parent().unwrap()).unwrap(); + + let err = claim_privileged_leaf_with_user( + &target, + &mut |command| match command.program.as_str() { + "mkdir" => { + fs::create_dir(&target).unwrap(); + Ok(()) + } + "chown" => Err(anyhow!("simulated chown failure")), + "rmdir" => Err(anyhow!("simulated rollback failure")), + other => panic!("unexpected command: {other}"), + }, + || { + Ok(EffectiveUser { + name: "tester".into(), + group: "tester".into(), + }) + }, + ) + .unwrap_err(); + let message = format!("{err:#}"); + + assert!(message.contains("simulated chown failure")); + assert!(message.contains("simulated rollback failure")); + } + + #[cfg(target_os = "linux")] + #[test] + fn staging_copy_failure_rolls_back_claimed_leaf() { + let temp = tempfile::tempdir().unwrap(); + let source_directory = temp.path().join("source.fit"); + fs::create_dir(&source_directory).unwrap(); + let tftp_root = temp.path().join("tftp"); + + stage_linux_fit_image(&source_directory, &tftp_root).unwrap_err(); + + let namespace_parent = tftp_root.join(TFTP_NAMESPACE); + assert_eq!(fs::read_dir(namespace_parent).unwrap().count(), 0); + } + + #[cfg(target_os = "linux")] + #[test] + fn staging_invalid_fit_filename_does_not_claim_leaf() { + let temp = tempfile::tempdir().unwrap(); + let tftp_root = temp.path().join("tftp"); + + stage_linux_fit_image(Path::new("/"), &tftp_root).unwrap_err(); + + let namespace_parent = tftp_root.join(TFTP_NAMESPACE); + assert!(!namespace_parent.exists()); + } + + #[cfg(all(target_os = "linux", unix))] + #[test] + fn staging_rejects_symlinked_namespace_parent() { + use std::os::unix::fs::symlink; + + let temp = tempfile::tempdir().unwrap(); + let fitimage = temp.path().join("image.fit"); + fs::write(&fitimage, [1_u8]).unwrap(); + let tftp_root = temp.path().join("tftp"); + let outside = temp.path().join("outside"); + fs::create_dir_all(&tftp_root).unwrap(); + fs::create_dir_all(&outside).unwrap(); + symlink(&outside, tftp_root.join(TFTP_NAMESPACE)).unwrap(); + + let err = stage_linux_fit_image(&fitimage, &tftp_root).unwrap_err(); + + assert!(err.to_string().contains("symlinked TFTP namespace parent")); + assert_eq!(fs::read_dir(outside).unwrap().count(), 0); + } + + #[test] + fn cleanup_linux_tftp_removes_exact_file_and_leaf() { + let temp = tempfile::tempdir().unwrap(); + let prepared = prepared_record(temp.path(), "123-abc-0"); + fs::create_dir_all(&prepared.target_dir).unwrap(); + fs::write(&prepared.absolute_fit_path, [1_u8, 2, 3]).unwrap(); + + cleanup_linux_tftp_staging(&prepared).unwrap(); + + assert!(!prepared.absolute_fit_path.exists()); + assert!(!prepared.target_dir.exists()); + assert!(temp.path().join(TFTP_NAMESPACE).is_dir()); + } + + #[test] + fn cleanup_linux_tftp_accepts_already_absent_record() { + let temp = tempfile::tempdir().unwrap(); + let prepared = prepared_record(temp.path(), "123-abc-0"); + + cleanup_linux_tftp_staging(&prepared).unwrap(); + } + + #[test] + fn cleanup_linux_tftp_does_not_remove_unexpected_files() { + let temp = tempfile::tempdir().unwrap(); + let prepared = prepared_record(temp.path(), "123-abc-0"); + fs::create_dir_all(&prepared.target_dir).unwrap(); + fs::write(&prepared.absolute_fit_path, [1_u8]).unwrap(); + let unexpected = prepared.target_dir.join("keep.txt"); + fs::write(&unexpected, b"keep").unwrap(); + + let err = cleanup_linux_tftp_staging(&prepared).unwrap_err(); + + assert!( + err.to_string() + .contains("failed to remove TFTP staging directory") + ); + assert!(unexpected.exists()); + assert!(!prepared.absolute_fit_path.exists()); + } + + #[test] + fn cleanup_linux_tftp_rejects_fabricated_targets() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + let invalid_leaf = prepared_record(root, "user-data"); + assert!(cleanup_linux_tftp_staging(&invalid_leaf).is_err()); + + let shared_parent = LinuxTftpPrepared { + tftp_root: root.to_path_buf(), + target_dir: root.join(TFTP_NAMESPACE), + absolute_fit_path: root.join(TFTP_NAMESPACE).join("image.fit"), + relative_filename: format!("{TFTP_NAMESPACE}/image.fit"), + }; + assert!(cleanup_linux_tftp_staging(&shared_parent).is_err()); + + let nested = LinuxTftpPrepared { + tftp_root: root.to_path_buf(), + target_dir: root.join(TFTP_NAMESPACE).join("123-abc-0").join("nested"), + absolute_fit_path: root + .join(TFTP_NAMESPACE) + .join("123-abc-0") + .join("nested/image.fit"), + relative_filename: format!("{TFTP_NAMESPACE}/123-abc-0/nested/image.fit"), + }; + assert!(cleanup_linux_tftp_staging(&nested).is_err()); + } + + #[test] + fn cleanup_linux_tftp_rejects_invalid_roots() { + let relative = prepared_record(Path::new("relative-root"), "123-abc-0"); + assert!(cleanup_linux_tftp_staging(&relative).is_err()); + + let parent = prepared_record(Path::new("/tmp/tftp/../escape"), "123-abc-0"); + assert!(cleanup_linux_tftp_staging(&parent).is_err()); + } + + #[cfg(unix)] + #[test] + fn cleanup_linux_tftp_rejects_symlinked_namespace_parent() { + use std::os::unix::fs::symlink; + + let temp = tempfile::tempdir().unwrap(); + let outside = temp.path().join("outside"); + let outside_leaf = outside.join("123-abc-0"); + fs::create_dir_all(&outside_leaf).unwrap(); + let outside_file = outside_leaf.join("image.fit"); + fs::write(&outside_file, [1_u8]).unwrap(); + symlink(&outside, temp.path().join(TFTP_NAMESPACE)).unwrap(); + let prepared = prepared_record(temp.path(), "123-abc-0"); + + let err = cleanup_linux_tftp_staging(&prepared).unwrap_err(); + + assert!(err.to_string().contains("symlinked TFTP namespace parent")); + assert!(outside_file.exists()); + } + + #[test] + fn cleanup_linux_tftp_permission_denied_requests_group_write_access() { + let temp = tempfile::tempdir().unwrap(); + let prepared = prepared_record(temp.path(), "123-abc-0"); + + let err = handle_tftp_staging_directory_removal( + &prepared.target_dir, + Err(io::Error::from(io::ErrorKind::PermissionDenied)), + ) + .unwrap_err(); + + let message = err.to_string(); + assert!(message.contains("failed to remove TFTP staging directory")); + assert!(message.contains("group write access")); + } + #[test] fn distro_detection_uses_id_and_id_like() { let ubuntu = r#" @@ -717,39 +1270,32 @@ TFTP_OPTIONS="-l -s -c" assert_eq!(config.options.as_deref(), Some("-l -s -c")); } + #[cfg(target_os = "linux")] #[test] - fn relative_filename_keeps_absolute_artifact_hierarchy() { - let fitimage = - Path::new("/home/zhourui/opensource/tgoskits2/target/aarch64/release/image.fit"); - let prepared = prepare_linux_tftp_paths(fitimage, Path::new("/srv/tftp")).unwrap(); - + fn staged_filename_uses_short_unique_namespace() { + let temp = tempfile::tempdir().unwrap(); + let fitimage = temp.path().join("build/image.fit"); + fs::create_dir_all(fitimage.parent().unwrap()).unwrap(); + fs::write(&fitimage, [1_u8, 2, 3]).unwrap(); + let tftp_root = temp.path().join("tftp"); + let prepared = stage_linux_fit_image(&fitimage, &tftp_root).unwrap(); + + assert!(prepared.relative_filename.starts_with("ostool/")); + assert!(prepared.relative_filename.ends_with("/image.fit")); + assert!(prepared.relative_filename.len() < 96); assert_eq!( - prepared.relative_filename, - "ostool/home/zhourui/opensource/tgoskits2/target/aarch64/release/image.fit" + prepared.absolute_fit_path, + tftp_root.join(&prepared.relative_filename) ); assert_eq!( - prepared.target_dir, - PathBuf::from( - "/srv/tftp/ostool/home/zhourui/opensource/tgoskits2/target/aarch64/release" - ) - ); - assert_eq!( - prepared.absolute_fit_path, - PathBuf::from( - "/srv/tftp/ostool/home/zhourui/opensource/tgoskits2/target/aarch64/release/image.fit" - ) + prepared.target_dir.parent(), + Some(tftp_root.join("ostool").as_path()) ); } #[test] - fn relative_tftp_filename_keeps_ostool_prefix_for_existing_tftp_root() { - let fitimage = - Path::new("/home/zhourui/opensource/tgoskits2/target/aarch64/release/image.fit"); - - assert_eq!( - relative_tftp_filename(fitimage).unwrap(), - "ostool/home/zhourui/opensource/tgoskits2/target/aarch64/release/image.fit" - ); + fn generated_namespace_candidates_are_distinct() { + assert_ne!(staging_namespace_candidate(), staging_namespace_candidate()); } #[test] diff --git a/ostool/src/run/uboot.rs b/ostool/src/run/uboot.rs index 747c7e19..e5f9b720 100644 --- a/ostool/src/run/uboot.rs +++ b/ostool/src/run/uboot.rs @@ -72,8 +72,6 @@ pub struct UbootConfig { /// Address passed to `bootm` after serial FIT upload. /// if not specified, use the FIT load address when configured. pub bootm_addr: Option, - /// TFTP boot configuration - pub net: Option, /// Board reset command /// shell command to reset the board pub board_reset_cmd: Option, @@ -498,6 +496,11 @@ struct ConsoleTransport { rx: BoxedAsyncRead, } +struct SecondaryRunFailure { + phase: &'static str, + error: anyhow::Error, +} + #[derive(Debug, Clone, Default)] struct ResolvedRuntime { server_ip: Option, @@ -544,6 +547,7 @@ struct LocalBackend { config: LocalUbootConfig, baud_rate: Option, linux_system_tftp: Option, + linux_tftp_staging: Vec, existing_tftp_dir: Option, builtin_tftp_started: bool, } @@ -554,6 +558,7 @@ impl LocalBackend { config, baud_rate: None, linux_system_tftp: None, + linux_tftp_staging: Vec::new(), existing_tftp_dir: None, builtin_tftp_started: false, } @@ -717,11 +722,13 @@ impl RunnerBackend for LocalBackend { { if let Some(system_tftp) = self.linux_system_tftp.as_ref() { let prepared = tftp::stage_linux_fit_image(fitimage, &system_tftp.directory)?; + let relative_filename = prepared.relative_filename().to_string(); info!( "Staged FIT image to: {}", - prepared.absolute_fit_path.display() + prepared.absolute_fit_path().display() ); - return Ok(StagedBootArtifact::network(prepared.relative_filename)); + self.linux_tftp_staging.push(prepared); + return Ok(StagedBootArtifact::network(relative_filename)); } } @@ -744,13 +751,29 @@ impl RunnerBackend for LocalBackend { } async fn after_run(&mut self, context: &ProcessContext) -> anyhow::Result<()> { + let mut cleanup_failures = Vec::new(); + for prepared in self.linux_tftp_staging.drain(..) { + let target = prepared.target_dir().display().to_string(); + if let Err(err) = tftp::cleanup_linux_tftp_staging(&prepared) { + cleanup_failures.push(format!("{target}: {err:#}")); + } + } + if let Some(cmd) = self.config.board_power_off_cmd.as_deref() && !cmd.trim().is_empty() && let Err(err) = crate::process::shell_run_cmd(context, cmd) { log::warn!("board power-off command failed: {err:#}"); } - Ok(()) + + if cleanup_failures.is_empty() { + Ok(()) + } else { + Err(anyhow!( + "failed to clean local TFTP staging:\n{}", + cleanup_failures.join("\n") + )) + } } } @@ -1010,6 +1033,61 @@ impl RunnerBackend for RemoteBackend { } } +async fn finalize_backend_run( + backend: &mut B, + context: &ProcessContext, + run_result: anyhow::Result<()>, +) -> anyhow::Result<()> { + let console_cleanup = backend.finish_console().await; + let post_run_cleanup = backend.after_run(context).await; + let (result, secondary_failures) = + select_runner_result(run_result, console_cleanup, post_run_cleanup); + + for failure in secondary_failures { + log::warn!("backend {} failed: {:#}", failure.phase, failure.error); + } + result +} + +fn select_runner_result( + run_result: anyhow::Result<()>, + console_cleanup: anyhow::Result<()>, + post_run_cleanup: anyhow::Result<()>, +) -> (anyhow::Result<()>, Vec) { + match run_result { + Err(primary) => { + let mut secondary = Vec::new(); + if let Err(error) = console_cleanup { + secondary.push(SecondaryRunFailure { + phase: "console cleanup", + error, + }); + } + if let Err(error) = post_run_cleanup { + secondary.push(SecondaryRunFailure { + phase: "post-run cleanup", + error, + }); + } + (Err(primary), secondary) + } + Ok(()) => match console_cleanup { + Err(primary) => { + let secondary = post_run_cleanup + .err() + .map(|error| SecondaryRunFailure { + phase: "post-run cleanup", + error, + }) + .into_iter() + .collect(); + (Err(primary), secondary) + } + Ok(()) => (post_run_cleanup, Vec::new()), + }, + } +} + impl Runner where B: RunnerBackend, @@ -1026,22 +1104,7 @@ where async fn run(&mut self) -> anyhow::Result<()> { let run_result = self._run().await; - - if let Err(err) = self.backend.finish_console().await { - if run_result.is_ok() { - return Err(err); - } - log::warn!("backend console cleanup failed: {err:#}"); - } - - if let Err(err) = self.backend.after_run(&self.input.process_context).await { - if run_result.is_ok() { - return Err(err); - } - log::warn!("backend post-run cleanup failed: {err:#}"); - } - - run_result + finalize_backend_run(&mut self.backend, &self.input.process_context, run_result).await } async fn _run(&mut self) -> anyhow::Result<()> { @@ -1469,6 +1532,8 @@ fn bootm_command(bootm_arg: Option) -> String { mod tests { use std::{collections::HashMap, time::Duration}; + use async_trait::async_trait; + use super::{ LocalBackend, LocalUbootConfig, Net, RemoteBackend, ResolvedRuntime, RunnerBackend, UbootConfig, build_network_boot_request, ensure_config_in_dir, fit_artifact_path, @@ -1511,6 +1576,7 @@ mod tests { config: LocalUbootConfig::default(), baud_rate: None, linux_system_tftp: None, + linux_tftp_staging: Vec::new(), existing_tftp_dir: None, builtin_tftp_started: false, } @@ -1523,6 +1589,143 @@ mod tests { fit_path } + struct CleanupTrackingBackend { + finish_calls: usize, + after_calls: usize, + finish_error: Option<&'static str>, + after_error: Option<&'static str>, + } + + #[async_trait] + impl RunnerBackend for CleanupTrackingBackend { + async fn resolve_runtime( + &mut self, + _input: &super::UbootRunInput, + _config: &UbootConfig, + ) -> anyhow::Result { + unreachable!() + } + + async fn prepare_dtb( + &mut self, + _input: &super::UbootRunInput, + _config: &UbootConfig, + ) -> anyhow::Result { + unreachable!() + } + + async fn open_console(&mut self) -> anyhow::Result { + unreachable!() + } + + async fn after_console_open( + &mut self, + _context: &crate::process::ProcessContext, + ) -> anyhow::Result<()> { + unreachable!() + } + + async fn stage_fit_image( + &mut self, + _fit_artifact: &BootArtifact, + _runtime: &ResolvedRuntime, + ) -> anyhow::Result { + unreachable!() + } + + async fn finish_console(&mut self) -> anyhow::Result<()> { + self.finish_calls += 1; + match self.finish_error { + Some(message) => Err(anyhow!(message)), + None => Ok(()), + } + } + + async fn after_run( + &mut self, + _context: &crate::process::ProcessContext, + ) -> anyhow::Result<()> { + self.after_calls += 1; + match self.after_error { + Some(message) => Err(anyhow!(message)), + None => Ok(()), + } + } + } + + #[tokio::test] + async fn runner_cleanup_invokes_after_run_when_console_cleanup_fails() { + let temp = tempfile::tempdir().unwrap(); + write_single_crate_manifest(temp.path()); + let context = make_invocation(temp.path()).process_context().unwrap(); + let mut backend = CleanupTrackingBackend { + finish_calls: 0, + after_calls: 0, + finish_error: Some("console cleanup failed"), + after_error: Some("post-run cleanup failed"), + }; + + let err = super::finalize_backend_run(&mut backend, &context, Ok(())) + .await + .unwrap_err(); + + assert_eq!(backend.finish_calls, 1); + assert_eq!(backend.after_calls, 1); + assert!(err.to_string().contains("console cleanup failed")); + } + + #[test] + fn runner_cleanup_error_precedence_covers_all_combinations() { + for mask in 0_u8..8 { + let run_fails = mask & 0b100 != 0; + let finish_fails = mask & 0b010 != 0; + let after_fails = mask & 0b001 != 0; + let result = |fails: bool, message: &'static str| { + if fails { Err(anyhow!(message)) } else { Ok(()) } + }; + + let (primary, secondary) = super::select_runner_result( + result(run_fails, "run failed"), + result(finish_fails, "console cleanup failed"), + result(after_fails, "post-run cleanup failed"), + ); + + let expected_primary = if run_fails { + Some("run failed") + } else if finish_fails { + Some("console cleanup failed") + } else if after_fails { + Some("post-run cleanup failed") + } else { + None + }; + assert_eq!( + primary.as_ref().err().map(ToString::to_string).as_deref(), + expected_primary, + "mask {mask:03b}" + ); + + let phases = secondary + .iter() + .map(|failure| failure.phase) + .collect::>(); + let expected_phases = if run_fails { + [ + finish_fails.then_some("console cleanup"), + after_fails.then_some("post-run cleanup"), + ] + .into_iter() + .flatten() + .collect::>() + } else if finish_fails && after_fails { + vec!["post-run cleanup"] + } else { + Vec::new() + }; + assert_eq!(phases, expected_phases, "mask {mask:03b}"); + } + } + fn prepare_elf_only_invocation(dir: &std::path::Path) -> Invocation { let source = std::env::current_exe().unwrap(); let copied = dir.join("sample-elf"); @@ -1718,8 +1921,9 @@ mod tests { .await .unwrap(); - let relative_filename = tftp::relative_tftp_filename(&fit_path).unwrap(); - assert_eq!(staged.bootfile(), Some(relative_filename.as_str())); + let relative_filename = staged.bootfile().unwrap(); + assert!(relative_filename.starts_with("ostool/")); + assert!(relative_filename.ends_with("/image.fit")); assert!(staged.network_transfer_ready()); assert_eq!( std::fs::read(tftp_root.join(relative_filename)).unwrap(), @@ -1727,6 +1931,124 @@ mod tests { ); } + #[cfg(target_os = "linux")] + #[tokio::test] + async fn local_backend_cleans_staged_linux_tftp_image() { + let temp = tempfile::tempdir().unwrap(); + write_single_crate_manifest(temp.path()); + let fit_path = write_fit_image(temp.path()); + let tftp_root = temp.path().join("tftp-root"); + let mut backend = local_backend(); + backend.linux_system_tftp = Some(tftp::TftpdHpaConfig { + username: None, + directory: tftp_root, + address: None, + options: None, + }); + + backend + .stage_fit_image( + &BootArtifact::fit_image(&fit_path), + &ResolvedRuntime::default(), + ) + .await + .unwrap(); + + assert_eq!(backend.linux_tftp_staging.len(), 1); + let staged_file = backend.linux_tftp_staging[0] + .absolute_fit_path() + .to_path_buf(); + let staged_dir = backend.linux_tftp_staging[0].target_dir().to_path_buf(); + let context = make_invocation(temp.path()).process_context().unwrap(); + backend.after_run(&context).await.unwrap(); + + assert!(!staged_file.exists()); + assert!(!staged_dir.exists()); + assert!(backend.linux_tftp_staging.is_empty()); + } + + #[cfg(target_os = "linux")] + #[tokio::test] + async fn local_backend_cleanup_continues_after_error() { + let temp = tempfile::tempdir().unwrap(); + write_single_crate_manifest(temp.path()); + let tftp_root = temp.path().join("tftp-root"); + let first_source = write_fit_image(temp.path()); + let second_source = temp.path().join("other.fit"); + std::fs::write(&second_source, [5_u8, 6]).unwrap(); + let first = tftp::stage_linux_fit_image(&first_source, &tftp_root).unwrap(); + let second = tftp::stage_linux_fit_image(&second_source, &tftp_root).unwrap(); + let unexpected = first.target_dir().join("keep.txt"); + std::fs::write(&unexpected, b"keep").unwrap(); + let second_file = second.absolute_fit_path().to_path_buf(); + let second_dir = second.target_dir().to_path_buf(); + let mut backend = local_backend(); + backend.linux_tftp_staging = vec![first, second]; + + let context = make_invocation(temp.path()).process_context().unwrap(); + let err = backend.after_run(&context).await.unwrap_err(); + + assert!( + err.to_string() + .contains("failed to clean local TFTP staging") + ); + assert!(unexpected.exists()); + assert!(!second_file.exists()); + assert!(!second_dir.exists()); + assert!(backend.linux_tftp_staging.is_empty()); + } + + #[cfg(target_os = "linux")] + #[tokio::test] + async fn local_backend_cleanup_reports_every_failure() { + let temp = tempfile::tempdir().unwrap(); + write_single_crate_manifest(temp.path()); + let tftp_root = temp.path().join("tftp-root"); + let first_source = write_fit_image(temp.path()); + let second_source = temp.path().join("other.fit"); + std::fs::write(&second_source, [5_u8, 6]).unwrap(); + let first = tftp::stage_linux_fit_image(&first_source, &tftp_root).unwrap(); + let second = tftp::stage_linux_fit_image(&second_source, &tftp_root).unwrap(); + std::fs::write(first.target_dir().join("keep.txt"), b"keep").unwrap(); + std::fs::remove_file(second.absolute_fit_path()).unwrap(); + std::fs::create_dir(second.absolute_fit_path()).unwrap(); + let first_dir = first.target_dir().display().to_string(); + let second_file = second.absolute_fit_path().display().to_string(); + let mut backend = local_backend(); + backend.linux_tftp_staging = vec![first, second]; + + let context = make_invocation(temp.path()).process_context().unwrap(); + let err = backend.after_run(&context).await.unwrap_err(); + let message = format!("{err:#}"); + + assert!(message.contains(&first_dir)); + assert!(message.contains(&second_file)); + assert!(message.contains("failed to remove TFTP staging directory")); + assert!(message.contains("failed to remove staged TFTP file")); + assert!(backend.linux_tftp_staging.is_empty()); + } + + #[cfg(target_os = "linux")] + #[tokio::test] + async fn local_backend_power_off_runs_after_cleanup_failure() { + let temp = tempfile::tempdir().unwrap(); + write_single_crate_manifest(temp.path()); + let tftp_root = temp.path().join("tftp-root"); + let fit_path = write_fit_image(temp.path()); + let prepared = tftp::stage_linux_fit_image(&fit_path, &tftp_root).unwrap(); + std::fs::write(prepared.target_dir().join("keep.txt"), b"keep").unwrap(); + let marker = temp.path().join("powered-off"); + let mut backend = local_backend(); + backend.config.board_power_off_cmd = Some(format!("touch {}", marker.display())); + backend.linux_tftp_staging.push(prepared); + + let context = make_invocation(temp.path()).process_context().unwrap(); + backend.after_run(&context).await.unwrap_err(); + + assert!(marker.exists()); + assert!(backend.linux_tftp_staging.is_empty()); + } + #[tokio::test] async fn remote_stage_fit_image_without_tftp_disables_transfer() { let temp = tempfile::tempdir().unwrap(); @@ -1826,6 +2148,25 @@ timeout = 0 assert_eq!(config.timeout, Some(0)); } + #[test] + fn uboot_config_parses_network_into_local_backend() { + let config: UbootConfig = toml::from_str( + r#" +serial = "/dev/null" +baud_rate = "115200" +success_regex = [] +fail_regex = [] + +[net] +interface = "eth0" +"#, + ) + .unwrap(); + + let net = config.local.net.unwrap(); + assert_eq!(net.interface, "eth0"); + } + #[test] fn uboot_config_replaces_string_fields() { let tmp = tempfile::tempdir().unwrap();