fix(core): read the Windows user PATH past REG_EXPAND_SZ quirks (#690)

* fix(core): read the Windows user PATH past REG_EXPAND_SZ quirks

`RegGetValueW` reports the unexpanded size of a `REG_EXPAND_SZ` value, so
a user `Path` that grows when its `%VAR%` references are expanded was
silently dropped. And when the terminating NUL leaked into the read
buffer with a stray UTF-16 unit behind it, `trim_end_matches('\0')`
left that unit in the string and `env::set_var("PATH", ...)` panicked
("strings passed to WinAPI cannot contain NULs", #683).

Re-read at the size the failed call reports, and cut the decoded string
at the first NUL instead of trimming trailing ones. Covered by a test
against a throwaway key under HKCU\Software.

* test: cover registry terminator corruption and refresh inspection groups

---------

Co-authored-by: l0ng-ai <[email protected]>

crates/ide-core/Cargo.toml

@@ -19,4 +19,4 @@ tokio-util.workspace = true
 tempfile.workspace = true
 
 [target.'cfg(windows)'.dependencies]
-windows-sys = { version = "0.61", features = ["Win32_Globalization", "Win32_System_Registry", "Win32_Foundation"] }
+windows-sys = { version = "0.61", features = ["Win32_Globalization", "Win32_Security", "Win32_System_Registry", "Win32_Foundation"] }

crates/ide-core/src/process.rs

@@ -245,10 +245,7 @@ pub fn find_executable(name: &str, search: &OsStr) -> Option<std::path::PathBuf>
 #[cfg(windows)]
 pub fn user_path() -> Option<OsString> {
     use windows_sys::Win32::Foundation::ERROR_SUCCESS;
-    use windows_sys::Win32::System::Registry::{
-        HKEY, HKEY_CURRENT_USER, KEY_READ, RRF_RT_REG_EXPAND_SZ, RRF_RT_REG_SZ, RegCloseKey, RegGetValueW,
-        RegOpenKeyExW,
-    };
+    use windows_sys::Win32::System::Registry::{HKEY, HKEY_CURRENT_USER, KEY_READ, RegCloseKey, RegOpenKeyExW};
 
     fn wide(s: &str) -> Vec<u16> {
         s.encode_utf16().chain(std::iter::once(0)).collect()
@@ -259,45 +256,53 @@ pub fn user_path() -> Option<OsString> {
         if RegOpenKeyExW(HKEY_CURRENT_USER, wide("Environment").as_ptr(), 0, KEY_READ, &mut key) != ERROR_SUCCESS {
             return None;
         }
-        let read = || {
-            // Only `Path`; `REG_EXPAND_SZ` is accepted so `%USERPROFILE%\…` entries expand.
-            let mut kind = 0u32;
-            let mut len = 0u32;
-            let name = wide("Path");
-            let status = RegGetValueW(
-                key,
-                std::ptr::null(),
-                name.as_ptr(),
-                RRF_RT_REG_SZ | RRF_RT_REG_EXPAND_SZ,
-                &mut kind,
-                std::ptr::null_mut(),
-                &mut len,
-            );
-            if status != ERROR_SUCCESS || len == 0 {
-                return None;
-            }
-            let mut buf = vec![0u16; (len as usize).div_ceil(2)];
-            let status = RegGetValueW(
-                key,
-                std::ptr::null(),
-                name.as_ptr(),
-                RRF_RT_REG_SZ | RRF_RT_REG_EXPAND_SZ,
-                &mut kind,
-                buf.as_mut_ptr() as *mut _,
-                &mut len,
-            );
-            if status != ERROR_SUCCESS {
-                return None;
-            }
-            let text = String::from_utf16_lossy(&buf);
-            Some(text.trim_end_matches('\0').to_owned())
-        };
-        let value = read();
+        let value = read_registry_string(key, &wide("Path"));
         RegCloseKey(key);
         value.filter(|v| !v.trim().is_empty()).map(OsString::from)
     }
 }
 
+/// The string of value `name` under open `key`: `REG_SZ` as-is, `REG_EXPAND_SZ` expanded
+/// against the current environment. `None` when the value is missing or unreadable.
+///
+/// A `REG_EXPAND_SZ` value's first size query reports the unexpanded size, so a value
+/// that grows when expanded is re-read at the size the failed call reports. And the
+/// terminating NUL can leak into the read buffer with a stray UTF-16 unit behind it —
+/// seen with a `REG_EXPAND_SZ` user `Path`, where it made `env::set_var("PATH", …)`
+/// panic with "strings passed to WinAPI cannot contain NULs" — so the text is cut at
+/// the first NUL, not just stripped of trailing ones.
+#[cfg(windows)]
+fn read_registry_string(key: windows_sys::Win32::System::Registry::HKEY, name: &[u16]) -> Option<String> {
+    use windows_sys::Win32::Foundation::{ERROR_MORE_DATA, ERROR_SUCCESS};
+    use windows_sys::Win32::System::Registry::{RRF_RT_REG_EXPAND_SZ, RRF_RT_REG_SZ, RegGetValueW};
+
+    const FLAGS: u32 = RRF_RT_REG_SZ | RRF_RT_REG_EXPAND_SZ;
+    let mut kind = 0u32;
+    let mut len = 0u32;
+    // SAFETY: `name` is NUL-terminated and the size query writes only `kind` and `len`.
+    let status =
+        unsafe { RegGetValueW(key, std::ptr::null(), name.as_ptr(), FLAGS, &mut kind, std::ptr::null_mut(), &mut len) };
+    if status != ERROR_SUCCESS || len == 0 {
+        return None;
+    }
+    let buf = loop {
+        let mut buf = vec![0u16; len as usize / 2 + 1];
+        // SAFETY: as above; the buffer holds at least the `len` bytes the call may write.
+        let status = unsafe {
+            RegGetValueW(key, std::ptr::null(), name.as_ptr(), FLAGS, &mut kind, buf.as_mut_ptr() as *mut _, &mut len)
+        };
+        if status == ERROR_SUCCESS {
+            break buf;
+        }
+        if status != ERROR_MORE_DATA || len == 0 {
+            return None;
+        }
+    };
+    let text = String::from_utf16_lossy(&buf);
+    let end = text.find('\0').unwrap_or(text.len());
+    Some(text[..end].to_owned())
+}
+
 /// The user's own `PATH` entries; `None` outside Windows (see the Windows impl).
 #[cfg(not(windows))]
 pub fn user_path() -> Option<OsString> {
@@ -334,6 +339,96 @@ mod tests {
         }
     }
 
+    #[cfg(windows)]
+    #[test]
+    fn registry_strings_expand_and_stay_terminator_free() {
+        use windows_sys::Win32::Foundation::ERROR_SUCCESS;
+        use windows_sys::Win32::System::Registry::{
+            HKEY, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE, REG_EXPAND_SZ, REG_SZ, RegCloseKey, RegCreateKeyExW,
+            RegDeleteTreeW, RegSetValueExW,
+        };
+
+        fn wide(s: &str) -> Vec<u16> {
+            s.encode_utf16().chain(std::iter::once(0)).collect()
+        }
+
+        struct TestState {
+            key: HKEY,
+            name: String,
+            variable: String,
+            previous: Option<OsString>,
+        }
+        impl Drop for TestState {
+            fn drop(&mut self) {
+                // SAFETY: the key belongs to this test; Windows environment mutation is safe.
+                unsafe {
+                    RegCloseKey(self.key);
+                    RegDeleteTreeW(HKEY_CURRENT_USER, wide(&self.name).as_ptr());
+                    match &self.previous {
+                        Some(value) => std::env::set_var(&self.variable, value),
+                        None => std::env::remove_var(&self.variable),
+                    }
+                }
+            }
+        }
+        let name = format!("Software\\ai.l0ng.openidea.registry-read-test-{}", std::process::id());
+        let variable = format!("OPENIDEA_TEST_REGVAR_{}", std::process::id());
+        let long = "x".repeat(600);
+        let mut key: HKEY = std::ptr::null_mut();
+        let opened = unsafe {
+            RegCreateKeyExW(
+                HKEY_CURRENT_USER,
+                wide(&name).as_ptr(),
+                0,
+                std::ptr::null(),
+                0,
+                KEY_READ | KEY_WRITE,
+                std::ptr::null(),
+                &mut key,
+                std::ptr::null_mut(),
+            )
+        };
+        assert_eq!(opened, ERROR_SUCCESS, "the test key must be creatable under HKCU\\Software");
+        let _cleanup = TestState { key, name, previous: std::env::var_os(&variable), variable: variable.clone() };
+        // SAFETY: Windows permits environment mutation. No other test uses this process-specific name.
+        unsafe { std::env::set_var(&variable, &long) };
+        let set = |value: &str, kind: u32, data: &[u16]| {
+            // SAFETY: `key` is open for writing and `data` holds `data.len() * 2` bytes.
+            assert_eq!(
+                unsafe {
+                    RegSetValueExW(
+                        key,
+                        wide(value).as_ptr(),
+                        0,
+                        kind,
+                        data.as_ptr() as *const _,
+                        (data.len() * 2) as u32,
+                    )
+                },
+                ERROR_SUCCESS
+            );
+        };
+        let read = |value: &str| read_registry_string(key, &wide(value));
+        set("Plain", REG_SZ, &wide("plain"));
+        set("Expand", REG_EXPAND_SZ, &wide(&format!("%{variable}%\\bin")));
+        set("ExpandLong", REG_EXPAND_SZ, &wide(&format!("%{variable}%")));
+        // Reproduce the reported interior terminator followed by a stale UTF-16 unit.
+        set("TrailingGarbage", REG_SZ, &wide("C:\\Zed\\bin\0n"));
+        set("ExpandTrailingGarbage", REG_EXPAND_SZ, &wide(&format!("%{variable}%\\bin\0n")));
+        set("Empty", REG_SZ, &wide(""));
+        let plain = read("Plain");
+        let expand = read("Expand");
+        let expand_long = read("ExpandLong");
+        assert_eq!(plain.as_deref(), Some("plain"));
+        assert_eq!(expand.as_deref(), Some(format!("{long}\\bin")).as_deref());
+        // Longer than its unexpanded form: needs the re-read at the expanded size.
+        assert_eq!(expand_long.as_deref(), Some(long.as_str()));
+        assert_eq!(read("TrailingGarbage").as_deref(), Some("C:\\Zed\\bin"));
+        assert_eq!(read("ExpandTrailingGarbage").as_deref(), Some(format!("{long}\\bin")).as_deref());
+        assert_eq!(read("Empty").as_deref(), Some(""));
+        assert_eq!(read("Missing"), None);
+    }
+
     #[test]
     fn append_path_entries_keeps_the_order_and_adds_what_is_missing() {
         let join = |dirs: &[&str]| std::env::join_paths(dirs).unwrap();

src/features/settings/__tests__/inspections.test.ts

@@ -45,6 +45,7 @@ describe("inspections tree", () => {
 
   it("groups servers by language with seen and configured codes, built-ins by category", () => {
     expect(tree.map((g) => g.name)).toEqual([
+      "Cargo",
       "General",
       "JavaScript",
       "Other",