Skip to content

PathBuf incorrectly transmutes OsString to Vec<u8> #124409

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
RalfJung opened this issue Apr 26, 2024 · 0 comments · Fixed by #124410
Closed

PathBuf incorrectly transmutes OsString to Vec<u8> #124409

RalfJung opened this issue Apr 26, 2024 · 0 comments · Fixed by #124410
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Apr 26, 2024

PathBuf contains this gem:

rust/library/std/src/path.rs

Lines 1172 to 1175 in f56afa0

#[inline]
fn as_mut_vec(&mut self) -> &mut Vec<u8> {
unsafe { &mut *(self as *mut PathBuf as *mut Vec<u8>) }
}

This effectively transmutes an OsString to Vec<u8>. But on Windows, OsString is Wtf8Buf:

/// An owned, growable string of well-formed WTF-8 data.
///
/// Similar to `String`, but can additionally contain surrogate code points
/// if they’re not in a surrogate pair.
#[derive(Eq, PartialEq, Ord, PartialOrd, Clone)]
pub struct Wtf8Buf {
bytes: Vec<u8>,
/// Do we know that `bytes` holds a valid UTF-8 encoding? We can easily
/// know this if we're constructed from a `String` or `&str`.
///
/// It is possible for `bytes` to have valid UTF-8 without this being
/// set, such as when we're concatenating `&Wtf8`'s and surrogates become
/// paired, as we don't bother to rescan the entire string.
is_known_utf8: bool,
}

This is not repr(transparent), so there is no guarantee that we can just transmute this. And I think with layout randomization this can actually fail -- that's probably what happened here.

Is there a reason why this uses transmutes rather than some sort of private accessor that exposes a *mut Vec<u8> through all these layers?

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 26, 2024
bors added a commit to rust-lang/miri that referenced this issue Apr 26, 2024
add smoke tests for basic PathBuf interactions

I wrote these while debugging [this](https://github.com/rust-lang/miri-test-libstd/actions/runs/8849912635/job/24302962983); it turns out the issue is [more complicated](rust-lang/rust#124409) but these tests still seemed worth keeping.
@Noratrieb Noratrieb added C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 26, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 26, 2024
…strieb

PathBuf: replace transmuting by accessor functions

The existing `repr(transparent)` was anyway insufficient as `OsString` was not `repr(transparent)`. And furthermore, on Windows it was blatantly wrong as `OsString` wraps `Wtf8Buf` which is a `repr(Rust)` type with 2 fields:

https://github.com/rust-lang/rust/blob/51a7396ad3d78d9326ee1537b9ff29ab3919556f/library/std/src/sys_common/wtf8.rs#L131-L146

So let's just be honest about what happens and add accessor methods that make this abstraction-breaking act of PathBuf visible on the APIs that it pierces through.

Fixes rust-lang#124409
@bors bors closed this as completed in 7cbba53 Apr 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 27, 2024
Rollup merge of rust-lang#124410 - RalfJung:path-buf-transmute, r=Nilstrieb

PathBuf: replace transmuting by accessor functions

The existing `repr(transparent)` was anyway insufficient as `OsString` was not `repr(transparent)`. And furthermore, on Windows it was blatantly wrong as `OsString` wraps `Wtf8Buf` which is a `repr(Rust)` type with 2 fields:

https://github.com/rust-lang/rust/blob/51a7396ad3d78d9326ee1537b9ff29ab3919556f/library/std/src/sys_common/wtf8.rs#L131-L146

So let's just be honest about what happens and add accessor methods that make this abstraction-breaking act of PathBuf visible on the APIs that it pierces through.

Fixes rust-lang#124409
RalfJung pushed a commit to RalfJung/rust that referenced this issue Apr 27, 2024
add smoke tests for basic PathBuf interactions

I wrote these while debugging [this](https://github.com/rust-lang/miri-test-libstd/actions/runs/8849912635/job/24302962983); it turns out the issue is [more complicated](rust-lang#124409) but these tests still seemed worth keeping.
RalfJung pushed a commit to RalfJung/rust that referenced this issue Apr 27, 2024
add smoke tests for basic PathBuf interactions

I wrote these while debugging [this](https://github.com/rust-lang/miri-test-libstd/actions/runs/8849912635/job/24302962983); it turns out the issue is [more complicated](rust-lang#124409) but these tests still seemed worth keeping.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants