diff --git a/Cargo.lock b/Cargo.lock index b44fc613..e7a0df39 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -349,6 +349,7 @@ dependencies = [ "serde_yaml", "sha256", "shell-quote", + "shell-words", "shellexpand", "simplelog", "sysinfo", @@ -2272,6 +2273,12 @@ dependencies = [ "bstr", ] +[[package]] +name = "shell-words" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "24188a676b6ae68c3b2cb3a01be17fbf7240ce009799bb56d5b1409051e78fde" + [[package]] name = "shellexpand" version = "3.1.1" diff --git a/Cargo.toml b/Cargo.toml index f992e596..288ea169 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,6 +62,7 @@ addr2line = "0.25" gimli = "0.32" open = "5.3.2" tabled = "0.17" +shell-words = "1.1.0" [target.'cfg(target_os = "linux")'.dependencies] procfs = "0.17.0" diff --git a/src/run/runner/helpers/apt.rs b/src/run/runner/helpers/apt.rs index af12ac81..1739839c 100644 --- a/src/run/runner/helpers/apt.rs +++ b/src/run/runner/helpers/apt.rs @@ -96,10 +96,10 @@ pub fn install(system_info: &SystemInfo, packages: &[&str]) -> Result<()> { info!("Installing packages: {}", packages.join(", ")); - run_with_sudo(&["apt-get", "update"])?; - let mut install_cmd = vec!["apt-get", "install", "-y", "--allow-downgrades"]; - install_cmd.extend_from_slice(packages); - run_with_sudo(&install_cmd)?; + run_with_sudo("apt-get", ["update"])?; + let mut install_argv = vec!["install", "-y", "--allow-downgrades"]; + install_argv.extend_from_slice(packages); + run_with_sudo("apt-get", &install_argv)?; debug!("Packages installed successfully"); Ok(()) @@ -155,9 +155,7 @@ fn restore_from_cache(system_info: &SystemInfo, cache_dir: &Path) -> Result<()> .to_str() .ok_or_else(|| anyhow!("Invalid cache directory path"))?; - let copy_cmd = format!("cp -r {cache_dir_str}/* /"); - - run_with_sudo(&["bash", "-c", ©_cmd])?; + run_with_sudo("cp", ["-r", &format!("{cache_dir_str}/*"), "/"])?; debug!("Cache restored successfully"); Ok(()) diff --git a/src/run/runner/helpers/command.rs b/src/run/runner/helpers/command.rs new file mode 100644 index 00000000..4dbc5364 --- /dev/null +++ b/src/run/runner/helpers/command.rs @@ -0,0 +1,188 @@ +use std::{ + collections::BTreeMap, + ffi::{OsStr, OsString}, + process::Command, +}; + +#[derive(Debug)] +pub struct CommandBuilder { + program: OsString, + argv: Vec, + envs: BTreeMap, + cwd: Option, +} + +impl CommandBuilder { + pub fn new>(program: S) -> Self { + Self { + program: program.as_ref().to_owned(), + argv: Vec::new(), + envs: BTreeMap::new(), + cwd: None, + } + } + + pub fn build(self) -> Command { + let mut command = Command::new(&self.program); + command.args(&self.argv); + command.envs(&self.envs); + if let Some(cwd) = self.cwd { + command.current_dir(cwd); + } + command + } + + pub fn arg>(&mut self, arg: S) -> &mut Self { + self.argv.push(arg.as_ref().to_owned()); + self + } + + pub fn args(&mut self, args: I) -> &mut Self + where + I: IntoIterator, + S: AsRef, + { + for arg in args { + self.arg(arg.as_ref()); + } + self + } + + pub fn current_dir(&mut self, dir: D) + where + D: AsRef, + { + self.cwd = Some(dir.as_ref().to_owned()); + } + + pub fn wrap(&mut self, wrapper: S, wrapper_args: I) -> &mut Self + where + S: AsRef, + I: IntoIterator, + T: AsRef, + { + let mut new_argv = Vec::new(); + + // Add wrapper arguments first + for arg in wrapper_args { + new_argv.push(arg.as_ref().to_owned()); + } + + // Add the current program + new_argv.push(self.program.clone()); + + // Add the current arguments + new_argv.extend(self.argv.iter().cloned()); + + // Update program to wrapper and argv to the new argument list + self.program = wrapper.as_ref().to_owned(); + self.argv = new_argv; + self + } + + pub fn wrap_with(&mut self, wrapper_cmd: CommandBuilder) -> &mut Self { + let mut new_argv = Vec::new(); + + // Add wrapper command arguments first + new_argv.extend(wrapper_cmd.argv.iter().cloned()); + + // Add the current program + new_argv.push(self.program.clone()); + + // Add the current arguments + new_argv.extend(self.argv.iter().cloned()); + + // Update program to wrapper and argv to the new argument list + self.program = wrapper_cmd.program; + self.argv = new_argv; + // Update cwd if wrapper has it set + self.cwd = wrapper_cmd.cwd.or(self.cwd.take()); + // Merge environment variables, with wrapper's envs taking precedence + self.envs.extend(wrapper_cmd.envs); + self + } + + /// Returns the command line as a string for debugging/testing purposes + pub fn as_command_line(&self) -> String { + let mut parts: Vec = vec![self.program.to_string_lossy().into_owned()]; + parts.extend( + self.argv + .iter() + .map(|arg| arg.to_string_lossy().into_owned()), + ); + shell_words::join(parts) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_wrap_with_args() { + let mut builder = CommandBuilder::new("ls"); + builder.arg("-la").wrap("sudo", ["-n"]); + assert_eq!(builder.as_command_line(), "sudo -n ls -la"); + } + + #[test] + fn test_wrap_without_args() { + let mut builder = CommandBuilder::new("ls"); + builder.arg("-la").wrap("time", [] as [&str; 0]); + assert_eq!(builder.as_command_line(), "time ls -la"); + } + + #[test] + fn test_multiple_wraps() { + let mut builder = CommandBuilder::new("valgrind"); + builder + .arg("my-program") + .wrap("setarch", ["x86_64", "-R"]) + .wrap("sudo", ["-n"]); + assert_eq!( + builder.as_command_line(), + "sudo -n setarch x86_64 -R valgrind my-program" + ); + } + + #[test] + fn test_wrap_with_spaces() { + let mut builder = CommandBuilder::new("echo"); + builder.arg("hello world").wrap("bash", ["-c"]); + assert_eq!(builder.as_command_line(), "bash -c echo 'hello world'"); + } + + #[test] + fn test_wrap_and_build() { + let mut builder = CommandBuilder::new("ls"); + builder.arg("-la").wrap("sudo", ["-n"]); + + let cmd = builder.build(); + assert_eq!(cmd.get_program(), "sudo"); + + let args: Vec<_> = cmd.get_args().collect(); + assert_eq!(args, vec!["-n", "ls", "-la"]); + } + + #[test] + fn test_wrap_with_builder() { + let mut wrapper = CommandBuilder::new("sudo"); + wrapper.arg("-n"); + + let mut builder = CommandBuilder::new("ls"); + builder.arg("-la").wrap_with(wrapper); + + assert_eq!(builder.as_command_line(), "sudo -n ls -la"); + } + + #[test] + fn test_wrap_with_preserves_env() { + let mut wrapper = CommandBuilder::new("env"); + wrapper.arg("FOO=bar"); + + let mut builder = CommandBuilder::new("ls"); + builder.arg("-la").wrap_with(wrapper); + + assert_eq!(builder.as_command_line(), "env 'FOO=bar' ls -la"); + } +} diff --git a/src/run/runner/helpers/mod.rs b/src/run/runner/helpers/mod.rs index ed1af9a8..4218e0b0 100644 --- a/src/run/runner/helpers/mod.rs +++ b/src/run/runner/helpers/mod.rs @@ -1,4 +1,5 @@ pub mod apt; +pub mod command; pub mod env; pub mod get_bench_command; pub mod introspected_golang; diff --git a/src/run/runner/helpers/run_with_sudo.rs b/src/run/runner/helpers/run_with_sudo.rs index 3fd27802..88f39101 100644 --- a/src/run/runner/helpers/run_with_sudo.rs +++ b/src/run/runner/helpers/run_with_sudo.rs @@ -1,5 +1,8 @@ -use crate::{local_logger::suspend_progress_bar, prelude::*}; +use crate::{ + local_logger::suspend_progress_bar, prelude::*, run::runner::helpers::command::CommandBuilder, +}; use std::{ + ffi::OsStr, io::IsTerminal, process::{Command, Stdio}, }; @@ -35,9 +38,7 @@ fn validate_sudo_access() -> Result<()> { if needs_password { suspend_progress_bar(|| { - info!( - "Sudo privileges are required to continue. Please enter your password if prompted." - ); + info!("Sudo privileges are required to continue. Please enter your password."); // Validate and cache sudo credentials let auth_status = Command::new("sudo") @@ -57,47 +58,47 @@ fn validate_sudo_access() -> Result<()> { Ok(()) } -/// Creates the base sudo command after validating sudo access -pub fn validated_sudo_command() -> Result { - validate_sudo_access()?; - let mut cmd = Command::new("sudo"); - // Password prompt should not appear here since it has already been validated - cmd.arg("--non-interactive"); - Ok(cmd) -} - -/// Build a command wrapped with sudo if possible -pub fn build_command_with_sudo(command_args: &[&str]) -> Result { - let command_str = command_args.join(" "); +/// Wrap with sudo if not running as root +pub fn wrap_with_sudo(mut cmd_builder: CommandBuilder) -> Result { if is_root_user() { - debug!("Running command without sudo: {command_str}"); - let mut c = Command::new(command_args[0]); - c.args(&command_args[1..]); - Ok(c) + Ok(cmd_builder) } else if is_sudo_available() { - debug!("Sudo is required for command: {command_str}"); - let mut c = validated_sudo_command()?; - c.args(command_args); - Ok(c) + debug!("Wrapping with sudo: {}", cmd_builder.as_command_line()); + validate_sudo_access()?; + cmd_builder.wrap( + "sudo", + // Password prompt should not appear here since it has already been validated + ["--non-interactive"], + ); + Ok(cmd_builder) } else { - bail!("Sudo is not available to run the command: {command_str}"); + bail!( + "Sudo is not available to run the command: {}", + cmd_builder.as_command_line() + ); } } /// Run a command with sudo after validating sudo access -pub fn run_with_sudo(command_args: &[&str]) -> Result<()> { - let command_str = command_args.join(" "); - debug!("Running command with sudo: {command_str}"); - let mut cmd = build_command_with_sudo(command_args)?; +pub fn run_with_sudo(program: S, argv: I) -> Result<()> +where + S: AsRef, + I: IntoIterator, + T: AsRef, +{ + let mut builder = CommandBuilder::new(program); + builder.args(argv); + debug!("Running command with sudo: {}", builder.as_command_line()); + let mut cmd = wrap_with_sudo(builder)?.build(); let output = cmd .stdout(Stdio::piped()) .output() - .map_err(|_| anyhow!("Failed to execute command with sudo: {command_str}"))?; + .map_err(|_| anyhow!("Failed to execute command with sudo: {cmd:?}"))?; if !output.status.success() { info!("stdout: {}", String::from_utf8_lossy(&output.stdout)); error!("stderr: {}", String::from_utf8_lossy(&output.stderr)); - bail!("Failed to execute command with sudo: {command_str}"); + bail!("Failed to execute command with sudo: {cmd:?}"); } Ok(()) diff --git a/src/run/runner/wall_time/executor.rs b/src/run/runner/wall_time/executor.rs index 1f424cbd..ff524759 100644 --- a/src/run/runner/wall_time/executor.rs +++ b/src/run/runner/wall_time/executor.rs @@ -3,12 +3,13 @@ use crate::prelude::*; use crate::run::RunnerMode; use crate::run::instruments::mongo_tracer::MongoTracer; use crate::run::runner::executor::Executor; +use crate::run::runner::helpers::command::CommandBuilder; use crate::run::runner::helpers::env::{get_base_injected_env, is_codspeed_debug_enabled}; use crate::run::runner::helpers::get_bench_command::get_bench_command; use crate::run::runner::helpers::introspected_golang; use crate::run::runner::helpers::introspected_nodejs; use crate::run::runner::helpers::run_command_with_log_pipe::run_command_with_log_pipe; -use crate::run::runner::helpers::run_with_sudo::validated_sudo_command; +use crate::run::runner::helpers::run_with_sudo::wrap_with_sudo; use crate::run::runner::{ExecutorName, RunData}; use crate::run::{check_system::SystemInfo, config::Config}; use async_trait::async_trait; @@ -100,7 +101,7 @@ impl WallTimeExecutor { fn walltime_bench_cmd( config: &Config, run_data: &RunData, - ) -> Result<(NamedTempFile, NamedTempFile, String)> { + ) -> Result<(NamedTempFile, NamedTempFile, CommandBuilder)> { let bench_cmd = get_bench_command(config)?; let system_env = get_exported_system_env()?; @@ -136,25 +137,28 @@ impl WallTimeExecutor { file }; - let quiet_flag = if is_codspeed_debug_enabled() { - "--quiet" - } else { - "" - }; - + let mut cmd_builder = CommandBuilder::new("systemd-run"); + if let Some(cwd) = &config.working_directory { + let abs_cwd = canonicalize(cwd)?; + cmd_builder.current_dir(abs_cwd); + } + if !is_codspeed_debug_enabled() { + cmd_builder.arg("--quiet"); + } // Remarks: // - We're using --scope so that perf is able to capture the events of the benchmark process. // - We can't user `--user` here because we need to run in `codspeed.slice`, otherwise we'd run in // user.slice` (which is isolated). We can use `--gid` and `--uid` to run the command as the current user. // - We must use `bash` here instead of `sh` since `source` isn't available when symlinked to `dash`. // - We have to pass the environment variables because `--scope` only inherits the system and not the user environment variables. - let uid = nix::unistd::Uid::current().as_raw(); - let gid = nix::unistd::Gid::current().as_raw(); - let cmd = format!( - "systemd-run {quiet_flag} --scope --slice=codspeed.slice --same-dir --uid={uid} --gid={gid} -- bash {}", - script_file.path().display() - ); - Ok((env_file, script_file, cmd)) + cmd_builder.arg("--slice=codspeed.slice"); + cmd_builder.arg("--scope"); + cmd_builder.arg("--same-dir"); + cmd_builder.arg(format!("--uid={}", nix::unistd::Uid::current().as_raw())); + cmd_builder.arg(format!("--gid={}", nix::unistd::Gid::current().as_raw())); + cmd_builder.args(["--", "bash"]); + cmd_builder.arg(script_file.path()); + Ok((env_file, script_file, cmd_builder)) } } @@ -179,25 +183,18 @@ impl Executor for WallTimeExecutor { run_data: &RunData, _mongo_tracer: &Option, ) -> Result<()> { - let mut cmd = validated_sudo_command()?; - - if let Some(cwd) = &config.working_directory { - let abs_cwd = canonicalize(cwd)?; - cmd.current_dir(abs_cwd); - } - let status = { let _guard = HookScriptsGuard::setup(); - let (_env_file, _script_file, bench_cmd) = Self::walltime_bench_cmd(config, run_data)?; + let (_env_file, _script_file, cmd_builder) = + WallTimeExecutor::walltime_bench_cmd(config, run_data)?; if let Some(perf) = &self.perf && config.enable_perf { - perf.run(cmd, &bench_cmd, config).await + perf.run(cmd_builder, config).await } else { - cmd.args(["sh", "-c", &bench_cmd]); + let cmd = wrap_with_sudo(cmd_builder)?.build(); debug!("cmd: {cmd:?}"); - run_command_with_log_pipe(cmd).await } }; diff --git a/src/run/runner/wall_time/perf/mod.rs b/src/run/runner/wall_time/perf/mod.rs index ad76f06d..17d0cfb1 100644 --- a/src/run/runner/wall_time/perf/mod.rs +++ b/src/run/runner/wall_time/perf/mod.rs @@ -3,9 +3,11 @@ use crate::prelude::*; use crate::run::UnwindingMode; use crate::run::config::Config; +use crate::run::runner::helpers::command::CommandBuilder; use crate::run::runner::helpers::env::is_codspeed_debug_enabled; use crate::run::runner::helpers::run_command_with_log_pipe::run_command_with_log_pipe_and_callback; use crate::run::runner::helpers::run_with_sudo::run_with_sudo; +use crate::run::runner::helpers::run_with_sudo::wrap_with_sudo; use crate::run::runner::valgrind::helpers::ignored_objects_path::get_objects_path_to_ignore; use crate::run::runner::valgrind::helpers::perf_maps::harvest_perf_maps_for_pids; use crate::run::runner::wall_time::perf::debug_info::ProcessDebugInfo; @@ -25,7 +27,6 @@ use runner_shared::metadata::PerfMetadata; use runner_shared::unwind_data::UnwindData; use std::collections::HashSet; use std::path::{Path, PathBuf}; -use std::process::Command; use std::time::Duration; use std::{cell::OnceCell, collections::HashMap, process::ExitStatus}; @@ -67,12 +68,12 @@ impl PerfRunner { // Allow access to kernel symbols if sysctl_read("kernel.kptr_restrict")? != 0 { - run_with_sudo(&["sysctl", "-w", "kernel.kptr_restrict=0"])?; + run_with_sudo("sysctl", ["-w", "kernel.kptr_restrict=0"])?; } // Allow non-root profiling if sysctl_read("kernel.perf_event_paranoid")? != -1 { - run_with_sudo(&["sysctl", "-w", "kernel.perf_event_paranoid=-1"])?; + run_with_sudo("sysctl", ["-w", "kernel.perf_event_paranoid=-1"])?; } Ok(()) @@ -86,8 +87,7 @@ impl PerfRunner { pub async fn run( &self, - mut cmd: Command, - bench_cmd: &str, + mut cmd_builder: CommandBuilder, config: &Config, ) -> anyhow::Result { let perf_fifo = PerfFifo::new()?; @@ -117,19 +117,14 @@ impl PerfRunner { }; debug!("Using call graph mode: {cg_mode:?}"); - let quiet_flag = if !is_codspeed_debug_enabled() { - "--quiet" - } else { - "" - }; - let working_perf_executable = get_working_perf_executable().context("Failed to find a working perf executable")?; - - let perf_args = [ - working_perf_executable.as_str(), - "record", - quiet_flag, + let mut perf_wrapper_builder = CommandBuilder::new(working_perf_executable); + perf_wrapper_builder.arg("record"); + if !is_codspeed_debug_enabled() { + perf_wrapper_builder.arg("--quiet"); + } + perf_wrapper_builder.args([ "--timestamp", // Required for matching the markers and URIs to the samples. "-k", @@ -146,16 +141,16 @@ impl PerfRunner { ), &format!("--output={PERF_DATA_PATH}"), "--", - bench_cmd, - ]; - - cmd.args(["sh", "-c", &perf_args.join(" ")]); + ]); + cmd_builder.wrap_with(perf_wrapper_builder); + let cmd = wrap_with_sudo(cmd_builder)?.build(); debug!("cmd: {cmd:?}"); let on_process_started = async |_| -> anyhow::Result<()> { let data = Self::handle_fifo(runner_fifo, perf_fifo).await?; - let _ = self.benchmark_data.set(data); - + self.benchmark_data.set(data).unwrap_or_else(|_| { + error!("Failed to set benchmark data in PerfRunner"); + }); Ok(()) }; run_command_with_log_pipe_and_callback(cmd, on_process_started).await @@ -165,16 +160,18 @@ impl PerfRunner { let start = std::time::Instant::now(); // We ran perf with sudo, so we have to change the ownership of the perf.data - run_with_sudo(&[ + run_with_sudo( "chown", - "-R", - &format!( - "{}:{}", - nix::unistd::Uid::current(), - nix::unistd::Gid::current() - ), - PERF_DATA_PATH, - ])?; + [ + "-R", + &format!( + "{}:{}", + nix::unistd::Uid::current(), + nix::unistd::Gid::current() + ), + PERF_DATA_PATH, + ], + )?; // Copy the perf data to the profile folder let perf_data_dest = profile_folder.join("perf.data"); diff --git a/src/run/runner/wall_time/perf/perf_executable.rs b/src/run/runner/wall_time/perf/perf_executable.rs index 1a6d2d5f..6da51cd6 100644 --- a/src/run/runner/wall_time/perf/perf_executable.rs +++ b/src/run/runner/wall_time/perf/perf_executable.rs @@ -1,13 +1,13 @@ use crate::prelude::*; -use std::process::Command; +use std::{ffi::OsString, process::Command}; const FIND_PERF_CMD: &str = "find /usr/lib -executable -path \"/usr/lib/linux-tools-*/perf\" | sort | tail -n1"; /// Attempts to find the path to the `perf` executable that is installed and working. /// Returns None if `perf` is not installed or not functioning correctly. -pub fn get_working_perf_executable() -> Option { +pub fn get_working_perf_executable() -> Option { let is_installed = Command::new("which") .arg("perf") .output() @@ -23,7 +23,7 @@ pub fn get_working_perf_executable() -> Option { .output() .is_ok_and(|output| output.status.success()) { - return Some("perf".to_string()); + return Some("perf".into()); } else { // The following is a workaround for this outstanding Ubuntu issue: https://bugs.launchpad.net/ubuntu/+source/linux-hwe-6.14/+bug/2117159/ debug!( @@ -53,7 +53,7 @@ pub fn get_working_perf_executable() -> Option { .trim() .to_string(); debug!("Found perf version from alternative path: {version}"); - return Some(path); + return Some(path.into()); } } }