Fix VecDeque::shrink_to UB when handle_alloc_error unwinds. - #123803
Conversation
commented
Apr 11, 2024
|
rustbot has assigned @Mark-Simulacrum. Use |
commented
Apr 13, 2024
|
@bors r+ |
commented
Apr 13, 2024
commented
Apr 14, 2024
commented
Apr 15, 2024
|
Woops, didn't mean to comment here from that account lol. |
|
|
||
| // preserve the old error hook just in case. | ||
| let old_error_hook = take_alloc_error_hook(); | ||
| set_alloc_error_hook(|_| panic!("alloc error")); |
There was a problem hiding this comment.
This changes the alloc error hook for all concurrently running tests... is that really a good idea?
There was a problem hiding this comment.
Hm, is there another mechanism to allow what I want to test there? Something like compiling this specific test with -Zoom=unwind maybe?
There was a problem hiding this comment.
You can make it a separate test crate by adding a file like alloc/tests/vec_deque_alloc_error.rs and add it to alloc/Cargo.toml separately.
commented
Apr 16, 2024
Yeah disabling the test in Miri is fine for now, but please ping |
Luckily it's comparatively simple to just restore the `VecDeque` into a valid state on unwinds.
This way, no other test can be tripped up by `test_shrink_to_unwind` changing the alloc error hook.
fb00ac6 to
5cb53bc
Compare
commented
May 25, 2024
|
@rustbot ready |
commented
May 25, 2024
|
@bors r+ |
Fixes #123369
For
VecDequeit's relatively simple to restore the buffer into a consistent state so this PR does just that.Note that with its current implementation,
shrink_tomay change the internal arrangement of elements in the buffer, so e.g.[D, <uninit>, A, B, C]will become[<uninit>, A, B, C, D]and[<uninit>, <uninit>, A, B, C]may become[B, C, <uninit>, <uninit>, A]ifshrink_tounwinds. This shouldn't be an issue though as we don't make any guarantees about the stability of the internal buffer arrangement (and this case is impossible to hit on stable anyways).This PR also includes a test with code adapted from #123369 which fails without the new
shrink_tocode. Does this suffice or do we maybe need more exhaustive tests like in #108475?cc @Amanieu
@rustbot label +T-libs