avoid creating Boxes of uninitalized values in RawVec

[matklad]

RawVec<bool>::into_box is definitely instant UB, if not all values are initialized.

See https://gankro.github.io/blah/initialize-me-maybe/

[rust-highfive]

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

[matklad]

r? @RalfJung

[matklad]

Note: I haven't actually audited the uses of this method, but it seems unlikely that they aren't fine, b/c we run tests under miri.

[RalfJung]

Note: I haven't actually audited the uses of this method, but it seems unlikely that they aren't fine, b/c we run tests under miri.

I am not sure how great our test coverage is here.^^ Also, "invalid data in a Box" is (like "invalid data behind a reference") an open question, and Miri currently does not check validity behind references and boxes. That would be extremely expensive.

So, it might be worth mentioning that the UB this talks about is a still-under-discussion point, similar to what I did in MaybeUninit:

(Notice that the rules around uninitialized integers are not finalized yet, but until they are, it is advisable to avoid them.)

And it's probably worth auditing the uses. There are not many (at least on in liballoc+libstd).

[matklad]

And it's probably worth auditing the uses.

Heh, you are 100% right. Here's at least one case where we do copy_from_slice for an slice of uninit values:

rust/src/liballoc/boxed.rs

Lines 549 to 550 in be10e62

	let mut boxed = unsafe { RawVec::with_capacity(slice.len()).into_box() };
	boxed.copy_from_slice(slice);

.

Should that code be (optimistically) rewritten to use RawVec::ptr and ptr::copy_nonoverlapping, or should we wait until we figure out if materializing an &mut[undef] is ok?

[RalfJung]

I'd prefer rewriting it -- also to see what the ergonomic impact is.

[matklad]

That usage turned out the singe one where we expose uninit memory

Things I've looked at

10:45:55|~/projects/rust/tt-parser/src/liballoc|ub-comment✓
λ rg into_box\( -F
vec.rs
638:            buf.into_box()

raw_vec.rs
696:    pub unsafe fn into_box(self) -> Box<[T]> {

boxed.rs
402:            from_boxed_utf8_unchecked(buf.into_box())
553:            buf.into_box()
876:        return unsafe { new.into_box() };
885:            unsafe fn into_box(self) -> Box<[T]> {
888:                raw.into_box()

11:03:17|~/projects/rust/tt-parser/src/liballoc|ub-comment⚡*
λ cd ../libstd/

11:05:06|~/projects/rust/tt-parser/src/libstd|ub-comment⚡*
λ rg into_box\( -F
ffi/os_str.rs
344:        let rw = Box::into_raw(self.inner.into_box()) as *mut OsStr;
663:        let rw = Box::into_raw(s.inner.into_box()) as *mut OsStr;

sys_common/os_str_bytes.rs
117:    pub fn into_box(self) -> Box<Slice> {
160:    pub fn into_box(&self) -> Box<Slice> {

sys_common/wtf8.rs
358:    pub fn into_box(self) -> Box<Wtf8> {
640:    pub fn into_box(&self) -> Box<Wtf8> {

sys/windows/os_str.rs
112:    pub fn into_box(self) -> Box<Slice> {
113:        unsafe { mem::transmute(self.inner.into_box()) }
153:    pub fn into_box(&self) -> Box<Slice> {
154:        unsafe { mem::transmute(self.inner.into_box()) }

I've rewritten the docs to just make "elements must be initialized" the invariant of the method itself, with a copy-pasted note that the language-level rule is under construction.

Don't know if the patch works: compiling LLVM 😭

[matklad]

tests pass locally

[RalfJung]

But where does this even allocate and copy now...?

[matklad]

In the From<[u8]> for Box<[u8]> impl, which is also adjusted in this PR

[matklad]

To clarify, the impl I am talking about is the generic one over T

[RalfJung]

Could you call Box<_>::from(...) instead of .into() to make that clearer? And maybe move that out of the unsafe block to make it clear that that is not a weird thing that "steals" self or so.

[matklad]

good call, fixed

[RalfJung]

Thanks!

@bors r+

[bors]

📌 Commit fe31ad3 has been approved by RalfJung