PathBuf: replace transmuting by accessor functions

[RalfJung]

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:

rust/library/std/src/sys_common/wtf8.rs

Lines 131 to 146 in 51a7396

	/// 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,
	}

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 #124409

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[RalfJung]

Turns out when this transmute was originally added 9 years ago, Wtf8Buf was still defined as

pub struct Wtf8Buf {
    bytes: Vec<u8>
}

So, it wasn't repr(transparent), but it was just a newtype. At some point is_known_utf8 got added and then the PathBuf assumption was silently broken.

A good lesson in why we should not use transmutes to circumvent field privacy.

[RalfJung]

FWIW I did not review whether the things PathBuf does with direct access to the internals of Wtf8Buf are actually correct. The only mutation it does is truncate and the field that got added to Wtf8Buf, is_known_utf8, sounds like truncation would preserve its invariant if truncation is done at character boundaries, but I can't dig into this right now. This PR doesn't make things any worse, it just makes them more visible.

A better fix may be to add a private truncate method to OsString for use by PathBuf (and it needs a last() as well).

[Noratrieb]

lovely.. i wonder how many sussy transmutes there are in other places...
r=me

[RalfJung]

@bors r=Nilstrieb

[bors]

📌 Commit c47978a has been approved by Nilstrieb

It is now in the queue for this repository.

[RalfJung]

@bors rollup

[crlf0710]

@RalfJung @Nilstrieb If it's exposing mutable reference to the buffer, i think Wtf8Buf's is_known_utf8 field needs to be revalidated after certain modifications...

[RalfJung]

Yes, see what I wrote above: #124410 (comment)

Feel free to open an issue to track a proper cleanup here. I just have the time for a basic soundness fix right now, not the time to make it nice.

[workingjubilee]

Hmm, it would have been sufficient to set the field to false every time that as_mut_vec_for_path_buf was called (on the assumption that whatever happens next would violate that).