Fix unwind vulnerability#660
Conversation
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
Please also add an explanation to the PR description (and commit message too, if we don't do a squash merge).
sgued
force-pushed
the
fix-unwind-vuln
branch
3 times, most recently
from
23a1499 to
45bb608
Compare
It is now, but anyways, the release tag already existed, and re-tagging is a very bad practice. |
| let len = self.vec.len; | ||
| self.vec.len = LenT::ZERO; | ||
| unsafe { | ||
| // Drop all the elements that have not been moved out of vec | ||
| ptr::drop_in_place(&mut self.vec.as_mut_slice()[self.next.into_usize()..]); | ||
| // Prevent dropping of other elements | ||
| self.vec.len = LenT::ZERO; | ||
| let remaining = slice::from_raw_parts_mut( | ||
| self.vec.as_mut_ptr().add(self.next.into_usize()), | ||
| (len - self.next).into_usize(), | ||
| ); | ||
| ptr::drop_in_place(remaining); | ||
| } |
There was a problem hiding this comment.
Isn't this basically the same logic as VecInner::truncate? Does VecInner::truncate have the same vulnerability?
There was a problem hiding this comment.
Ah, VecInner::truncate sets self.len before ptr::drop_in_place, so no, but would be good to clean up the unsafe block there as well to be consistent with this one, and add the same test for it.
There was a problem hiding this comment.
It's not the same as truncate because we don't drop the beginning of the vec if it has already been iterated over.
| fn drop(&mut self) { | ||
| let len = self.vec.len; | ||
| self.vec.len = LenT::ZERO; | ||
| unsafe { |
There was a problem hiding this comment.
| unsafe { | |
| // SAFETY: `self.vec` contains initialized elements in the range `self.next..len` | |
| // which have not been consumed yet, so it is safe to create | |
| // a mutable slice to them and drop them. | |
| // | |
| // `self.vec.len` is set to `0` before dropping the elements so that | |
| // a panicking `Drop` implementation does not result in inconsistent | |
| // state during unwinding that could lead to undefined behaviour. | |
| unsafe { |
There was a problem hiding this comment.
Good idea, I added a safety comment.
|
Added the same test for |
reitermarkus
force-pushed
the
fix-unwind-vuln
branch
from
c4c1bff to
1b1cacb
Compare
| ## [v0.9.3] 2025-04-15 | ||
|
|
||
| - Fixed unsoundness in `Deque::clear`, `HistoryBuf::clear` and `IndexMap::clear` in the context | ||
| - Fixed unsoundness in `Deque::clear`, `HistoryBuf::clear`, `IndexMap::clear`, `Vec::IntoIter::drop` and `HistoryBuf::write` in the context |
There was a problem hiding this comment.
| - Fixed unsoundness in `Deque::clear`, `HistoryBuf::clear`, `IndexMap::clear`, `Vec::IntoIter::drop` and `HistoryBuf::write` in the context | |
| ## [Unreleased] | |
| - Fixed unsoundness in `Vec::IntoIter::drop` and `HistoryBuf::write` in the context of panicking drop implementations. | |
| ## [v0.9.3] 2025-04-15 | |
| - Fixed unsoundness in `Deque::clear`, `HistoryBuf::clear` and `IndexMap::clear` in the context | |
| - Fixed unsoundness in `Deque::clear`, `HistoryBuf::clear` and `IndexMap::clear` in the context |
reitermarkus
force-pushed
the
fix-unwind-vuln
branch
2 times, most recently
from
1b2ac67 to
d1f3f26
Compare
…ntation Prior to this, a panic in a downstream drop implementation could lead to an inconsistent state during unwinding that could lead to undefined behaviour. For more info see rust-embedded#659
…implementation Prior to this, a panic in a downstream drop implementation could lead to an inconsistent state during unwinding that could lead to undefined behaviour. For more info see rust-embedded#659
4 participants
Fixes #659
Prior to this, a panic in a downstream drop implementation could lead to an inconsistent state during unwinding that could lead to undefined behaviour.
0.9.3 has not been released to crates.io for some reason, so I suggest we add #652 and this PR to it.