use shell quoting for directories in remote execution wrapper script (#…

…21801)

Use the `shlex` crate to properly shell quote directories emitted into a
remote execution wrapper script, instead of using lossy conversions.

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"] }

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,

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<String, String> {
+        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}"))?;
     }

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!(