diff --git a/Cargo.lock b/Cargo.lock index 71bdd09..a6c9d40 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -32,6 +32,7 @@ dependencies = [ "serde_default", "serde_json", "serde_regex", + "subprocess", "toml 1.1.2+spec-1.1.0", "uzers", ] @@ -853,6 +854,16 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" +[[package]] +name = "subprocess" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3713691ac66e56ba6bafb8a62158859653b3127ae5cacae98d437bbb0c9ccb06" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "syn" version = "2.0.118" @@ -1016,6 +1027,22 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + [[package]] name = "winapi-util" version = "0.1.11" @@ -1025,6 +1052,12 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + [[package]] name = "windows-link" version = "0.2.1" diff --git a/angrr/Cargo.toml b/angrr/Cargo.toml index c58b848..b3885ca 100644 --- a/angrr/Cargo.toml +++ b/angrr/Cargo.toml @@ -30,3 +30,4 @@ serde_default = "*" serde_json = "*" serde_regex = "*" ignore = "*" +subprocess = "*" diff --git a/angrr/src/command.rs b/angrr/src/command.rs index 4a8754f..0079ef6 100644 --- a/angrr/src/command.rs +++ b/angrr/src/command.rs @@ -1,4 +1,5 @@ use clap::{Parser, Subcommand, ValueEnum}; +use humantime::Duration; use core::fmt; use std::{ffi::OsString, path::PathBuf}; @@ -105,6 +106,10 @@ pub struct RunOptions { #[arg(long)] pub null_output_delimiter: bool, + /// Timeout for external filters + #[arg(long, default_value = "1000ms")] + pub filter_timeout: Duration, + /// Do not remove file. #[arg(long)] pub dry_run: bool, diff --git a/angrr/src/config.rs b/angrr/src/config.rs index 2a131fc..e728148 100644 --- a/angrr/src/config.rs +++ b/angrr/src/config.rs @@ -232,7 +232,7 @@ pub struct ProfileConfig { /// Each rule retains `n` generations every `bucket-window` duration for `bucket-amount` buckets. /// `n` defaults to 1. #[serde(default)] - pub keep_n_per_bucket: Vec, + pub keep_n_per_bucket: Vec, } const fn default_periodic_rule_n() -> usize { @@ -242,15 +242,11 @@ const fn default_periodic_rule_n() -> usize { #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] #[serde(deny_unknown_fields)] -pub struct KeepNPerBucket { +pub struct KeepNPerBucketConfig { #[serde(default = "default_periodic_rule_n")] pub n: usize, - #[serde(with = "humantime_serde")] - #[serde(default)] pub bucket_window: Duration, - - #[serde(default)] pub bucket_amount: u32, } @@ -266,12 +262,17 @@ impl ProfileConfig { ); } for path in &self.profile_paths { - if !(path.starts_with("~") || path.is_absolute()) { + if !(path.starts_with("~/") || path.is_absolute()) { anyhow::bail!( - "invalid profile policy {name}: profile path \"{path:?}\" must be absolute or start with `~`", + "invalid profile policy {name}: profile path \"{path:?}\" must be absolute or start with `~/`", ); } } + for cfg in &self.keep_n_per_bucket { + if cfg.bucket_window.is_zero() { + anyhow::bail!("invalid keep-n-per-bucket: bucket window is zero"); + } + } Ok(()) } } @@ -434,7 +435,7 @@ mod tests { foo = true ", ); - shouldnt_parse::("foo = 1"); + shouldnt_parse::("foo = 1"); shouldnt_parse::("foo = true"); shouldnt_parse::("foo = true"); } diff --git a/angrr/src/filter.rs b/angrr/src/filter.rs index f6047a3..bc8a98b 100644 --- a/angrr/src/filter.rs +++ b/angrr/src/filter.rs @@ -1,11 +1,11 @@ -use std::{ - io::Write, - path::PathBuf, - process::{Command, Stdio}, -}; +use std::path::PathBuf; use anyhow::Context; +use log::Level; use serde::{Deserialize, Serialize}; +use subprocess::Exec; + +use crate::command::RunOptions; #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] @@ -22,7 +22,7 @@ pub struct Input { } impl Filter { - pub fn run(&self, input: &Input) -> anyhow::Result { + pub fn run(&self, options: &RunOptions, input: &Input) -> anyhow::Result { let json = serde_json::to_string(input) .with_context(|| format!("failed to serialize input for filter: {input:?}"))?; log::trace!( @@ -30,44 +30,32 @@ impl Filter { self.program, self.arguments ); - let mut child = Command::new(&self.program) + let job = Exec::cmd(&self.program) .args(&self.arguments) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() + .stdin(json.into_bytes()) + .start() .with_context(|| { format!( "failed to invoke external filter {:?} with arguments: {:?}", self.program, self.arguments ) })?; - { - let mut stdin = child - .stdin - .take() - .with_context(|| "failed to take stdin of external filter")?; - stdin - .write_all(json.as_bytes()) - .context("can not write to stdin of external filter")?; - // stdin drop and closed here - } - let output = child - .wait_with_output() - .context("failed to wait on external filter")?; - // log stdout and stderr for debugging - if !output.stdout.is_empty() { - log::debug!( - "external filter stdout: {}", - String::from_utf8_lossy(&output.stdout) - ); + let capture = job.capture_timeout(options.filter_timeout.into())?; + if !capture.stdout.is_empty() { + log::debug!("external filter stdout: {}", capture.stdout_str()); } - if !output.stderr.is_empty() { - log::warn!( - "external filter stderr: {}", - String::from_utf8_lossy(&output.stderr) - ); + if !capture.stderr.is_empty() { + log::warn!("external filter stderr: {}", capture.stderr_str()); } - Ok(output.status.success()) + log::log!( + if capture.exit_status.success() { + Level::Trace + } else { + Level::Debug + }, + "external filter exit with status: {}", + capture.exit_status + ); + Ok(capture.exit_status.success()) } } diff --git a/angrr/src/policy/profile.rs b/angrr/src/policy/profile.rs index 19c48e0..318aba8 100644 --- a/angrr/src/policy/profile.rs +++ b/angrr/src/policy/profile.rs @@ -2,7 +2,7 @@ use std::collections::BTreeSet; use std::{fs, path::PathBuf}; use crate::{ - config::KeepNPerBucket, config::ProfileConfig, profile::Generation, profile::Profile, + config::KeepNPerBucketConfig, config::ProfileConfig, profile::Generation, profile::Profile, utils::format_duration_short, }; use anyhow::Context; @@ -104,7 +104,7 @@ impl ProfilePolicy { }; // Keep track of what was processed and skip them. let mut processed: BTreeSet = BTreeSet::new(); - for &KeepNPerBucket { + for &KeepNPerBucketConfig { n, bucket_window, bucket_amount, @@ -112,16 +112,15 @@ impl ProfilePolicy { { for i in 0..bucket_amount { let mut processed_curr: BTreeSet = BTreeSet::new(); - sorted_generations.iter().filter(|(_, generation)| { + sorted_generations.iter().filter(|(gen_index, generation)| { let within_window = bucket_window * i <= generation.root.age && generation.root.age < bucket_window * (i + 1); - let not_processed = !processed.contains(&generation.number); + let not_processed = !processed.contains(gen_index); within_window && not_processed }) .take(n) .for_each(|(gen_index, generation)| { - processed_curr.insert(generation.number); - keep_generation[*gen_index] = true; + processed_curr.insert(*gen_index); log::debug!( "[{}] keep generation {} by keep_n_per_bucket, namely {} generation each bucket spanning {} for {} buckets", self.name, @@ -130,6 +129,7 @@ impl ProfilePolicy { format_duration_short(bucket_window), bucket_amount, ); + keep_generation[*gen_index] = true; }); processed.extend(processed_curr); diff --git a/angrr/src/policy/temporary.rs b/angrr/src/policy/temporary.rs index c544ca1..26f38dc 100644 --- a/angrr/src/policy/temporary.rs +++ b/angrr/src/policy/temporary.rs @@ -3,6 +3,7 @@ use std::os::unix::ffi::OsStrExt; use std::{fs, os::unix::fs::MetadataExt, path::Path}; use uzers::{get_user_by_uid, os::unix::UserExt}; +use crate::command::RunOptions; use crate::gc_root::GcRoot; use crate::{config::TemporaryRootConfig, filter}; @@ -19,7 +20,7 @@ impl TemporaryRootPolicy { } impl TemporaryRootPolicy { - pub fn monitored(&self, root: &GcRoot) -> anyhow::Result { + pub fn monitored(&self, options: &RunOptions, root: &GcRoot) -> anyhow::Result { if self.ignored_by_prefix(&root.path) || self.ignored_by_prefix_in_home(&root.path, &root.path_metadata) { @@ -50,14 +51,18 @@ impl TemporaryRootPolicy { path: root.path.clone(), gc_root: root.link_path.clone(), }; - let not_ignored = filter - .run(&input) - .with_context(|| format!("failed to run filter on input: {input:?}"))?; + let not_ignored = filter.run(options, &input).with_context(|| { + format!( + "failed to run filter {:?} on input: {input:?}", + filter.program + ) + })?; if !not_ignored { log::debug!( - "[{}] ignore {:?}, filtered out by external filter", + "[{}] ignore {:?}, filtered out by external filter {:?}", self.name, root.path, + filter.program ); return Ok(false); } diff --git a/angrr/src/run.rs b/angrr/src/run.rs index ebeede9..26dcd58 100644 --- a/angrr/src/run.rs +++ b/angrr/src/run.rs @@ -145,7 +145,7 @@ impl RunContext { for gc_root in gc_roots { let mut matched = None; for (name, policy) in policies { - if policy.monitored(gc_root)? { + if policy.monitored(&self.options, gc_root)? { matched = Some((name, policy)); break; } @@ -379,7 +379,7 @@ impl RunContext { P: AsRef, { let path = path.as_ref(); - if path.starts_with("~") { + if path.starts_with("~/") { if self.owned_only { let home = utils::current_user_home()?; Ok(vec![home.join(path.strip_prefix("~").unwrap())]) diff --git a/angrr/src/statistics.rs b/angrr/src/statistics.rs index e3d3935..e917cf9 100644 --- a/angrr/src/statistics.rs +++ b/angrr/src/statistics.rs @@ -9,7 +9,6 @@ pub struct Statistics { pub traversed: Counter, pub monitored: Counter, pub expired: Counter, - pub invalid: Counter, pub removed: Counter, } @@ -19,7 +18,6 @@ impl Statistics { let monitored = self.monitored.done(); let expired = self.expired.done(); let removed = self.removed.done(); - let invalid = self.invalid.done(); let kept = traversed - removed; let num_style = |n| term.style().bold().apply_to(n); [ @@ -31,7 +29,6 @@ impl Statistics { num_style(removed), dry_run_indicator(term, removed != 0 && dry_run) ), - format!("invalid: {}", num_style(invalid)), format!("kept: {}", num_style(kept)), ] .join("\n") diff --git a/angrr/src/utils.rs b/angrr/src/utils.rs index df43152..db1a0b2 100644 --- a/angrr/src/utils.rs +++ b/angrr/src/utils.rs @@ -15,16 +15,9 @@ pub fn validate_store_path, P2: AsRef>( store: P1, target: P2, ) -> Option { - let store = store.as_ref(); let target = target.as_ref(); - match fs::canonicalize(target) { - Ok(path) => { - if path.starts_with(store) { - Some(path) - } else { - None - } - } + match canonicalize_to_store(store, target) { + Ok(path) => Some(path), Err(e) => { log::warn!("failed to canonicalize {target:?} for validation: {e}"); None @@ -32,6 +25,25 @@ pub fn validate_store_path, P2: AsRef>( } } +pub fn canonicalize_to_store, P2: AsRef>( + store: P1, + path: P2, +) -> anyhow::Result { + let store = store.as_ref(); + let path = path.as_ref(); + if path.starts_with(store) { + Ok(PathBuf::from(path)) + } else { + let metadata = fs::symlink_metadata(path)?; + if metadata.is_symlink() { + let next = fs::read_link(path)?; + canonicalize_to_store(store, next) + } else { + anyhow::bail!("canonicalized to a non-store path") + } + } +} + pub fn discover_users(roots: &[Arc]) -> anyhow::Result> { let uids: BTreeSet<_> = roots.iter().map(|root| root.path_metadata.uid()).collect(); let mut users = Vec::new(); diff --git a/nixos/tests/filter.nix b/nixos/tests/filter.nix index fcbbe0c..be6c8f1 100644 --- a/nixos/tests/filter.nix +++ b/nixos/tests/filter.nix @@ -98,15 +98,32 @@ su user1 --command "${lib.getExe (mkGcRoot user1GcRoots)}" su user2 --command "${lib.getExe (mkGcRoot user2GcRoots)}" "${lib.getExe (mkGcRoot rootGcRoots)}" - su user1 --command "${lib.getExe angrrCommand}" + su user1 --command "${lib.getExe angrrCommand} --dry-run" echo "comparing removed paths..." diff --unified <(sort "${expectedRemovedPathsFile}") <(sort /tmp/removed) echo "done" ''; }; + toml = pkgs.formats.toml { }; + slowJq = pkgs.writeShellApplication { + name = "slow-jq"; + runtimeInputs = [ pkgs.jq ]; + text = '' + sleep 0.5 + exec jq "$@" + ''; + }; + slowConfig = toml.generate "filter-timeout-slow.toml" { + temporary-root-policies.result.filter.program = "${lib.getExe slowJq}"; # never exits + }; in '' start_all() - machine.succeed("${lib.getExe testScript}") + with subtest("Filter with jq"): + machine.succeed("${lib.getExe testScript}") + with subtest("Filter timeout"): + machine.succeed("angrr run --interactive=never --dry-run --config ${slowConfig}") + machine.succeed("angrr run --interactive=never --dry-run --filter-timeout=10s --config ${slowConfig}") + machine.fail("angrr run --interactive=never --dry-run --filter-timeout=100ms --config ${slowConfig}") ''; }