Skip to content

IndexMap::retain: Fix unsoundness in the context of panicking predicate - #689

Merged
sgued merged 2 commits into
rust-embedded:mainfrom
sgued:indexmap
Sep 15, 2026
Merged

sgued merged 2 commits into
rust-embedded:mainfrom
sgued:indexmap

Conversation

@sgued

@sgued sgued commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Closes #688

@sgued sgued changed the title IndexMap::retain: Fixed unsoundness in the context of panicking predicate IndexMap::retain: Fix unsoundness in the context of panicking predicate Sep 14, 2026
@sgued

sgued commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Looks like miri doesn't like the fix by itself. I'll take a deeper look later.

@sgued

sgued commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

The CI was broken with miri because Vec::retain didn't have tests and thus was silently broken with miri. This PR will also close #690

@sgued
sgued force-pushed the indexmap branch 2 times, most recently from 6115b08 to 07c8972 Compare September 14, 2026 20:22
@sgued
sgued requested a review from zeenix September 14, 2026 20:29

@zeenix zeenix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM otherwise.

Comment thread src/index_map.rs Outdated
.entries
.retain_mut(|entry| keep(&mut entry.key, &mut entry.value));

self.reinsert_all();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should now be removed? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, removed it and force-pushed.

Miri didn't like the two borrowing made of the `v` field since the second
mutable borrow would invalidate any borrow and thus the pointers from the first
non mutable borrow.

This can be fixed by just borrowing once mutably and then only manipulating
pointers.

Closes rust-embedded#690
@sgued
sgued added this pull request to the merge queue Sep 15, 2026
Merged via the queue into rust-embedded:main with commit a50891c Sep 15, 2026
21 checks passed
@sgued
sgued deleted the indexmap branch September 15, 2026 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IndexMap::retain is not exception safe, leading to read of uninit values

2 participants