From 527b10973a1eaf12e28b8b2ef70b2e2f1db55385 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Wed, 16 Mar 2022 15:32:04 -0700 Subject: [PATCH] Fix exclusive spawn mechanism for relative paths and working directories. (#14812) The `exclusive_spawn` facility to avoid/retry for `ExecutableFileBusy` / "Text file busy" is triggered by having materialized `arg[0]` of a process into the process sandbox. But in the presence of a `Process::working_directory` and a relative path as `arg[0]`, the facility was not being triggered (since validation of `arg[0]` as a `RelativePath` would fail due to it escaping its root). This relates to #13424 (which would remove the need for the `exclusive_spawn` facility), but does not fix it: only ensure that we handle an existing known case. [ci skip-build-wheels] --- src/rust/engine/process_execution/src/lib.rs | 8 ++ .../engine/process_execution/src/local.rs | 14 ++- .../process_execution/src/local_tests.rs | 91 +++++++++++++++++-- 3 files changed, 98 insertions(+), 15 deletions(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index bf5785393b1..eeec86cc773 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -462,6 +462,14 @@ impl Process { self } + /// + /// Replaces the working_directory for this process. + /// + pub fn working_directory(mut self, working_directory: Option) -> Process { + self.working_directory = working_directory; + self + } + /// /// Replaces the output files for this process. /// diff --git a/src/rust/engine/process_execution/src/local.rs b/src/rust/engine/process_execution/src/local.rs index 470b7e3e0b4..cdb4ed83800 100644 --- a/src/rust/engine/process_execution/src/local.rs +++ b/src/rust/engine/process_execution/src/local.rs @@ -634,13 +634,17 @@ pub async fn prepare_workdir( // Capture argv0 as the executable path so that we can test whether we have created it in the // sandbox. - let maybe_executable_path = RelativePath::new(&req.argv[0]).map(|relative_path| { - if let Some(working_directory) = &req.working_directory { - working_directory.join(relative_path) + let maybe_executable_path = { + let mut executable_path = PathBuf::from(&req.argv[0]); + if executable_path.is_relative() { + if let Some(working_directory) = &req.working_directory { + executable_path = working_directory.as_ref().join(executable_path) + } + Some(executable_path) } else { - relative_path + None } - }); + }; // Start with async materialization of input snapshots, followed by synchronous materialization // of other configured inputs. Note that we don't do this in parallel, as that might cause diff --git a/src/rust/engine/process_execution/src/local_tests.rs b/src/rust/engine/process_execution/src/local_tests.rs index f1c21785530..7eb478e24f7 100644 --- a/src/rust/engine/process_execution/src/local_tests.rs +++ b/src/rust/engine/process_execution/src/local_tests.rs @@ -1,25 +1,24 @@ -use std; use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::path::PathBuf; use std::str; use std::time::Duration; -use hashing::EMPTY_DIGEST; use maplit::hashset; use shell_quote::bash; use spectral::{assert_that, string::StrAssertions}; -use store::Store; -use tempfile; use tempfile::TempDir; -use testutil; + +use hashing::EMPTY_DIGEST; +use store::Store; use testutil::data::{TestData, TestDirectory}; use testutil::path::{find_bash, which}; use testutil::{owned_string_vec, relative_paths}; use workunit_store::{RunningWorkunit, WorkunitStore}; use crate::{ - CacheName, CommandRunner as CommandRunnerTrait, Context, FallibleProcessResultWithPlatform, - ImmutableInputs, InputDigests, NamedCaches, Platform, Process, RelativePath, + local, CacheName, CommandRunner as CommandRunnerTrait, Context, + FallibleProcessResultWithPlatform, ImmutableInputs, InputDigests, NamedCaches, Platform, Process, + RelativePath, }; #[derive(PartialEq, Debug)] @@ -642,6 +641,77 @@ async fn immutable_inputs() { assert_eq!(result.original.exit_code, 0); } +#[tokio::test] +async fn prepare_workdir_exclusive_relative() { + // Test that we detect that we should should exclusive spawn when a relative path that points + // outside of a working directory is used. + let _ = WorkunitStore::setup_for_tests(); + + let store_dir = TempDir::new().unwrap(); + let executor = task_executor::Executor::new(); + let store = Store::local_only(executor.clone(), store_dir.path()).unwrap(); + let (_caches_dir, named_caches, immutable_inputs) = + named_caches_and_immutable_inputs(store.clone()); + + store + .store_file_bytes(TestData::roland().bytes(), false) + .await + .expect("Error saving file bytes"); + store + .store_file_bytes(TestData::catnip().bytes(), false) + .await + .expect("Error saving file bytes"); + store + .record_directory(&TestDirectory::recursive().directory(), true) + .await + .expect("Error saving directory"); + store + .record_directory(&TestDirectory::containing_roland().directory(), true) + .await + .expect("Error saving directory"); + + let work_dir = TempDir::new().unwrap(); + + // NB: This path is not marked executable, but that isn't (currently) relevant to the heuristic. + let mut process = Process::new(vec!["../treats.ext".to_owned()]) + .working_directory(Some(RelativePath::new("cats").unwrap())); + process.input_digests = InputDigests::new( + &store, + TestDirectory::recursive().digest(), + BTreeMap::new(), + vec![], + ) + .await + .unwrap(); + + let exclusive_spawn = local::prepare_workdir( + work_dir.path().to_owned(), + &process, + TestDirectory::recursive().digest(), + Context::default(), + store, + executor, + &named_caches, + &immutable_inputs, + ) + .await + .unwrap(); + + assert_eq!(exclusive_spawn, true); +} + +fn named_caches_and_immutable_inputs(store: Store) -> (TempDir, NamedCaches, ImmutableInputs) { + let root = TempDir::new().unwrap(); + let root_path = root.path().to_owned(); + let named_cache_dir = root_path.join("named"); + + ( + root, + NamedCaches::new(named_cache_dir), + ImmutableInputs::new(store, &root_path).unwrap(), + ) +} + async fn run_command_locally(req: Process) -> Result { let (_, mut workunit) = WorkunitStore::setup_for_tests(); let work_dir = TempDir::new().unwrap(); @@ -658,16 +728,17 @@ async fn run_command_locally_in_dir( executor: Option, ) -> Result { let store_dir = TempDir::new().unwrap(); - let named_cache_dir = TempDir::new().unwrap(); let executor = executor.unwrap_or_else(|| task_executor::Executor::new()); let store = store.unwrap_or_else(|| Store::local_only(executor.clone(), store_dir.path()).unwrap()); + let (_caches_dir, named_caches, immutable_inputs) = + named_caches_and_immutable_inputs(store.clone()); let runner = crate::local::CommandRunner::new( store.clone(), executor.clone(), dir.clone(), - NamedCaches::new(named_cache_dir.path().to_owned()), - ImmutableInputs::new(store.clone(), &dir)?, + named_caches, + immutable_inputs, cleanup, ); let original = runner.run(Context::default(), workunit, req.into()).await?;