Undefined behaviour in slice::fill specialization.

[m-ou-se]

This was reported by @the8472 on Zulip: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/potential.20UB.20in.20slice.3A.3Afill/near/248871405

I'm wondering if the optimization here is correct:

rust/library/core/src/slice/specialize.rs

Lines 22 to 28 in ae90dcf

	if size_of::<T>() == 1 {
	// SAFETY: The size_of check above ensures that values are 1 byte wide, as required
	// for the transmute and write_bytes
	unsafe {
	let value: u8 = transmute_copy(&value);
	write_bytes(self.as_mut_ptr(), value, self.len());
	}

Specifically if T is MaybeUninit<u8>. It momentarily transmutes that to an u8 before passing it to write_bytes.

Demo:

use std::mem::MaybeUninit;

fn main() {
    let mut a = [MaybeUninit::<u8>::uninit(); 10];
    a.fill(MaybeUninit::uninit());
}

error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
    --> /home/mara/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/intrinsics.rs:2191:14
     |
2191 |     unsafe { write_bytes(dst, val, count) }
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

[Aaron1011]

I think the fix should be as simple as using MaybeUninit<u8> instead of u8 as the type of value - that will work regardless of whether or not the memory is initialized.

[m-ou-se]

The other option is just removing the if size_of::<T>() == 1 case entirely. Llvm is smart enough to turn that loop in the else branch into a memset.

[the8472]

It does help the gcc codgen backend at least as it can't manage to produce a memset for u64 so I assume it would fail to do the same for u8 if it weren't for that specialization.

https://rust.godbolt.org/z/hrMYvWz9n

[m-ou-se]

@the8472: It also fails for the u8 case: https://rust.godbolt.org/z/Wjx5Mnj5K

[the8472]

It does call memset if you don't pass an array. https://rust.godbolt.org/z/x4KT99heT

[RalfJung]

I'd think the write_bytes intrinsic could easily have its type changed to take a MaybeUninit<u8>, which would let one solve this problem. The stable function of course cannot be changed, though...

[RalfJung]

Reopening to track adding a test. (That's the standard procedure for this, isn't it?)

[atsuzaki]

@RalfJung I would like to try working on adding the test, though this will be my first contribution so I'd need some guidance. I'm assuming this would be a miri test as it's related to correctness--may I DM you on Zulip? (or would someone else be more suitable?)

[RalfJung]

@atsuzaki great. :)

Miri runs the libcore test suite via https://github.com/rust-lang/miri-test-libstd. So adding it as a test in libcore makes most sense, IMO.