Skip to content

Fix VecDeque::shrink_to UB when handle_alloc_error unwinds. - #123803

Merged
bors merged 2 commits into
rust-lang:masterfrom
Sp00ph:shrink_to_fix
May 26, 2024
Merged

Fix VecDeque::shrink_to UB when handle_alloc_error unwinds.#123803
bors merged 2 commits into
rust-lang:masterfrom
Sp00ph:shrink_to_fix

Conversation

@Sp00ph

@Sp00ph Sp00ph commented Apr 11, 2024

Copy link
Copy Markdown
Member

Fixes #123369

For VecDeque it's relatively simple to restore the buffer into a consistent state so this PR does just that.

Note that with its current implementation, shrink_to may 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] if shrink_to unwinds. 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_to code. Does this suffice or do we maybe need more exhaustive tests like in #108475?

cc @Amanieu

@rustbot label +T-libs

@rustbot

ghost commented Apr 11, 2024

Copy link
Copy Markdown
Collaborator

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 11, 2024
@Mark-Simulacrum

ghost commented Apr 13, 2024

Copy link
Copy Markdown
Member

@bors r+

@bors

ghost commented Apr 13, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit fb00ac6 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2024
@matthiaskrgr

ghost commented Apr 14, 2024

Copy link
Copy Markdown
Member

@bors r- #123910 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 14, 2024
@Sp00ph

ghost commented Apr 15, 2024

Copy link
Copy Markdown
Member Author

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"));

ghost Apr 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the alloc error hook for all concurrently running tests... is that really a good idea?

ghost Apr 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, is there another mechanism to allow what I want to test there? Something like compiling this specific test with -Zoom=unwind maybe?

ghost Apr 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RalfJung

ghost commented Apr 16, 2024

Copy link
Copy Markdown
Member

Hmm, so is this just blocked on rust-lang/miri#3439 then? Or should I just slap a #[cfg(not(miri))] on the test for now?

Yeah disabling the test in Miri is fine for now, but please ping @rust-lang/miri when you do that so we can keep tabs of these tests and re-enable them when the underlying issue is fixed.

Markus Everling added 2 commits May 7, 2024 19:30
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.
@Sp00ph

ghost commented May 25, 2024

Copy link
Copy Markdown
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2024
@Mark-Simulacrum

ghost commented May 25, 2024

Copy link
Copy Markdown
Member

@bors r+

@bors

ghost commented May 25, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 5cb53bc has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2024
@bors
bors merged commit 7fb8122 into rust-lang:master May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VecDeque::shrink_to leads to UB if handle_alloc_error unwinds.

6 participants