shlex the env var names

use Vec<&str> due to IntoIterator issues with &[&str]
pantsbuild · Jan 3, 2025 · 476049e · 476049e
1 parent 266645c
commit 476049e
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 9 deletions.
diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs
@@ -1028,7 +1028,7 @@ fn maybe_make_wrapper_script(
     append_only_caches_base_path: Option<&str>,
     working_directory: Option<&str>,
     sandbox_root_token: Option<&str>,
-    env_vars_to_substitute: &[&str],
+    env_vars_to_substitute: Vec<&str>,
 ) -> Result<Option<String>, String> {
     let caches_fragment = match append_only_caches_base_path {
         Some(base_path) if !caches.is_empty() => {
@@ -1097,7 +1097,8 @@ fn maybe_make_wrapper_script(
             token
         );
         if !env_vars_to_substitute.is_empty() {
-            let env_vars_str = env_vars_to_substitute.join(" "); // TODO: shlex the strings
+            let env_vars_str = shlex::try_join(env_vars_to_substitute)
+                .map_err(|e| format!("Failed to shell quote environment names: {e}"))?;
             writeln!(&mut fragment, "for env_var in {env_vars_str}; do")
                 .map_err(|err| format!("write! failed: {err}"))?;
             writeln!(
@@ -1189,7 +1190,7 @@ pub async fn make_execute_request(
             append_only_caches_base_path,
             req.working_directory.as_ref().and_then(|p| p.to_str()),
             sandbox_root_token,
-            env_var_with_chroot_marker_refs.as_slice(),
+            env_var_with_chroot_marker_refs,
         )?
     };
     let wrapper_script_digest_opt = if let Some(script) = wrapper_script_content_opt {

diff --git a/src/rust/engine/process_execution/src/tests.rs b/src/rust/engine/process_execution/src/tests.rs
@@ -182,7 +182,7 @@ async fn wrapper_script_supports_append_only_caches() {
         dummy_caches_base_path.path().to_str(),
         Some(SUBDIR_NAME),
         None,
-        &[],
+        vec![],
     )
     .unwrap()
     .unwrap();
@@ -243,7 +243,7 @@ async fn wrapper_script_supports_append_only_caches() {
 async fn wrapper_script_supports_sandbox_root_replacements_in_args() {
     let caches = BTreeMap::new();
 
-    let script_content = maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), &[])
+    let script_content = maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), vec![])
         .unwrap()
         .unwrap();
 
@@ -287,10 +287,15 @@ async fn wrapper_script_supports_sandbox_root_replacements_in_args() {
 async fn wrapper_script_supports_sandbox_root_replacements_in_environmenbt() {
     let caches = BTreeMap::new();
 
-    let script_content =
-        maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), &["TEST_FILE_PATH"])
-            .unwrap()
-            .unwrap();
+    let script_content = maybe_make_wrapper_script(
+        &caches,
+        None,
+        None,
+        Some("__ROOT__"),
+        vec!["TEST_FILE_PATH"],
+    )
+    .unwrap()
+    .unwrap();
 
     let dummy_sandbox_path = TempDir::new().unwrap();
     let script_path = dummy_sandbox_path.path().join("wrapper");