search

udoprog / checkers

A sanity checker for global allocations in Rust
Apache License 2.0
48 stars 2 forks source link

Use of uninitialized memory in realloc #9

Closed saethlin closed 2 years ago

saethlin commented 2 years ago

This crate tries to hash the contents of a previous allocation when it is passed to Allocator::realloc. But it's perfectly valid for the previous allocation to not be completely initialized, and cause a read of uninitialized memory inside this allocator. One can observe this by running MIRIFLAGS="-Zmiri-check-number-validity" cargo +nightly miri test on this crate.

I'm not entirely sure what this check is for, but I think the only valid thing to do here is to remove it because inside an allocator there's no way to know how much of an allocation was initialized :cry:

udoprog commented 2 years ago

So I believe the UB is documented in the README under the Safety and Features sections. I'm also not sure how to implement this check (in Rust) without also invoking UB. I'm including it currently to get a feel for how useful it is and whether more effort should be expended to keep it in the future.

I'm not entirely sure what this check is for,

The check is to ensure that the GlobalAlloc::realloc impl is correct:

The new memory block is allocated with layout, but with the size updated to new_size. This new layout should be used when deallocating the new memory block with dealloc. The range 0..min(layout.size(), new_size) of the new memory block is guaranteed to have the same values as the original block.

So if someone can figure out if it's possible to hash a region of memory in Rust which might be uninitialized I'd be happy to hear it!

saethlin commented 2 years ago

I appreciate the commits that avoid tripping the UB in the tests. I suppose that's an improvement.

To be honest, I didn't look to see if this was already documented. I've been finding a lot of things of this sort, so I was in the wrong frame of mind to say "I wonder if the maintainers already know about this". Sorry about the churn.

udoprog commented 2 years ago

No worries!

Your comment made me re-examine the problem. Sadly there still doesn't seem to be a great solution. But thank you for the interest in making this project better!

saethlin commented 2 years ago

FYI I think it is likely this will never be valid in Rust. It isn't valid in C or C++, and in general it sounds like Rust is heading towards having more UB than those languages, or at least being honest about what you can and can't define in a consistent compiler.

But also, if you want to open an issue on https://github.com/rust-lang/unsafe-code-guidelines/ it might not hurt to at least see what people think about this use, other than "that's currently UB".