Fix potential UB in RawTableInner::fallible_with_capacity

[N1ark]

This is a fix for a potential UB in the library, that occurs anytime a HashTable is created.

The code is the following:

hashbrown/src/raw.rs

Lines 1647 to 1657 in d84a552

	let buckets = capacity_to_buckets(capacity, table_layout)
	.ok_or_else(|| fallibility.capacity_overflow())?;
	let mut result =
	Self::new_uninitialized(alloc, table_layout, buckets, fallibility)?;
	// SAFETY: We checked that the table is allocated and therefore the table already has
	// `self.bucket_mask + 1 + Group::WIDTH` number of control bytes (see TableLayout::calculate_layout_for)
	// so writing `self.num_ctrl_bytes() == bucket_mask + 1 + Group::WIDTH` bytes is safe.
	result.ctrl_slice().fill_empty();
	Ok(result)

• on line 1651, a new table is allocated; it's content is thus uninitialised
• line 1655 calls ctrl_slice, which before this PR returned the slice of control tags, as &mut [Tag]. However, these control bytes are uninitialised.

This can be reproduced without additional tests, running the command below. The repr uses -Zmiri-recursive-validation, an experimental flag that may be removed in the future.

$ MIRIFLAGS="-Zmiri-recursive-validation" cargo +nightly miri test --no-fail-fast
test result: FAILED. 72 passed; 203 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.51s

Importantly, this has not been decided to be undefined behaviour yet; there is ongoing discussion on the topic, see rust-lang/unsafe-code-guidelines#346. However it seems to me that it is at least unwise to pass around references to uninit values, in particular when this slice is only used for writing anyways, and the issue can be trivially avoided.

This fix thus replaces the &mut [Tag] with &mut [MaybeUninit<Tag>], ensuring that the possibility of the slice's content being uninitialised is made explicit.

Regardless of the decision on whether this should be UB or not, the code as it stands violates the safety condition of ctrl_slice, which states "We've initialized all control bytes", since as shown above the bytes are not initialised in fallible_with_capacity.

hashbrown/src/raw.rs

Lines 2646 to 2649 in d84a552

	fn ctrl_slice(&mut self) -> &mut [Tag] {
	// SAFETY: We've initialized all control bytes, and have the correct number.
	unsafe { slice::from_raw_parts_mut(self.ctrl.as_ptr().cast(), self.num_ctrl_bytes()) }
	}

[clarfonthey]

Good catch. While this isn't currently treated as UB by miri, it's definitely not good to have and worth fixing.

[Amanieu]

Thanks for catching this!

Looking at the code, the only thing we ever do with ctrl_slice is to call fill_empty on it. @clarfonthey (since you last touched this code) would it make sense to just have a single method which clears the control slice? Or are there other plans for using ctrl_slice?

[clarfonthey]

The plan was to eventually make ctrl_slice be the API that replaces the ctrl method which just returns a pointer to a single element, but that's going to require a lot of other changes first. I think this is a reasonable change for now, though.

I think that the main issue was assuming that the tags would be always initialized, which technically holds for every case but the beginning. So, maybe in this case it would be good to just have a fill method here; I'm indifferent.

[N1ark]

I think keeping ctrl_slice as an interface is fine, if when removing ctrl we realise we need Tags instead of MaybeUninit<Tag>s, an unsafe ctrl_slice_init method can be added that assumes they're initialised

[clarfonthey]

Either way, the API here is going to get changed in refactoring once we've decided what we want to do there, so, I'm fine just making a minimal change to remove UB in the meantime.

[RalfJung]

FWIW this signature allows safe callers to de-initialize the tags. That's likely to lead to UB later. So technically the function should probably be unsafe now with a safety requirement of "don't de-initialize things". Not sure how strict hashbrown is about unsafety hygiene for private functions.

[clarfonthey]

We should be more strict about it, but at least for now, all of these APIs are being reworked. Goal is mostly to make the HashTable type be the actual implementation for HashMap and HashSet with no separate raw table API (thus, everything would need to be properly marked) but it's a slow process.