Repository: a-scie/jump (rust)

Issue:
Env var removal via regex blows up when the env contains non-utf8 env vars or values
The issue is here:
https://github.com/a-scie/jump/blob/faa65b7875c1eee5fa902d81138782b6f1d00232/jump/src/process.rs#L65-L66

The `env::vars()` call effectively asserts that the env will be fully utf8 decodable. Instead the loop should be over `env::vars_os()` which works in OsString and does not try to do conversions.

Pull request:
Fix regex env removal vs. non-utf8 env entries.
Previously the scie-jump boot would crash when a manifest requested
regex removals and the ambient environment contained non-utf8 entries.

Fixes #105

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index 33b7373..1cd63db 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -1,5 +1,10 @@
 # Release Notes
 
+## 0.11.1
+
+This release fixes a bug handling environment variable removal via regex when the environment
+contains non-utf8 entries.
+
 ## 0.11.0
 
 Support is added for `{scie.env.*}` placeholders referring to environment variables defined in the
diff --git a/Cargo.lock b/Cargo.lock
index 3a2f1be..ec98f47 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -164,8 +164,8 @@ checksum = "191d9573962933b4027f932c600cd252ce27a8ad5979418fe78e43c07996f27b"
 dependencies = [
  "heck",
  "proc-macro2",
- "quote 1.0.26",
- "syn 2.0.10",
+ "quote 1.0.28",
+ "syn 2.0.18",
 ]
 
 [[package]]
@@ -217,6 +217,16 @@ dependencies = [
  "typenum",
 ]
 
+[[package]]
+name = "ctor"
+version = "0.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "990a40740adf249724a6000c0fc4bd574712f50bb17c2d6f6cec837ae2f0ee75"
+dependencies = [
+ "quote 1.0.28",
+ "syn 2.0.18",
+]
+
 [[package]]
 name = "digest"
 version = "0.10.5"
