Repository: GuillaumeGomez/sysinfo (language: rust)

Issue:
the process API on macOS can returned non UTF-8 String
On some macOS computers, we observed invalid Strings returned by sysinfo through the use of the process API, notably `process.cmd()`. Those Strings are not utf-8, and cause issues down the line when other code uses those invalid strings.

From what I can find, the macos implementation of the process API uses the `from_utf8_unchecked` function in several places, including when building the cmd value: https://github.com/GuillaumeGomez/sysinfo/blob/0.30/src/unix/apple/macos/process.rs#L606

This bug seems to have been fixed on master as a side-effect of https://github.com/GuillaumeGomez/sysinfo/commit/5f7a3972114b00919ecd730f1295218daae01289, since the value is now saved as an OsString, and the onus is on the caller to check or not whether this OsString is valid utf-8.

However, the master branch is unreleased, and this bug is still present in the 0.30 branch, so a fix in 0.30 would be welcomed. I think replacing the calls to `from_utf8_unchecked` into calls to `from_utf8_lossy` would be good enough while we wait for the next major release. If that sounds ok to you, I can do a PR with those changes


Discussion / hints:
Fixing this bug requires an API change so it cannot be released as 0.30.x. Since this is already solved, closing this issue.
Do you have an estimation as to when the next major version would be released?
> Fixing this bug requires an API change so it cannot be released as 0.30.x. Since this is already solved, closing this issue.

Couldn't this at least be made sound by replacing `from_utf8_unchecked` with `from_utf8_lossy`, without an API change? Sure, it'll return "wrong" results by replacing invalid utf8 characters with the replacement char, but that seems better than the current Undefined Behavior.
> Do you have an estimation as to when the next major version would be released?

When it's ready.

> Couldn't this at least be made sound by replacing `from_utf8_unchecked` with `from_utf8_lossy`, without an API change? Sure, it'll return "wrong" results by replacing invalid utf8 characters with the replacement char, but that seems better than the current Undefined Behavior.

Sounds good to me. Want to send a PR?
I'm a bit confused cause that's exactly what I suggested in the description of the issue :smile: I'll send a PR
Oh sorry. I'm in holiday, still early morning. :sweat_smile:

Fix patch:
diff --git a/src/unix/apple/macos/process.rs b/src/unix/apple/macos/process.rs
index 3d2580f0f..c01950d69 100644
--- a/src/unix/apple/macos/process.rs
+++ b/src/unix/apple/macos/process.rs
@@ -2,7 +2,9 @@
 
 #![allow(clippy::assigning_clones)]
 
+use std::ffi::OsStr;
 use std::mem::{self, MaybeUninit};
+use std::os::unix::ffi::OsStrExt;
 use std::path::{Path, PathBuf};
 
 use libc::{c_int, c_void, kill};
@@ -407,7 +409,7 @@ unsafe fn get_exe_and_name_backup(
     ) {
         x if x > 0 => {
             buffer.set_len(x as _);
-            let tmp = String::from_utf8_unchecked(buffer);
+            let tmp = String::from_utf8_lossy(&buffer).to_string();
             let exe = PathBuf::from(tmp);
             if process.name.is_empty() {
                 process.name = exe
@@ -575,12 +577,10 @@ unsafe fn get_process_infos(process: &mut ProcessInner, refresh_kind: ProcessRef
 
 fn get_exe(data: &[u8]) -> (PathBuf, &[u8]) {
     let pos = data.iter().position(|c| *c == 0).unwrap_or(data.len());
-    unsafe {
-        (
-            Path::new(std::str::from_utf8_unchecked(&data[..pos])).to_path_buf(),
-            &data[pos..],
-        )
-    }
+    (
+        Path::new(OsStr::from_bytes(&data[..pos])).to_path_buf(),
+        &data[pos..],
+    )
 }
 
 fn get_arguments<'a>(
@@ -600,21 +600,19 @@ fn get_arguments<'a>(
         data = &data[1..];
     }
 
-    unsafe {
-        while n_args > 0 && !data.is_empty() {
-            let pos = data.iter().position(|c| *c == 0).unwrap_or(data.len());
-            let arg = std::str::from_utf8_unchecked(&data[..pos]);
-            if !arg.is_empty() && refresh_cmd {
-                cmd.push(arg.to_string());
-            }
-            data = &data[pos..];
-            while data.first() == Some(&0) {
-                data = &data[1..];
-            }
-            n_args -= 1;
+    while n_args > 0 && !data.is_empty() {
+        let pos = data.iter().position(|c| *c == 0).unwrap_or(data.len());
+        let arg = String::from_utf8_lossy(&data[..pos]);
+        if !arg.is_empty() && refresh_cmd {
+            cmd.push(arg.to_string());
         }
-        data
+        data = &data[pos..];
+        while data.first() == Some(&0) {
+            data = &data[1..];
+        }
+        n_args -= 1;
     }
+    data
 }
 
 fn get_environ(environ: &mut Vec<String>, mut data: &[u8]) {
@@ -624,18 +622,16 @@ fn get_environ(environ: &mut Vec<String>, mut data: &[u8]) {
         data = &data[1..];
     }
 
-    unsafe {
-        while !data.is_empty() {
-            let pos = data.iter().position(|c| *c == 0).unwrap_or(data.len());
-            let arg = std::str::from_utf8_unchecked(&data[..pos]);
-            if arg.is_empty() {
-                return;
-            }
-            environ.push(arg.to_string());
-            data = &data[pos..];
-            while data.first() == Some(&0) {
-                data = &data[1..];
-            }
+    while !data.is_empty() {
+        let pos = data.iter().position(|c| *c == 0).unwrap_or(data.len());
+        let arg = String::from_utf8_lossy(&data[..pos]);
+        if arg.is_empty() {
+            return;
+        }
+        environ.push(arg.to_string());
+        data = &data[pos..];
+        while data.first() == Some(&0) {
+            data = &data[1..];
         }
     }
 }


Test patch: