Fix incorrect length if a hasher panics during rehash#710
Fix incorrect length if a hasher panics during rehash#710Amanieu merged 2 commits intorust-lang:masterfrom
Conversation
|
Just to double-check: this is only a length mismatch, right, and not the potential to drop stuff twice? Good find either way. |
|
This bug only happens if the The problem is that |
|
Ah, yeah, that makes sense. I mostly only ask to see if we could add some test that would be able to detect that under miri if the code regressed, and I'm not sure the existing one does. Although, I guess that it doesn't really matter if they don't need to be dropped, since it would probably be fine to read them more than necessary. And the control is still limited by the mask, which from my understanding, would still be correct here. |
|
Miri seems to be hanging on this test in CI... |
|
Was thinking, doesn't the miri run usually take a while? And no, latest commit took 1min30s: https://github.com/rust-lang/hashbrown/actions/runs/23953355374/job/69865909833 |
| for i in 0..111 { | ||
| hashmap_reserve_survives_panicking_build_hasher_inner(i); | ||
| } |
There was a problem hiding this comment.
On my system, each iteration of this takes about 15s in miri, which tracks with CI's final completion time around 30min. Maybe it would suffice to try only specific first, middle, last indices, i.e. [0, 55, 110].
Update hashbrown to 0.17 The main benefit of this update is to include one potential UB fix and one bug; relevant changelog entries: * Fixed potential UB in `RawTableInner::fallible_with_capacity` (rust-lang/hashbrown#692) * Fixed incorrect length if a hasher panics during rehash (rust-lang/hashbrown#710) cc @Amanieu Also cc @RalfJung who had also noticed the UB issue with `-Zmiri-recursive-validation`.
3 participants
If the user-provided hasher panics during an in-place rehash and the key/value types do not need to be dropped (according to
core::mem::needs_drop) then the length field of the hash map may not be updated correctly. The map will appear to have more elements than it actually does, which will cause iterators (which blindly trust the length) to read past the end of the map, potentially reading uninitialized memory.This issue was discovered by @lopopolo and @codex and the reproducer that they produced has been adapted an included in this PR.