@@ -455,13 +465,15 @@ dependencies = [
 
 [[package]]
 name = "jump"
-version = "0.11.0"
+version = "0.11.1"
 dependencies = [
  "bstr",
  "byteorder",
  "bzip2",
+ "ctor",
  "dirs",
  "dotenv",
+ "env_logger",
  "fd-lock",
  "flate2",
  "indexmap",
@@ -469,6 +481,8 @@ dependencies = [
  "log",
  "logging_timer",
  "memmap",
+ "once_cell",
+ "parking_lot",
  "regex",
  "serde",
  "serde_json",
@@ -501,6 +515,16 @@ version = "0.3.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "d59d8c75012853d2e872fb56bc8a2e53718e2cafe1a4c823143141c6d90c322f"
 
+[[package]]
+name = "lock_api"
+version = "0.4.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "435011366fe56583b16cf956f9df0095b405b82d76425bc8981c0e22e60ec4df"
+dependencies = [
+ "autocfg",
+ "scopeguard",
+]
+
 [[package]]
 name = "log"
 version = "0.4.18"
@@ -525,7 +549,7 @@ checksum = "10a9062912d7952c5588cc474795e0b9ee008e7e6781127945b85413d4b99d81"
 dependencies = [
  "log",
  "proc-macro2",
- "quote 1.0.26",
+ "quote 1.0.28",
  "syn 1.0.103",
 ]
 
@@ -599,9 +623,9 @@ dependencies = [
 
 [[package]]
 name = "once_cell"
-version = "1.15.0"
+version = "1.17.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e82dad04139b71a90c080c8463fe0dc7902db5192d939bd0950f074d014339e1"
+checksum = "9670a07f94779e00908f3e686eab508878ebb390ba6e604d3a284c00e8d0487b"
 
 [[package]]
 name = "package"
@@ -614,6 +638,29 @@ dependencies = [
  "sha2",
 ]
 
+[[package]]
+name = "parking_lot"
+version = "0.12.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3742b2c103b9f06bc9fff0a37ff4912935851bee6d36f3c02bcc755bcfec228f"
+dependencies = [
+ "lock_api",
+ "parking_lot_core",
+]
+
+[[package]]
+name = "parking_lot_core"
+version = "0.9.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9069cbb9f99e3a5083476ccb29ceb1de18b9118cafa53e90c9551235de2b9521"
+dependencies = [
+ "cfg-if",
+ "libc",
+ "redox_syscall 0.2.16",
+ "smallvec",
+ "windows-sys 0.45.0",
+]
+
 [[package]]
 name = "pin-utils"
 version = "0.1.0"
@@ -649,9 +696,9 @@ checksum = "7be55bf0ae1635f4d7c7ddd6efc05c631e98a82104a73d35550bbc52db960027"
 
 [[package]]
 name = "proc-macro2"
-version = "1.0.54"
+version = "1.0.59"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e472a104799c74b514a57226160104aa483546de37e839ec50e3c2e41dd87534"
+checksum = "6aeca18b86b413c660b781aa319e4e2648a3e6f9eadc9b47e9038e6fe9f3451b"
 dependencies = [
  "unicode-ident",
 ]
@@ -664,9 +711,9 @@ checksum = "7a6e920b65c65f10b2ae65c831a81a073a89edd28c7cce89475bff467ab4167a"
 
 [[package]]
 name = "quote"
-version = "1.0.26"
+version = "1.0.28"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "4424af4bf778aae2051a77b60283332f386554255d722233d09fbfc7e30da2fc"
+checksum = "1b9ab9c7eadfd8df19006f1cf1a4aed13540ed5cbc047010ece5826e10825488"
 dependencies = [
  "proc-macro2",
 ]
@@ -766,7 +813,7 @@ dependencies = [
 
 [[package]]
 name = "scie-jump"
-version = "0.11.0"
+version = "0.11.1"
 dependencies = [
  "bstr",
  "env_logger",
@@ -779,6 +826,12 @@ dependencies = [
  "zip",
 ]
 
+[[package]]
+name = "scopeguard"
+version = "1.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd"
+
 [[package]]
 name = "serde"
 version = "1.0.147"
@@ -795,7 +848,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "4f1d362ca8fc9c3e3a7484440752472d68a6caa98f1ab81d99b5dfe517cec852"
 dependencies = [
  "proc-macro2",
- "quote 1.0.26",
+ "quote 1.0.28",
  "syn 1.0.103",
 ]
 
@@ -821,6 +874,12 @@ dependencies = [
  "digest",
 ]
 
+[[package]]
+name = "smallvec"
+version = "1.10.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a507befe795404456341dfab10cef66ead4c041f62b8b11bbb92bffe5d0953e0"
+
 [[package]]
 name = "static_assertions"
 version = "1.1.0"
@@ -861,18 +920,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "a864042229133ada95abf3b54fdc62ef5ccabe9515b64717bcb9a1919e59445d"
 dependencies = [
  "proc-macro2",
- "quote 1.0.26",
+ "quote 1.0.28",
  "unicode-ident",
 ]
 
 [[package]]
 name = "syn"
-version = "2.0.10"
+version = "2.0.18"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5aad1363ed6d37b84299588d62d3a7d95b5a5c2d9aad5c85609fda12afaa1f40"
+checksum = "32d41677bcbe24c20c52e7c70b0d8db04134c5d1066bf98662e2871ad200ea3e"
 dependencies = [
  "proc-macro2",
- "quote 1.0.26",
+ "quote 1.0.28",
  "unicode-ident",
 ]
 
@@ -916,7 +975,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "982d17546b47146b28f7c22e3d08465f6b8903d0ea13c1660d9d84a6e7adcdbb"
 dependencies = [
  "proc-macro2",
- "quote 1.0.26",
+ "quote 1.0.28",
  "syn 1.0.103",
 ]
 
diff --git a/Cargo.toml b/Cargo.toml
index 10fdd03..bb4494b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -5,7 +5,7 @@ members = [
 
 [package]
 name = "scie-jump"
-version = "0.11.0"
+version = "0.11.1"
 description = "The self contained interpreted executable launcher."
 authors = [
     "John Sirois <john.sirois@gmail.com>",
@@ -21,6 +21,7 @@ codegen-units = 1
 
 [workspace.dependencies]
 bstr = "1.5"
+env_logger = { version = "0.10", default-features = false }
 log = "0.4"
 logging_timer = "1.1"
 tempfile = "3.5"
@@ -36,7 +37,7 @@ features = ["deflate"]
 
 [dependencies]
 bstr = { workspace = true }
-env_logger = { version = "0.10", default-features = false }
+env_logger = { workspace = true }
 jump = { path = "jump" }
 log = { workspace = true }
 logging_timer = { workspace = true }
diff --git a/examples/python/lift.linux-x86_64.json b/examples/python/lift.linux-x86_64.json
index 6e831da..13c3b32 100644
--- a/examples/python/lift.linux-x86_64.json
+++ b/examples/python/lift.linux-x86_64.json
@@ -15,7 +15,7 @@
           "repl": {
             "description": "A Python repl with Pants (minus plugins) available for inspection.",
             "env": {
-              "=PYTHONPATH": null
+              "PYTHON.*": null
             },
             "exe": "{scie.bindings.venv}/venv-3.{scie.env.PYTHON_MINOR=9}/bin/python"
           }
diff --git a/examples/python/lift.macos-x86_64.json b/examples/python/lift.macos-x86_64.json
index a3beb3c..f8b884d 100644
--- a/examples/python/lift.macos-x86_64.json
+++ b/examples/python/lift.macos-x86_64.json
@@ -15,7 +15,7 @@
           "repl": {
             "description": "A Python repl with Pants (minus plugins) available for inspection.",
             "env": {
-              "=PYTHONPATH": null
+              "PYTHON.*": null
             },
             "exe": "{scie.bindings.venv}/venv-3.{scie.env.PYTHON_MINOR=9}/bin/python"
           }
diff --git a/jump/Cargo.toml b/jump/Cargo.toml
index 47b356e..0ee400a 100644
--- a/jump/Cargo.toml
+++ b/jump/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "jump"
-version = "0.11.0"
+version = "0.11.1"
 description = "The bulk of the scie-jump binary logic."
 authors = [
     "John Sirois <john.sirois@gmail.com>",
@@ -33,3 +33,9 @@ xz2 = "0.1"
 zip = { workspace = true }
 zstd = "0.12"
 walkdir = "2.3"
+
+[dev-dependencies]
+ctor = "0.2"
+env_logger = { workspace = true }
+once_cell = "1.17"
+parking_lot = "0.12"
diff --git a/jump/src/process.rs b/jump/src/process.rs
index 78d6919..06ad726 100644
--- a/jump/src/process.rs
+++ b/jump/src/process.rs
@@ -63,9 +63,20 @@ impl EnvVars {
                     removals.insert(name.to_owned());
                 }
                 EnvVar::RemoveMatching(regex) => {
-                    for (name, _) in env::vars() {
-                        if regex.is_match(name.as_str()) {
-                            removals.insert(name.into());
+                    for (name, _) in env::vars_os() {
+                        match name.into_string() {
+                            Ok(name_utf8) => {
+                                if regex.is_match(&name_utf8) {
+                                    removals.insert(name_utf8.into());
+                                }
+                            }
+                            Err(name_os) => {
+                                warn!(
+                                    "Cannot process env removal matching regex '{regex}' for \
+                                    non-utf8 env var name {name_os:?}",
+                                    regex = regex.as_str()
+                                )
+                            }
                         }
                     }
                 }
@@ -196,3 +207,133 @@ impl Process {
             })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::ffi::OsString;
+    use std::sync::Arc;
+
+    use once_cell::sync::Lazy;
+    use parking_lot::ReentrantMutex;
+
+    use crate::comparable_regex::ComparableRegex;
+    use crate::{EnvVar, EnvVars};
+
+    static ENV_LOCK: Lazy<Arc<ReentrantMutex<()>>> =
+        Lazy::new(|| Arc::new(ReentrantMutex::new(())));
+
+    #[ctor::ctor]
+    fn init() {
+        env_logger::init();
+    }
+
+    fn with_env<T>(func: T)
+    where
+        T: FnOnce() -> (),
+    {
+        let _env_lock = ENV_LOCK.lock();
+        func();
+    }
+
+    fn with_extra_env<T>(extra_env: &[(OsString, OsString)], func: T)
+    where
+        T: FnOnce() -> (),
+    {
+        with_env(|| {
+            let mut original_env = vec![];
+            for (name, value) in extra_env {
+                original_env.push((name, std::env::var_os(name)));
+                std::env::set_var(name, value);
+            }
+            func();
+            for (name, value) in original_env {
+                if let Some(val) = value {
+                    std::env::set_var(name, val);
+                } else {
+                    std::env::remove_var(name);
+                }
+            }
+        });
+    }
+
+    #[test]
+    fn to_env_vars_empty() {
+        with_env(|| {
+            assert_eq!(
+                Vec::<(OsString, Option<OsString>)>::new(),
+                EnvVars { vars: vec![] }.to_env_vars()
+            )
+        })
+    }
+
+    fn assert_to_env_vars() {
+        with_env(|| {
+            assert_eq!(
+                vec![("foo".into(), Some("bar".into()))],
+                EnvVars {
+                    vars: vec![
+                        EnvVar::Replace(("foo".into(), "bar".into())),
+                        EnvVar::RemoveMatching(ComparableRegex::try_from("__DNE__.*").unwrap())
+                    ]
+                }
+                .to_env_vars()
+            )
+        })
+    }
+
+    #[test]
+    fn to_env_vars() {
+        assert_to_env_vars()
+    }
+
+    #[cfg(windows)]
+    fn create_non_utf8_string() -> OsString {
+        use std::os::windows::ffi::OsStringExt;
+
+        // This value is the 1st high surrogate code point See Chapter 3 (Conformance) section 3.9,
+        // "UTF-8": https://www.unicode.org/versions/Unicode15.0.0/ch03.pdf
+        //
+        // > * Because surrogate code points are not Unicode scalar values, any UTF-8 byte
+        // >   sequence that would otherwise map to code points U+D800..U+DFFF is ill-
+        // >   formed.
+        //
+        // As such it is valid utf16 but we expect it to fail to convert to utf8.
+        OsString::from_wide(&[0xD800])
+    }
+
+    #[cfg(unix)]
+    fn create_non_utf8_string() -> OsString {
+        use std::os::unix::ffi::OsStringExt;
+
+        // This value is taken directly from the original repro example in
+        // https://github.com/pantsbuild/scie-pants/issues/198 that led here.
+        OsString::from_vec(vec![b'b', 0xA5, b'r'])
+    }
+
+    #[test]
+    fn to_env_vars_non_utf8() {
+        let non_utf8 = create_non_utf8_string();
+        assert!(non_utf8.clone().into_string().is_err());
+
+        with_extra_env(&[(non_utf8.clone(), "baz".into())], assert_to_env_vars);
+        with_extra_env(&[("baz".into(), non_utf8.clone())], assert_to_env_vars);
+        with_extra_env(&[(non_utf8.clone(), non_utf8.clone())], assert_to_env_vars);
+
+        with_extra_env(&[("baz".into(), non_utf8.clone())], || {
+            assert_eq!(
+                // N.B.: Our docs guaranty removals are done first, then defaults are set and
+                // finally overwrites are processed. This is regardless of the order of the env var
+                // entries in the lift manifest JSON document.
+                vec![("baz".into(), None), ("foo".into(), Some("bar".into()))],
+                EnvVars {
+                    vars: vec![
+                        EnvVar::Replace(("foo".into(), "bar".into())),
+                        EnvVar::RemoveMatching(ComparableRegex::try_from("^baz$").unwrap())
+                    ]
+                }
+                .to_env_vars(),
+                "Expected removal of an env var with a non-utf8 value to succeed."
+            )
+        })
+    }
+}


Test patch:
diff --git a/examples/python/test.sh b/examples/python/test.sh
index 96e07d8..96973ec 100644
--- a/examples/python/test.sh
+++ b/examples/python/test.sh
@@ -5,6 +5,8 @@
 source "${COMMON}"
 trap gc EXIT
 
+check_cmd env
+
 "${SCIE_JUMP}" "${LIFT}"
 gc "${PWD}/pants" "${PWD}/.pants.d" "${PWD}/.pids"
 
@@ -45,3 +47,20 @@ time RUST_LOG=debug ./pants-extra -V
 SCIE_BOOT=repl ./pants -c 'import sys; assert (3, 9) == sys.version_info[:2]'
 PYTHON_MINOR=8 SCIE_BOOT=repl ./pants -c 'import sys; assert (3, 8) == sys.version_info[:2]'
 PYTHON_MINOR=9 SCIE_BOOT=repl ./pants -c 'import sys; assert (3, 9) == sys.version_info[:2]'
+
+# Check non-utf8 env vars are handled: https://github.com/a-scie/jump/issues/105
+# The PEX_.* zap directive in the lift manifest needs to traverse all env vars and we insert some
+# non-utf8 env vars to ensure it skips past them.
+env \
+$'B\xa5R=FOO' \
+$'FOO=B\xa5R' \
+$'\xca\xfe\xba\xbe=B\xa5R' \
+RUST_LOG=warn \
+PEX_MODULE="foo.bar:baz" \
+PYTHONVERBOSE=x \
+SCIE_BOOT=repl \
+  ./pants -c '
+import os
+assert "PEX_MODULE" in os.environ, "PEX_.* are only scrubbed in the venv binding command."
+assert "PYTHONVERBOSE" not in os.environ, "Expected PYTHON.* to be scrubbed for the repl command."
+'