Repository: clap-rs/clap (language: rust)

Issue:
Soundness: clap_lex makes incorrect assumption about OsStr
https://github.com/clap-rs/clap/blob/48d28aa689bfd0fb44ec025244b30ba261e2515a/clap_lex/src/ext.rs#L248-L256

It's not true that `OsStr` is _stably_ a transparent wrapper around `[u8]`, this code is technically unsound. It's fine as long as the Rust stdlib avoids making this change.

Fortunately the soon-to-be-stabilized [`as_os_str_bytes()`](https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.as_os_str_bytes) will fix this. Opening this issue to track it.


Discussion / hints:
While I generally feel I'm not creative enough to play these games, when I analyzed this, I could not find a way that the language or standard library can change that would violate this assumption because of the roundtripping involved with the conversions: "ref to DST" (`&[u8]`) -> "ref to DST" (`&str`) -> "ref to DST" (`&OsStr`) -> "ref to DST" (`&str`) -> "ref to DST" (`&[u8]`).  Is there something I'm missing?

I guess if the language allowed refs to DSTs to choose their representation (`(ptr, ptr)` vs `(ptr, length)`) and they chose different representations, then this could happen but (again, maybe not being creative enough) I cannot think of a situation this would actually happen.

As you said, `as_os_str_bytes` will soon be in our MSRV and we can switch to it.  I'm more concerned about the perception of risk from people seeing this issue if the perception of risk doesn't match the reality of the risk.
> Is there something I'm missing?

Yes, that it's still not a documented invariant; I'm in general not comfortable relying on those in unsafe code and usually note them when performing unsafe review. I mostly filed this issue so I have something to point to in the [published unsafe audit](https://github.com/google/rust-crate-audits/blob/7345370de0faff4a4812ad02653ffd3b6f31a9cf/audits.toml#L961-L970), it's not a huge deal and this will be a non-problem in the future anyway.

DSTs changing their representation is indeed a real worry. In general it's not a huge one: so much code relies on DST repr that it's unlikely to change without major warning, but that still does make it important to track. Fortunately in this case it will be fixed before any such change can come down the pipeline.

Fix patch:
diff --git a/clap_lex/src/ext.rs b/clap_lex/src/ext.rs
index 009c03c5105..729ba7e1f07 100644
--- a/clap_lex/src/ext.rs
+++ b/clap_lex/src/ext.rs
@@ -183,7 +183,7 @@ pub trait OsStrExt: private::Sealed {
 
 impl OsStrExt for OsStr {
     fn try_str(&self) -> Result<&str, std::str::Utf8Error> {
-        let bytes = to_bytes(self);
+        let bytes = self.as_encoded_bytes();
         std::str::from_utf8(bytes)
     }
 
@@ -192,22 +192,22 @@ impl OsStrExt for OsStr {
     }
 
     fn find(&self, needle: &str) -> Option<usize> {
-        let bytes = to_bytes(self);
+        let bytes = self.as_encoded_bytes();
         (0..=self.len().checked_sub(needle.len())?)
             .find(|&x| bytes[x..].starts_with(needle.as_bytes()))
     }
 
     fn strip_prefix(&self, prefix: &str) -> Option<&OsStr> {
-        let bytes = to_bytes(self);
+        let bytes = self.as_encoded_bytes();
         bytes.strip_prefix(prefix.as_bytes()).map(|s| {
             // SAFETY:
-            // - This came from `to_bytes`
-            // - Since `prefix` is `&str`, any split will be along UTF-8 boundarie
-            unsafe { to_os_str_unchecked(s) }
+            // - This came from `as_encoded_bytes`
+            // - Since `prefix` is `&str`, any split will be along UTF-8 boundary
+            unsafe { OsStr::from_encoded_bytes_unchecked(s) }
         })
     }
     fn starts_with(&self, prefix: &str) -> bool {
-        let bytes = to_bytes(self);
+        let bytes = self.as_encoded_bytes();
         bytes.starts_with(prefix.as_bytes())
     }
 
@@ -222,13 +222,18 @@ impl OsStrExt for OsStr {
     fn split_once(&self, needle: &'_ str) -> Option<(&OsStr, &OsStr)> {
         let start = self.find(needle)?;
         let end = start + needle.len();
-        let haystack = to_bytes(self);
+        let haystack = self.as_encoded_bytes();
         let first = &haystack[0..start];
         let second = &haystack[end..];
         // SAFETY:
-        // - This came from `to_bytes`
-        // - Since `needle` is `&str`, any split will be along UTF-8 boundarie
-        unsafe { Some((to_os_str_unchecked(first), to_os_str_unchecked(second))) }
+        // - This came from `as_encoded_bytes`
+        // - Since `needle` is `&str`, any split will be along UTF-8 boundary
+        unsafe {
+            Some((
+                OsStr::from_encoded_bytes_unchecked(first),
+                OsStr::from_encoded_bytes_unchecked(second),
+            ))
+        }
     }
 }
 
@@ -238,45 +243,6 @@ mod private {
     impl Sealed for std::ffi::OsStr {}
 }
 
-/// Allow access to raw bytes
-///
-/// As the non-UTF8 encoding is not defined, the bytes only make sense when compared with
-/// 7-bit ASCII or `&str`
-///
-/// # Compatibility
-///
-/// There is no guarantee how non-UTF8 bytes will be encoded, even within versions of this crate
-/// (since its dependent on rustc)
-fn to_bytes(s: &OsStr) -> &[u8] {
-    // SAFETY:
-    // - Lifetimes are the same
-    // - Types are compatible (`OsStr` is effectively a transparent wrapper for `[u8]`)
-    // - The primary contract is that the encoding for invalid surrogate code points is not
-    //   guaranteed which isn't a problem here
-    //
-    // There is a proposal to support this natively (https://github.com/rust-lang/rust/pull/95290)
-    // but its in limbo
-    unsafe { std::mem::transmute(s) }
-}
-
-/// Restore raw bytes as `OsStr`
-///
-/// # Safety
-///
-/// - `&[u8]` must either by a `&str` or originated with `to_bytes` within the same binary
-/// - Any splits of the original `&[u8]` must be done along UTF-8 boundaries
-unsafe fn to_os_str_unchecked(s: &[u8]) -> &OsStr {
-    // SAFETY:
-    // - Lifetimes are the same
-    // - Types are compatible (`OsStr` is effectively a transparent wrapper for `[u8]`)
-    // - The primary contract is that the encoding for invalid surrogate code points is not
-    //   guaranteed which isn't a problem here
-    //
-    // There is a proposal to support this natively (https://github.com/rust-lang/rust/pull/95290)
-    // but its in limbo
-    std::mem::transmute(s)
-}
-
 pub struct Split<'s, 'n> {
     haystack: Option<&'s OsStr>,
     needle: &'n str,
@@ -309,7 +275,10 @@ impl<'s, 'n> Iterator for Split<'s, 'n> {
 ///
 /// `index` must be at a valid UTF-8 boundary
 pub(crate) unsafe fn split_at(os: &OsStr, index: usize) -> (&OsStr, &OsStr) {
-    let bytes = to_bytes(os);
+    let bytes = os.as_encoded_bytes();
     let (first, second) = bytes.split_at(index);
-    (to_os_str_unchecked(first), to_os_str_unchecked(second))
+    (
+        OsStr::from_encoded_bytes_unchecked(first),
+        OsStr::from_encoded_bytes_unchecked(second),
+    )
 }


Test patch: