diff --git a/src/agent/Cargo.lock b/src/agent/Cargo.lock index a705ba3b29..9032aff50e 100644 --- a/src/agent/Cargo.lock +++ b/src/agent/Cargo.lock @@ -2123,6 +2123,7 @@ dependencies = [ "notify", "onefuzz-telemetry", "pete", + "pretty_assertions", "proc-maps", "process_control", "rand 0.8.4", diff --git a/src/agent/onefuzz-task/src/tasks/coverage/dotnet.rs b/src/agent/onefuzz-task/src/tasks/coverage/dotnet.rs index 548cecdddc..b2c1dfbf94 100644 --- a/src/agent/onefuzz-task/src/tasks/coverage/dotnet.rs +++ b/src/agent/onefuzz-task/src/tasks/coverage/dotnet.rs @@ -341,12 +341,12 @@ impl<'a> TaskContext<'a> { let input = PlaceHolder::Input.get_string(); for entry in &self.config.target_options { - if entry.contains(&input) { + if entry.contains(input) { return true; } } for (k, v) in &self.config.target_env { - if k == &input || v.contains(&input) { + if k == input || v.contains(input) { return true; } } diff --git a/src/agent/onefuzz-task/src/tasks/coverage/generic.rs b/src/agent/onefuzz-task/src/tasks/coverage/generic.rs index 79ae89f2a3..554623e012 100644 --- a/src/agent/onefuzz-task/src/tasks/coverage/generic.rs +++ b/src/agent/onefuzz-task/src/tasks/coverage/generic.rs @@ -270,12 +270,12 @@ impl<'a> TaskContext<'a> { let input = PlaceHolder::Input.get_string(); for entry in &self.config.target_options { - if entry.contains(&input) { + if entry.contains(input) { return true; } } for (k, v) in &self.config.target_env { - if k == &input || v.contains(&input) { + if k == input || v.contains(input) { return true; } } diff --git a/src/agent/onefuzz/Cargo.toml b/src/agent/onefuzz/Cargo.toml index 72c23246a9..286d8fd7c4 100644 --- a/src/agent/onefuzz/Cargo.toml +++ b/src/agent/onefuzz/Cargo.toml @@ -20,7 +20,11 @@ lazy_static = "1.4" log = "0.4" notify = "5.0.0-pre.14" regex = "1.6.0" -reqwest = { version = "0.11", features = ["json", "stream", "native-tls-vendored"], default-features=false } +reqwest = { version = "0.11", features = [ + "json", + "stream", + "native-tls-vendored", +], default-features = false } sha2 = "0.10.2" url = { version = "2.3", features = ["serde"] } serde = "1.0" @@ -38,8 +42,8 @@ strum = "0.24" strum_macros = "0.24" tempfile = "3.2" process_control = "4.0" -reqwest-retry = { path = "../reqwest-retry"} -onefuzz-telemetry = { path = "../onefuzz-telemetry"} +reqwest-retry = { path = "../reqwest-retry" } +onefuzz-telemetry = { path = "../onefuzz-telemetry" } stacktrace-parser = { path = "../stacktrace-parser" } backoff = { version = "0.4", features = ["tokio"] } @@ -60,3 +64,4 @@ proc-maps = "0.2" [dev-dependencies] structopt = "0.3" +pretty_assertions = "1.3.0" diff --git a/src/agent/onefuzz/src/expand.rs b/src/agent/onefuzz/src/expand.rs index 346822d0d4..a4688281e2 100644 --- a/src/agent/onefuzz/src/expand.rs +++ b/src/agent/onefuzz/src/expand.rs @@ -4,6 +4,7 @@ use crate::{machine_id::MachineIdentity, sha256::digest_file_blocking}; use anyhow::{format_err, Context, Result}; use onefuzz_telemetry::{InstanceTelemetryKey, MicrosoftTelemetryKey}; +use regex::Regex; use std::path::{Path, PathBuf}; use std::{collections::HashMap, hash::Hash}; use strum::IntoEnumIterator; @@ -17,7 +18,7 @@ pub enum ExpandedValue<'a> { Mapping(MappingFn<'a>), } -type MappingFn<'a> = Box, &str) -> Result>> + Send>; +type MappingFn<'a> = Box) -> Result> + Send>; #[derive(PartialEq, Eq, Hash, EnumIter)] pub enum PlaceHolder { @@ -52,7 +53,7 @@ pub enum PlaceHolder { } impl PlaceHolder { - pub fn get_string(&self) -> String { + pub fn get_string(&self) -> &'static str { match self { Self::Input => "{input}", Self::Crashes => "{crashes}", @@ -83,12 +84,11 @@ impl PlaceHolder { Self::InstanceTelemetryKey => "{instance_telemetry_key}", Self::InputFileSha256 => "{input_file_sha256}", } - .to_string() } } pub struct Expand<'a> { - values: HashMap>, + values: HashMap<&'static str, ExpandedValue<'a>>, machine_identity: &'a MachineIdentity, } @@ -121,49 +121,73 @@ impl<'a> Expand<'a> { Ok(self.set_value(PlaceHolder::MachineId, ExpandedValue::Scalar(value))) } - fn input_file_sha256(&self, _format_str: &str) -> Result>> { - let val = match self.values.get(&PlaceHolder::Input.get_string()) { - Some(ExpandedValue::Path(fp)) => { - let file = PathBuf::from(fp); - let hash = digest_file_blocking(file)?; - Some(ExpandedValue::Scalar(hash)) - } - _ => None, + fn input_file_sha256(&self) -> Result> { + let Some(val) = self.values.get(PlaceHolder::Input.get_string()) else { + bail!("no value found for {}, unable to evaluate {}", + PlaceHolder::Input.get_string(), + PlaceHolder::InputFileSha256.get_string(), + ) + }; + + let ExpandedValue::Path(fp) = val else { + bail!( + "{} must be used with a path value for {}", + PlaceHolder::InputFileSha256.get_string(), + PlaceHolder::Input.get_string() + ) }; - Ok(val) + let file = PathBuf::from(fp); + let hash = digest_file_blocking(file)?; + Ok(ExpandedValue::Scalar(hash)) } - fn extract_file_name_no_ext(&self, _format_str: &str) -> Result>> { - let val = match self.values.get(&PlaceHolder::Input.get_string()) { - Some(ExpandedValue::Path(fp)) => { - let file = PathBuf::from(fp); - let stem = file - .file_stem() - .ok_or_else(|| format_err!("missing file stem: {}", file.display()))?; - let name_as_str = stem.to_string_lossy().to_string(); - Some(ExpandedValue::Scalar(name_as_str)) - } - _ => None, + fn extract_file_name_no_ext(&self) -> Result> { + let Some(val) = self.values.get(PlaceHolder::Input.get_string()) else { + bail!("no value found for {}, unable to evaluate {}", + PlaceHolder::Input.get_string(), + PlaceHolder::InputFileNameNoExt.get_string(), + ) + }; + + let ExpandedValue::Path(fp) = val else { + bail!( + "{} must be used with a path value for {}", + PlaceHolder::InputFileNameNoExt.get_string(), + PlaceHolder::Input.get_string() + ) }; - Ok(val) + let file = PathBuf::from(fp); + let stem = file + .file_stem() + .ok_or_else(|| format_err!("missing file stem: {}", file.display()))?; + let name_as_str = stem.to_string_lossy().to_string(); + Ok(ExpandedValue::Scalar(name_as_str)) } - fn extract_file_name(&self, _format_str: &str) -> Result>> { - let val = match self.values.get(&PlaceHolder::Input.get_string()) { - Some(ExpandedValue::Path(fp)) => { - let file = PathBuf::from(fp); - let name = file - .file_name() - .ok_or_else(|| format_err!("missing file name: {}", file.display()))?; - let name_as_str = name.to_string_lossy().to_string(); - Some(ExpandedValue::Scalar(name_as_str)) - } - _ => None, + fn extract_file_name(&self) -> Result> { + let Some(val) = self.values.get(PlaceHolder::Input.get_string()) else { + bail!("no value found for {}, unable to evaluate {}", + PlaceHolder::Input.get_string(), + PlaceHolder::InputFileName.get_string(), + ) + }; + + let ExpandedValue::Path(fp) = val else { + bail!( + "{} must be used with a path value for {}", + PlaceHolder::InputFileName.get_string(), + PlaceHolder::Input.get_string() + ) }; - Ok(val) + let file = PathBuf::from(fp); + let name = file + .file_name() + .ok_or_else(|| format_err!("missing file name: {}", file.display()))?; + let name_as_str = name.to_string_lossy().to_string(); + Ok(ExpandedValue::Scalar(name_as_str)) } pub fn set_value(self, name: PlaceHolder, value: ExpandedValue<'a>) -> Self { @@ -339,76 +363,113 @@ impl<'a> Expand<'a> { ) } - fn replace_value( + fn get_value( &self, - fmtstr: &str, - mut arg: String, ev: &ExpandedValue<'a>, + eval_stack: &mut Vec<&'static str>, ) -> Result { match ev { ExpandedValue::Path(v) => { - let path = String::from( - dunce::canonicalize(v) - .with_context(|| { - format!("unable to canonicalize path during extension: {v}") - })? - .to_string_lossy(), - ); - arg = arg.replace(fmtstr, &path); - Ok(arg) - } - ExpandedValue::Scalar(v) => { - arg = arg.replace(fmtstr, v); - Ok(arg) + // evaluate the inner replacement first, + // since it may in turn have replacements to be performed + let evaluated = self.evaluate_value_checked(v, eval_stack)?; + let path = dunce::canonicalize(evaluated) + .with_context(|| format!("unable to canonicalize path during extension: {v}"))? + .to_string_lossy() + .to_string(); + Ok(path) } + ExpandedValue::Scalar(v) => Ok(v.clone()), ExpandedValue::List(value) => { - let replaced = self.evaluate(value)?; + let replaced = self.evaluate_checked(value, eval_stack)?; let replaced = replaced.join(" "); - arg = arg.replace(fmtstr, &replaced); - Ok(arg) - } - ExpandedValue::Mapping(func) => { - if let Some(value) = func(self, fmtstr)? { - let arg = self.replace_value(fmtstr, arg, &value)?; - Ok(arg) - } else { - Ok(arg) - } + Ok(replaced) } + ExpandedValue::Mapping(func) => self.get_value(&func(self)?, eval_stack), } } - pub fn evaluate_value>(&self, arg: T) -> Result { - let mut arg = arg.as_ref().to_owned(); + fn evaluate_value_checked( + &self, + arg: impl AsRef, + eval_stack: &mut Vec<&'static str>, + ) -> Result { + lazy_static::lazy_static! { + static ref VAR_RE: Regex = Regex::new(r"\{[^}]+?\}").unwrap(); + } - for placeholder in PlaceHolder::iter() { - let fmtstr = &placeholder.get_string(); - match ( - arg.contains(fmtstr), - self.values.get(&placeholder.get_string()), - ) { - (true, Some(ev)) => { - arg = self - .replace_value(fmtstr, arg.clone(), ev) - .with_context(|| format!("replace_value failed: {fmtstr} {arg}"))? + let arg = arg.as_ref().to_owned(); + let mut errors = Vec::new(); + + let result = VAR_RE.replace_all(&arg, |captures: ®ex::Captures| -> String { + let matched = captures.get(0).unwrap().as_str(); // capture 0 must always be present here + match self.values.get_key_value(matched) { + Some((placeholder, ev)) => { + if eval_stack.contains(placeholder) { + eval_stack.push(placeholder); + let path = eval_stack.join("->"); + errors.push(format!( + "attempting to replace {placeholder} with a value that contains itself (replacements {path})" + )); + eval_stack.pop(); + String::new() + } else { + eval_stack.push(placeholder); + let result = self.get_value(ev, eval_stack) + .with_context(|| format!("unable to get value of {placeholder}")); + eval_stack.pop(); + + match result { + Ok(v) => v, + Err(e) => { + errors.push(format!("{e:#}")); + String::new() + } + } + } + } + None => { + if PlaceHolder::iter().any(|v| v.get_string() == matched) { + // this is a known replacement but no value is defined: + errors.push(format!("replacement {matched} is not available")) + } else { + // probably a typo, we don't know this placeholder: + errors.push(format!("unknown variable replacement {matched}")) + } + String::new() } - (true, None) => bail!("missing argument {}", fmtstr), - (false, _) => (), } + }); + + if errors.is_empty() { + Ok(result.into_owned()) + } else { + bail!(errors.join("; ")); } - Ok(arg) } - pub fn evaluate>(&self, args: &[T]) -> Result> { + pub fn evaluate_value(&self, arg: impl AsRef) -> Result { + self.evaluate_value_checked(arg, &mut Vec::new()) + } + + fn evaluate_checked( + &self, + args: &[impl AsRef], + eval_stack: &mut Vec<&'static str>, + ) -> Result> { let mut result = Vec::new(); for arg in args { let arg = self - .evaluate_value(arg) + .evaluate_value_checked(arg, eval_stack) .with_context(|| format!("evaluating argument failed: {}", arg.as_ref()))?; result.push(arg); } Ok(result) } + + pub fn evaluate(&self, args: &[impl AsRef]) -> Result> { + self.evaluate_checked(args, &mut Vec::new()) + } } #[cfg(test)] @@ -417,6 +478,7 @@ mod tests { use super::Expand; use anyhow::{Context, Result}; + use pretty_assertions::assert_eq; use std::path::Path; use uuid::Uuid; @@ -428,21 +490,96 @@ mod tests { } } + #[test] + fn test_setup_dir_and_target_exe() -> Result<()> { + // use current exe name here, since path must exist: + let current_exe = std::env::current_exe()?; + let dir_part = current_exe.parent().unwrap(); + let name_part = current_exe.file_name().unwrap().to_string_lossy(); + + let target_exe = Expand::new(&test_machine_identity()) + .setup_dir(dir_part) + .target_exe(format!("{{setup_dir}}/{name_part}")) + .evaluate_value("{target_exe}")?; + + assert_eq!(target_exe, current_exe.to_string_lossy()); + + Ok(()) + } + #[test] fn test_expand_nested() -> Result<()> { - let supervisor_options = vec!["{target_options}".to_string()]; - let target_options: Vec<_> = vec!["a", "b", "c"].iter().map(|p| p.to_string()).collect(); let result = Expand::new(&test_machine_identity()) - .target_options(&target_options) - .evaluate(&supervisor_options)?; - let expected = vec!["a b c"]; + .target_options(&["a".to_string(), "b".to_string(), "c".to_string()]) + .supervisor_options(&["{target_options}".to_string()]) + .evaluate(&["{supervisor_options}"])?; + assert_eq!(result, vec!["a b c"]); + Ok(()) + } + + #[test] + fn test_expand_nested_reverse() -> Result<()> { + let result = Expand::new(&test_machine_identity()) + .supervisor_options(&["a".to_string(), "b".to_string(), "c".to_string()]) + .target_options(&["{supervisor_options}".to_string()]) + .evaluate(&["{target_options}"])?; + assert_eq!(result, vec!["a b c"]); + Ok(()) + } + + #[test] + fn test_self_referential_list() -> Result<()> { + let result = Expand::new(&test_machine_identity()) + .supervisor_options(&["{supervisor_options}".to_string()]) + .evaluate(&["{supervisor_options}"]); + + let e = result.err().unwrap(); assert_eq!( - result, expected, - "result: {result:?} expected: {expected:?}" + format!("{e:#}"), + "evaluating argument failed: {supervisor_options}: \ + unable to get value of {supervisor_options}: \ + evaluating argument failed: {supervisor_options}: \ + attempting to replace {supervisor_options} with a value that contains itself \ + (replacements {supervisor_options}->{supervisor_options})" ); Ok(()) } + #[test] + fn test_self_referential_path() { + let result = Expand::new(&test_machine_identity()) + .target_exe("{target_exe}") + .evaluate(&["{target_exe}"]); + + let e = result.err().unwrap(); + assert_eq!( + format!("{e:#}"), + "evaluating argument failed: {target_exe}: \ + unable to get value of {target_exe}: \ + attempting to replace {target_exe} with a value that contains itself \ + (replacements {target_exe}->{target_exe})" + ); + } + + #[test] + fn test_mutually_recursive() { + let result = Expand::new(&test_machine_identity()) + .supervisor_options(&["{target_exe}".to_string()]) + .target_exe("{supervisor_options}") + .evaluate(&["{target_exe}"]); + + let e = result.err().unwrap(); + assert_eq!( + format!("{e:#}"), + "evaluating argument failed: {target_exe}: \ + unable to get value of {target_exe}: \ + unable to get value of {supervisor_options}: \ + evaluating argument failed: {target_exe}: \ + attempting to replace {target_exe} with a value that contains itself \ + (replacements {target_exe}->{supervisor_options}->{target_exe})" + ); + } + #[test] fn test_expand() -> Result<()> { let my_options: Vec<_> = vec![ @@ -523,6 +660,76 @@ mod tests { Ok(()) } + #[test] + fn missing_replacement() { + let result = Expand::new(&test_machine_identity()).evaluate_value("a {input} b"); + assert_eq!( + format!("{:#}", result.err().unwrap()), + "replacement {input} is not available" + ); + } + + #[test] + fn typoed_variable() { + let result = Expand::new(&test_machine_identity()) + .input_path("src/lib.rs") + .evaluate_value("a {input_paht} b"); + assert_eq!( + format!("{:#}", result.err().unwrap()), + "unknown variable replacement {input_paht}" + ); + } + + #[test] + fn multiple_errors() { + let result = + Expand::new(&test_machine_identity()).evaluate_value("a {input_paht} {input} b"); + assert_eq!( + format!("{:#}", result.err().unwrap()), + "unknown variable replacement {input_paht}; replacement {input} is not available" + ); + } + + #[test] + fn missing_input() { + for mapping_fn in [ + "{input_file_sha256}", + "{input_file_name}", + "{input_file_name_no_ext}", + ] { + let result = Expand::new(&test_machine_identity()).evaluate_value(mapping_fn); + + assert_eq!( + format!("{:#}", result.err().unwrap()), + format!( + "unable to get value of {mapping_fn}: \ + no value found for {{input}}, unable to evaluate {mapping_fn}" + ) + ); + } + } + + #[test] + fn wrong_input_type() { + for mapping_fn in [ + "{input_file_sha256}", + "{input_file_name}", + "{input_file_name_no_ext}", + ] { + let result = Expand::new(&test_machine_identity()) + .input_marker("not a path") // this inserts {input} with a Scalar type + .evaluate_value(mapping_fn); + + assert_eq!( + format!("{:#}", result.err().unwrap()), + format!( + "unable to get value of {mapping_fn}: \ + {mapping_fn} must be used with a path value for {{input}}" + ) + ); + } + } + #[tokio::test] async fn test_expand_machine_id() -> Result<()> { let machine_identity = &test_machine_identity();