From 176738e931e99d7915a026607fd0c84fdad155e1 Mon Sep 17 00:00:00 2001 From: SFG545 Date: Mon, 13 Jul 2026 05:02:22 -0500 Subject: [PATCH] feat: implement preflight checks for file ownership and order in package transactions --- src/commands.rs | 230 +++++++++++++++++++++++++++++++++++++----- src/commands/tests.rs | 117 +++++++++++++++++++++ src/db/mod.rs | 36 ++++++- 3 files changed, 358 insertions(+), 25 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index f7fbcd6..cc61e24 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1292,6 +1292,163 @@ fn run_transaction_hooks_for_plans( install::hooks::run_transaction_hooks_batch(rootfs, phase, &contexts) } +fn preflight_file_ownership_and_order( + plans: &[PlannedPackageInstall], + pre_removed_packages: &HashSet, + rootfs: &Path, + config: &config::Config, +) -> Result> { + let db_path = config.installed_db_path(rootfs); + let installed_ownership = db::get_file_ownership(&db_path)?; + let mut manifests = Vec::with_capacity(plans.len()); + let mut plan_by_package = BTreeMap::new(); + let mut replacement_plan_by_package = BTreeMap::new(); + let mut violations = BTreeSet::new(); + + for (idx, plan) in plans.iter().enumerate() { + let package = &plan.spec.package.name; + if plan_by_package.insert(package.clone(), idx).is_some() { + violations.insert(format!( + "package '{}' appears more than once in the transaction", + package + )); + } + for replaced in &plan.staged.replacement_removals { + if replacement_plan_by_package + .insert(replaced.clone(), idx) + .is_some() + { + violations.insert(format!( + "installed package '{}' is replaced by more than one transaction package", + replaced + )); + } + } + if let Some(transition) = &plan.staged.renamed_transition + && replacement_plan_by_package + .insert(transition.replaced.name.clone(), idx) + .is_some() + { + violations.insert(format!( + "installed package '{}' is replaced by more than one transaction package", + transition.replaced.name + )); + } + + let manifest = staging::generate_manifest_with_dirs(&plan.destdir).with_context(|| { + format!( + "Failed to inspect staged files for package '{}'", + plan.spec.package.name + ) + })?; + manifests.push(manifest.files.into_iter().collect::>()); + } + + let mut planned_owner_by_path: BTreeMap<&str, usize> = BTreeMap::new(); + for (idx, manifest) in manifests.iter().enumerate() { + for path in manifest { + if let Some(previous_idx) = planned_owner_by_path.insert(path, idx) + && plans[previous_idx].spec.package.name != plans[idx].spec.package.name + && !db::should_auto_clear_conflict(&plans[previous_idx].spec.package.name, path) + { + violations.insert(format!( + "{} -> provided by both {} and {}", + path, plans[previous_idx].spec.package.name, plans[idx].spec.package.name + )); + } + } + } + + let mut edges = vec![BTreeSet::new(); plans.len()]; + let mut indegree = vec![0_usize; plans.len()]; + for (taker_idx, manifest) in manifests.iter().enumerate() { + let taker = &plans[taker_idx].spec.package.name; + for path in manifest { + let Some(owner) = installed_ownership.get(path) else { + continue; + }; + if owner == taker + || pre_removed_packages.contains(owner) + || db::should_auto_clear_conflict(owner, path) + { + continue; + } + + let owner_plan_idx = if let Some(owner_idx) = plan_by_package.get(owner) { + if manifests[*owner_idx].contains(path) { + violations.insert(format!( + "{} -> owned by {} and still provided by its transaction update (wanted by {})", + path, owner, taker + )); + continue; + } + Some(*owner_idx) + } else if let Some(owner_idx) = replacement_plan_by_package.get(owner) { + let retained_by_rename = plans[*owner_idx] + .staged + .renamed_transition + .as_ref() + .is_some_and(|transition| { + transition.replaced.name == *owner + && transition.retained_files.contains(path) + }); + if retained_by_rename { + violations.insert(format!( + "{} -> retained by renamed package {} (wanted by {})", + path, owner, taker + )); + continue; + } + Some(*owner_idx) + } else { + violations.insert(format!( + "{} -> owned by {} (wanted by {})", + path, owner, taker + )); + None + }; + + if let Some(owner_idx) = owner_plan_idx + && owner_idx != taker_idx + && edges[owner_idx].insert(taker_idx) + { + indegree[taker_idx] += 1; + } + } + } + + if !violations.is_empty() { + let mut message = String::from("File ownership conflict detected before transaction:\n"); + for violation in violations { + message.push_str(&format!(" {violation}\n")); + } + anyhow::bail!(message); + } + + let mut order = Vec::with_capacity(plans.len()); + let mut emitted = vec![false; plans.len()]; + while order.len() < plans.len() { + let Some(next) = (0..plans.len()).find(|idx| !emitted[*idx] && indegree[*idx] == 0) else { + let packages = (0..plans.len()) + .filter(|idx| !emitted[*idx]) + .map(|idx| plans[idx].spec.package.name.as_str()) + .collect::>() + .join(", "); + anyhow::bail!( + "File ownership handoff cycle detected before transaction among: {}", + packages + ); + }; + emitted[next] = true; + order.push(next); + for dependent in &edges[next] { + indegree[*dependent] -= 1; + } + } + + Ok(order.into_iter().map(|idx| plans[idx].clone()).collect()) +} + fn install_staged_to_rootfs( pkg_spec: &package::PackageSpec, destdir: &Path, @@ -1393,6 +1550,24 @@ fn install_planned_packages_to_rootfs_with_pre_removed( config: &config::Config, pre_removed_packages: &HashSet, show_progress: bool, +) -> Result<()> { + let ordered_plans = + preflight_file_ownership_and_order(plans, pre_removed_packages, rootfs, config)?; + install_preflighted_planned_packages_to_rootfs_with_pre_removed( + &ordered_plans, + rootfs, + config, + pre_removed_packages, + show_progress, + ) +} + +fn install_preflighted_planned_packages_to_rootfs_with_pre_removed( + plans: &[PlannedPackageInstall], + rootfs: &Path, + config: &config::Config, + pre_removed_packages: &HashSet, + show_progress: bool, ) -> Result<()> { let mut removed_replacements = HashSet::new(); let mut pending_post_hooks = Vec::new(); @@ -1510,6 +1685,8 @@ fn install_package_outputs_to_rootfs( config: &config::Config, ) -> Result> { let plans = plan_package_outputs_for_install(pkg_spec, destdir, rootfs, config)?; + let ordered_plans = + preflight_file_ownership_and_order(&plans, &HashSet::new(), rootfs, config)?; let installed = plans .iter() .map(|plan| InstalledPackageOutcome { @@ -1517,9 +1694,15 @@ fn install_package_outputs_to_rootfs( is_update: plan.staged.is_update, }) .collect(); - run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Pre, &plans)?; - install_planned_packages_to_rootfs(&plans, rootfs, config)?; - run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Post, &plans)?; + run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Pre, &ordered_plans)?; + install_preflighted_planned_packages_to_rootfs_with_pre_removed( + &ordered_plans, + rootfs, + config, + &HashSet::new(), + true, + )?; + run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Post, &ordered_plans)?; Ok(installed) } @@ -2183,17 +2366,7 @@ fn run_direct_archive_install_requests( transaction_plans.extend(output_plans); } - run_transaction_hooks_for_plans( - options.rootfs, - install::hooks::HookPhase::Pre, - &transaction_plans, - )?; - install_planned_packages_to_rootfs(&transaction_plans, options.rootfs, config)?; - run_transaction_hooks_for_plans( - options.rootfs, - install::hooks::HookPhase::Post, - &transaction_plans, - )?; + install_direct_transaction(&transaction_plans, options.rootfs, config)?; Ok(true) } @@ -2624,9 +2797,16 @@ fn install_direct_transaction( rootfs: &Path, config: &config::Config, ) -> Result<()> { - run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Pre, plans)?; - install_planned_packages_to_rootfs(plans, rootfs, config)?; - run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Post, plans)?; + let ordered_plans = preflight_file_ownership_and_order(plans, &HashSet::new(), rootfs, config)?; + run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Pre, &ordered_plans)?; + install_preflighted_planned_packages_to_rootfs_with_pre_removed( + &ordered_plans, + rootfs, + config, + &HashSet::new(), + true, + )?; + run_transaction_hooks_for_plans(rootfs, install::hooks::HookPhase::Post, &ordered_plans)?; Ok(()) } @@ -2717,7 +2897,13 @@ fn install_update_transaction( rootfs: &Path, config: &config::Config, ) -> Result<()> { - let contexts = transaction_contexts_for_update(removals, plans); + let pre_removed_packages: HashSet = removals + .iter() + .map(|removal| removal.package.clone()) + .collect(); + let ordered_plans = + preflight_file_ownership_and_order(plans, &pre_removed_packages, rootfs, config)?; + let contexts = transaction_contexts_for_update(removals, &ordered_plans); install::hooks::run_transaction_hooks_batch(rootfs, install::hooks::HookPhase::Pre, &contexts)?; for removal in removals { @@ -2729,12 +2915,8 @@ fn install_update_transaction( )?; } - let pre_removed_packages: HashSet = removals - .iter() - .map(|removal| removal.package.clone()) - .collect(); - install_planned_packages_to_rootfs_with_pre_removed( - plans, + install_preflighted_planned_packages_to_rootfs_with_pre_removed( + &ordered_plans, rootfs, config, &pre_removed_packages, diff --git a/src/commands/tests.rs b/src/commands/tests.rs index cf19b44..c0adfe7 100644 --- a/src/commands/tests.rs +++ b/src/commands/tests.rs @@ -1158,6 +1158,123 @@ fn plan_staged_install_reads_updates_from_rootfs_installed_db() -> Result<()> { Ok(()) } +fn file_ownership_test_spec(name: &str, version: &str) -> package::PackageSpec { + let mut spec = test_package_spec(package::BuildType::Bin, None, &[]); + spec.package.name = name.to_string(); + spec.package.version = version.to_string(); + spec.source.clear(); + spec +} + +fn stage_file(destdir: &Path, path: &str, contents: &str) -> Result<()> { + let file = destdir.join(path); + fs::create_dir_all( + file.parent() + .context("Staged file path must have a parent")?, + )?; + fs::write(file, contents)?; + Ok(()) +} + +#[test] +fn transaction_orders_relinquishing_update_before_new_file_owner() -> Result<()> { + let rootfs = tempfile::tempdir().context("Failed to create temp rootfs")?; + let payloads = tempfile::tempdir().context("Failed to create payload dir")?; + let mut cfg = config::Config::for_rootfs(rootfs.path()); + cfg.build_dir = rootfs.path().join("var/cache/depot/build"); + cfg.db_dir = rootfs.path().join("var/lib/depot"); + let db_path = cfg.installed_db_path(rootfs.path()); + + let old_alpha = file_ownership_test_spec("alpha", "1.0"); + let old_alpha_dest = payloads.path().join("old-alpha"); + stage_file(&old_alpha_dest, "usr/bin/shared", "alpha-old")?; + stage_file(rootfs.path(), "usr/bin/shared", "alpha-old")?; + db::register_package(&db_path, &old_alpha, &old_alpha_dest)?; + + let new_alpha = file_ownership_test_spec("alpha", "2.0"); + let new_alpha_dest = payloads.path().join("new-alpha"); + stage_file(&new_alpha_dest, "usr/bin/alpha", "alpha-new")?; + let beta = file_ownership_test_spec("beta", "1.0"); + let beta_dest = payloads.path().join("beta"); + stage_file(&beta_dest, "usr/bin/shared", "beta")?; + + let mut plans = plan_package_outputs_for_install(&beta, &beta_dest, rootfs.path(), &cfg)?; + plans.extend(plan_package_outputs_for_install( + &new_alpha, + &new_alpha_dest, + rootfs.path(), + &cfg, + )?); + + let ordered = preflight_file_ownership_and_order(&plans, &HashSet::new(), rootfs.path(), &cfg)?; + assert_eq!(ordered[0].spec.package.name, "alpha"); + assert_eq!(ordered[1].spec.package.name, "beta"); + + install_direct_transaction(&plans, rootfs.path(), &cfg)?; + + assert_eq!( + fs::read_to_string(rootfs.path().join("usr/bin/shared"))?, + "beta" + ); + assert_eq!( + db::owns_path(&db_path, Path::new("usr/bin/shared"))?, + Some("beta".into()) + ); + assert_eq!( + db::get_package_version(&db_path, "alpha")?, + Some("2.0".into()) + ); + Ok(()) +} + +#[test] +fn transaction_rejects_file_still_owned_after_planned_update_before_mutation() -> Result<()> { + let rootfs = tempfile::tempdir().context("Failed to create temp rootfs")?; + let payloads = tempfile::tempdir().context("Failed to create payload dir")?; + let mut cfg = config::Config::for_rootfs(rootfs.path()); + cfg.build_dir = rootfs.path().join("var/cache/depot/build"); + cfg.db_dir = rootfs.path().join("var/lib/depot"); + let db_path = cfg.installed_db_path(rootfs.path()); + + let old_alpha = file_ownership_test_spec("alpha", "1.0"); + let old_alpha_dest = payloads.path().join("old-alpha"); + stage_file(&old_alpha_dest, "usr/bin/shared", "alpha-old")?; + stage_file(rootfs.path(), "usr/bin/shared", "alpha-old")?; + db::register_package(&db_path, &old_alpha, &old_alpha_dest)?; + + let new_alpha = file_ownership_test_spec("alpha", "2.0"); + let new_alpha_dest = payloads.path().join("new-alpha"); + stage_file(&new_alpha_dest, "usr/bin/shared", "alpha-new")?; + let beta = file_ownership_test_spec("beta", "1.0"); + let beta_dest = payloads.path().join("beta"); + stage_file(&beta_dest, "usr/bin/shared", "beta")?; + + let mut plans = plan_package_outputs_for_install(&beta, &beta_dest, rootfs.path(), &cfg)?; + plans.extend(plan_package_outputs_for_install( + &new_alpha, + &new_alpha_dest, + rootfs.path(), + &cfg, + )?); + + let err = install_direct_transaction(&plans, rootfs.path(), &cfg) + .expect_err("retained ownership conflict should fail preflight"); + assert!( + err.to_string() + .contains("still provided by its transaction update") + ); + assert_eq!( + fs::read_to_string(rootfs.path().join("usr/bin/shared"))?, + "alpha-old" + ); + assert_eq!( + db::get_package_version(&db_path, "alpha")?, + Some("1.0".into()) + ); + assert_eq!(db::get_package_version(&db_path, "beta")?, None); + Ok(()) +} + #[test] fn renamed_abi_updates_keep_versioned_shared_libraries() -> Result<()> { let rootfs = tempfile::tempdir().context("Failed to create temp rootfs")?; diff --git a/src/db/mod.rs b/src/db/mod.rs index cc6d8e6..4e1799a 100755 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -6,6 +6,7 @@ use crate::package::PackageSpec; use crate::staging; use anyhow::{Context, Result}; use rusqlite::{Connection, OptionalExtension, params}; +use std::collections::BTreeMap; use std::fs; use std::path::Path; use walkdir::WalkDir; @@ -24,10 +25,43 @@ fn should_ignore_sbase_conflicts() -> bool { std::env::var_os(DEPOT_BOOTSTRAP_IGNORE_SBASE_CONFLICTS).is_some() } -fn should_auto_clear_conflict(owner: &str, path: &str) -> bool { +pub(crate) fn should_auto_clear_conflict(owner: &str, path: &str) -> bool { (owner == "sbase" && should_ignore_sbase_conflicts()) || is_auto_removable_path(path) } +/// Return every installed file path and its owning package. +pub(crate) fn get_file_ownership(db_path: &Path) -> Result> { + if !db_path.exists() { + return Ok(BTreeMap::new()); + } + + let conn = Connection::open(db_path) + .with_context(|| format!("Failed to open package database at {}", db_path.display()))?; + init_db(&conn).with_context(|| { + format!( + "Failed to initialize package database at {}", + db_path.display() + ) + })?; + let mut stmt = conn + .prepare( + "SELECT f.path, p.name + FROM files f + JOIN packages p ON p.id = f.package_id + ORDER BY f.path, p.name", + ) + .context("Failed to prepare installed file ownership query")?; + let rows = stmt + .query_map([], |row| Ok((row.get(0)?, row.get(1)?))) + .context("Failed to query installed file ownership")?; + let mut ownership = BTreeMap::new(); + for row in rows { + let (path, owner) = row.context("Failed to read installed file ownership row")?; + ownership.insert(path, owner); + } + Ok(ownership) +} + /// Installed package row from the local package database. #[derive(Debug, Clone, PartialEq, Eq)] pub struct InstalledPackageRecord {