diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index bd84f1d8773..49229d0e6b5 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -3006,6 +3006,7 @@ dependencies = [ "sha2", "sharded_lmdb", "shell-quote", + "shlex", "store", "strum", "strum_macros", diff --git a/src/rust/engine/process_execution/Cargo.toml b/src/rust/engine/process_execution/Cargo.toml index 909db080c35..7a0db6bc750 100644 --- a/src/rust/engine/process_execution/Cargo.toml +++ b/src/rust/engine/process_execution/Cargo.toml @@ -46,6 +46,7 @@ once_cell = { workspace = true } rand = { workspace = true } prost = { workspace = true } prost-types = { workspace = true } +shlex = { workspace = true } strum = { workspace = true } strum_macros = { workspace = true } tonic = { workspace = true, features = ["transport", "codegen", "tls", "tls-roots", "prost"] } diff --git a/src/rust/engine/process_execution/remote/src/remote_tests.rs b/src/rust/engine/process_execution/remote/src/remote_tests.rs index af372c50a4b..33e987518ad 100644 --- a/src/rust/engine/process_execution/remote/src/remote_tests.rs +++ b/src/rust/engine/process_execution/remote/src/remote_tests.rs @@ -724,7 +724,7 @@ async fn make_execute_request_with_append_only_caches() { input_root_digest: Some( (Digest::new( Fingerprint::from_hex_string( - "92f5d2ff07cb6cdf4a70f2d6392781b482cd587b9dd69d6729ac73eb54110a69", + "9f4ad1839decdee5e0b01d8ab2562302e42d590f331c3efb747c17f1936bedd4", ) .unwrap(), 178, @@ -739,7 +739,7 @@ async fn make_execute_request_with_append_only_caches() { action_digest: Some( (&Digest::new( Fingerprint::from_hex_string( - "e4196db365556cbeed4941845f448cfafc1fabb76b3c476c3f378f358235d3c4", + "18a5a53ad2909432f848f161af2b6886659b4047d608742b64a80f0879dc0a69", ) .unwrap(), 146, @@ -752,7 +752,7 @@ async fn make_execute_request_with_append_only_caches() { let want_input_root_digest = DirectoryDigest::from_persisted_digest(Digest::new( Fingerprint::from_hex_string( - "92f5d2ff07cb6cdf4a70f2d6392781b482cd587b9dd69d6729ac73eb54110a69", + "9f4ad1839decdee5e0b01d8ab2562302e42d590f331c3efb747c17f1936bedd4", ) .unwrap(), 178, diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 81875b2bf85..19801d7db29 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -1021,27 +1021,39 @@ fn make_wrapper_for_append_only_caches( let mut script = String::new(); writeln!(&mut script, "#!/bin/sh").map_err(|err| format!("write! failed: {err:?}"))?; + fn quote_path(path: &Path) -> Result { + let as_str = path + .to_str() + .ok_or_else(|| "Failed to convert path".to_string())?; + let quoted = + shlex::try_quote(as_str).map_err(|e| format!("Failed to convert path: {e}"))?; + Ok(quoted.to_string()) + } + // Setup the append-only caches. for (cache_name, path) in caches { - writeln!( - &mut script, - "/bin/mkdir -p '{}/{}'", - base_path, - cache_name.name() - ) - .map_err(|err| format!("write! failed: {err:?}"))?; + let cache_path = { + let mut p = PathBuf::new(); + p.push(base_path); + p.push(cache_name.name()); + + quote_path(&p)? + }; + writeln!(&mut script, "/bin/mkdir -p {cache_path}",) + .map_err(|err| format!("write! failed: {err:?}"))?; + if let Some(parent) = path.parent() { if !parent.as_os_str().is_empty() { - writeln!(&mut script, "/bin/mkdir -p '{}'", parent.to_string_lossy()) + let parent_quoted = quote_path(parent)?; + writeln!(&mut script, "/bin/mkdir -p {}", &parent_quoted) .map_err(|err| format!("write! failed: {err}"))?; } } writeln!( &mut script, - "/bin/ln -s '{}/{}' '{}'", - base_path, - cache_name.name(), - path.as_path().to_string_lossy() + "/bin/ln -s {} {}", + &cache_path, + quote_path(path.as_path())? ) .map_err(|err| format!("write! failed: {err}"))?; } @@ -1052,16 +1064,18 @@ fn make_wrapper_for_append_only_caches( // field on the `ExecuteRequest` so that this wrapper script can operate in the input root // first. if let Some(path) = working_directory { + let quoted_path = + shlex::try_quote(path).map_err(|e| format!("Failed to convert path: {e}"))?; writeln!( &mut script, concat!( - "cd '{0}'\n", + "cd {0}\n", "if [ \"$?\" != 0 ]; then\n", - " echo \"pants-wrapper: Failed to change working directory to: {0}\" 1>&2\n", + " echo \"pants-wrapper: Failed to change working directory to: \" {0} 1>&2\n", " exit 1\n", "fi\n", ), - path + quoted_path ) .map_err(|err| format!("write! failed: {err}"))?; } diff --git a/src/rust/engine/process_execution/src/tests.rs b/src/rust/engine/process_execution/src/tests.rs index 8bfbb5103d4..85f38ed2320 100644 --- a/src/rust/engine/process_execution/src/tests.rs +++ b/src/rust/engine/process_execution/src/tests.rs @@ -161,22 +161,25 @@ fn process_result_metadata_time_saved_from_cache() { #[tokio::test] async fn test_make_wrapper_for_append_only_caches_success() { + const CACHE_NAME: &str = "test_cache"; + const SUBDIR_NAME: &str = "a subdir"; // Space intentionally included to test shell quoting. + let mut caches = BTreeMap::new(); caches.insert( - CacheName::new("test_cache".into()).unwrap(), + CacheName::new(CACHE_NAME.into()).unwrap(), RelativePath::new("foo").unwrap(), ); let dummy_caches_base_path = TempDir::new().unwrap(); let dummy_sandbox_path = TempDir::new().unwrap(); - tokio::fs::create_dir_all(dummy_sandbox_path.path().join("a-subdir")) + tokio::fs::create_dir_all(dummy_sandbox_path.path().join(SUBDIR_NAME)) .await .unwrap(); let script_content = make_wrapper_for_append_only_caches( &caches, dummy_caches_base_path.path().to_str().unwrap(), - Some("a-subdir"), + Some(SUBDIR_NAME), ) .unwrap(); @@ -204,11 +207,11 @@ async fn test_make_wrapper_for_append_only_caches_success() { panic!("Wrapper script failed to run: {}", output.status); } - let cache_dir_path = dummy_caches_base_path.path().join("test_cache"); + let cache_dir_path = dummy_caches_base_path.path().join(CACHE_NAME); let cache_dir_metadata = tokio::fs::metadata(&cache_dir_path).await.unwrap(); assert!( cache_dir_metadata.is_dir(), - "test_cache directory exists in caches base path" + "`test_cache` directory exists in caches base path" ); let cache_symlink_path = dummy_sandbox_path.path().join("foo"); @@ -223,7 +226,7 @@ async fn test_make_wrapper_for_append_only_caches_success() { assert_eq!(&link_target, &cache_dir_path); let test_file_metadata = - tokio::fs::metadata(dummy_sandbox_path.path().join("a-subdir/file.txt")) + tokio::fs::metadata(dummy_sandbox_path.path().join(SUBDIR_NAME).join("file.txt")) .await .unwrap(); assert!(