Reading uninitialized memory in Allocator::realloc is unsound

[HeroicKatora]

First the critical part: In Allocator::realloc, the crate fails to check if the delegate failed reallocation. This is signalled by a returned null pointer and means that the allocation was not reallocated. It is UB to create a slice from a null pointer in any case.

Then also the hash_ptr function wraps arbitrary uninitialized memory into a &[u8] slice. This is unsound and might return inconsistent results which would subvert the usefulness of the hash in the first place.

• checkers/src/allocator.rs

  Line 119 in 99e01e3

  let old_hash = crate::utils::hash_ptr(ptr, min_size);

• checkers/src/allocator.rs

  Line 126 in 99e01e3

  let is_relocated = Some(old_hash == crate::utils::hash_ptr(new_ptr, min_size));

But it also triggers UB when fxhash reads from any uninitialized part of the memory, potentially sooner.

Workarounds

Turn off the realloc feature.

[udoprog]

Nice catch! I initially had hash_ptr return an Option<bool> and did the null check internally, but forgot to migrate it. (Same issue for #2).

But it also triggers UB when fxhash reads from any uninitialized part of the memory, potentially sooner.

A properly behaving realloc implementation is supposed to migrate and initialize the common prefix, who's length is decided here by min_size. If the allocator leaves it uninitialized you are correct that this might or might not trigger. Hopefully (!) it will. I'll need to think more about the safety implications, but I'll get the null checks fixed ASAP!

[udoprog]

Hm. To my understanding the safety implications of possibly reading uninitialized memory due to a badly implemented would propagate to the underlying application as well, so us doing that for diagnostics purposes would be no better nor worse than having it move into the application. But I suspect we do have a chance of catching it which would be a net positive.

I will do the following:

• Add a notice to the README and lib.rs that the library should strictly be used for testing with the default set of feature flags, with the implication that it will produce UB if the underlying allocator is badly implemented (read uninitialized memory). With the hope being that we will catch it anyways.
• I'll add a feature flag zeroed, and also document in the README and lib.rs that if you want to guarantee that checkers doesn't produce UB, both realloc and zeroed need to be disabled.

In a future release, I'll put those checks behind an opt-in feature flag instead of having them on by default.

@HeroicKatora How does that sound to you?

[HeroicKatora]

Add a notice to the README and lib.rs that the library should strictly be used for testing with the default set of feature flags, with the implication that it will produce UB if the underlying allocator is badly implemented (read uninitialized memory).

That's not strictly true. Contrary to the zeroed method the memory passed into and from realloc need not be initialized. It is also within the discretion of the delegate if a successful reallocation initializes the reallocated memory even for a well-behaved implementation. For example, if realloc merely appends a few pages then any uninitialized portion of the memory of the original allocation remains uninitialized. And semantically, all uninitialized bytes of the allocation remain uninitialized in the new allocation even if implementation-defined ways were used to copy them over and especially if a library-implementation used ptr::copy for that task. It is however not yet determined if reading an uninitialized u8 is UB but it could be ruled that the arithmetic performed to compute the hash sum qualifies as such an invalid use (See rust-lang/unsafe-code-guidelines#71).

[udoprog]

True. Thanks for detailing the issue. I'll reopen until the notices have been amended. I'll make sure they are amended again depending on how rust-lang/unsafe-code-guidelines#71 pans out.

Thank you!

[udoprog]

I've changed the title since hash_ptr ultimately is unsafe, and should have a safety notice saying that the memory should be initialized. The problem is in realloc in my view.