fix: lossless OsStr/Path round-trip on unix (sound on all platforms)#13
Open
ofek-sha wants to merge 2 commits into
Open
fix: lossless OsStr/Path round-trip on unix (sound on all platforms)#13ofek-sha wants to merge 2 commits into
ofek-sha wants to merge 2 commits into
Conversation
`BorrowedInterned::as_os_str()`/`as_path()` reconstructed the value via bstr's `to_os_str_lossy()`/`to_path_lossy()`. On non-unix platforms (e.g. Windows) that path falls back to a UTF-8-lossy conversion, so non-UTF-8 OS strings/paths came back corrupted with U+FFFD replacement characters — even though the store side (`Interned`'s `From<&OsStr>` / `From<&Path>` impls) preserves the exact `OsStr::as_encoded_bytes()`. Reconstruct losslessly via `OsStr::from_encoded_bytes_unchecked` on the stored bytes, with a SAFETY comment documenting that those bytes came from `as_encoded_bytes()` on the same platform/build (the function's documented precondition). Return types stay `Cow` (now always `Cow::Borrowed`) so the public API is unchanged. Kept bstr-gated. Adds a unix non-UTF-8 round-trip test asserting the raw bytes survive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous commit reconstructed the value via `OsStr::from_encoded_bytes_unchecked(self.deref())`. That is unsound: `as_os_str()` is public and callable on any `Interned`, including one created from arbitrary bytes (e.g. `Interned::new(b"\xff\xff")`). On Windows `from_encoded_bytes_unchecked` requires valid WTF-8; arbitrary bytes are not, so the call is UB. There is no safe lossless reconstruction from arbitrary bytes in std on Windows. Fix without any `unsafe`, cfg-gated: - unix: `OsStrExt::from_bytes(self.deref())` — a unix `OsStr` is just bytes, so this is lossless AND safe; returns `Cow::Borrowed`. - non-unix (Windows/WASI): keep the existing safe lossy path (`as_bstr().to_os_str_lossy()`). - `as_path()` is now implemented in terms of `as_os_str()` (Borrowed -> `Path::new`, Owned -> `PathBuf::from`) so the two stay consistent. Both remain bstr-gated and keep their `Cow` return types. Document on both methods that the conversion is lossless + zero-copy on unix and best-effort lossy on non-unix, with a note that a lossless-on-Windows path could be added later as an explicit `unsafe` opt-in whose caller guarantees `OsStr` provenance. The `os_str_non_utf8_round_trip` test already carries `#[cfg(unix)]` and stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BorrowedInterned::as_os_str()/as_path()reconstructed viabstr's lossy conversion, which can replace bytes with U+FFFD — corrupting non-UTF-8 OS paths that were interned intact.OsStrExt::from_bytes(Cow::Borrowed).OsStrfrom arbitrary bytes there — the only lossless option (from_encoded_bytes_unchecked) isunsafeand would be unsound here, because anInternedmay hold arbitrary bytes (Interned::new(b"\xff\xff")) that aren't valid WTF-8 → UB. Documented, with a note that a lossless-on-Windows path could be a future explicitunsafeopt-in.as_path()delegates toas_os_str(); both staybstr-gated withCowreturns. Nounsafecode.Review gap finding #9. An earlier attempt used
from_encoded_bytes_uncheckedunconditionally (UB on Windows) — this version is sound.cargo test/--all-features/--features bstrgreen; non-UTF-8 unix round-trip test added; clippy/fmt clean.🤖 Generated with Claude Code