Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update unsound DrainFilter and RString::retain #44

Closed
Qwaz opened this issue Dec 21, 2020 · 3 comments
Closed

Update unsound DrainFilter and RString::retain #44

Qwaz opened this issue Dec 21, 2020 · 3 comments

Comments

@Qwaz
Copy link

Qwaz commented Dec 21, 2020

Hello, we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

impl<T, F> Iterator for DrainFilter<'_, T, F>
where F: FnMut(&mut T) -> bool,
{
type Item = T;
fn next(&mut self) -> Option<T> {
unsafe {
while self.idx != self.old_len {
let i = self.idx;
self.idx += 1;
let v = slice::from_raw_parts_mut(self.vec.as_mut_ptr(), self.old_len);
if (self.pred)(&mut v[i]) {
self.del += 1;
return Some(ptr::read(&v[i]));
} else if self.del > 0 {
let del = self.del;
let src: *const T = &v[i];
let dst: *mut T = &mut v[i - del];
ptr::copy_nonoverlapping(src, dst, 1);
}
}
None
}
}
fn size_hint(&self) -> (usize, Option<usize>) {
(0, Some(self.old_len - self.idx))
}
}

#[inline]
pub fn retain<F>(&mut self, mut pred: F)
where F: FnMut(char) -> bool
{
// literal copy-paste of std,so if this is wrong std is wrong.
let len = self.len();
let mut del_bytes = 0;
let mut idx = 0;
while idx < len {
let ch = unsafe {
self.get_unchecked(idx..len).chars().next().unwrap()
};
let ch_len = ch.len_utf8();
if !pred(ch) {
del_bytes += ch_len;
} else if del_bytes > 0 {
unsafe {
ptr::copy(
self.inner.as_ptr().add(idx),
self.inner.as_mut_ptr().add(idx - del_bytes),
ch_len
);
}
}
idx += ch_len;
}
if del_bytes > 0 {
unsafe { self.inner.set_len(len - del_bytes); }
}
}

These two implementations are copy-pasted from Rust's standard library, and unfortunately it turns out that std implementations were containing soundness bugs (rust-lang/rust#60977 and rust-lang/rust#78498, respectively). Could you check them and update the respective part of this crate?

rodrimati1992 added a commit that referenced this issue Dec 21, 2020
These methods copied their implementation from the standard library,
which had memory safety bugs discovered in
rust-lang/rust#60977 and rust-lang/rust#78498 .

This bug was reported in #44 .

Added adapted tests from std which test these bugs.
@rodrimati1992
Copy link
Owner

That you for reporting this, I'll be making the 0.9.1 patch in the 0_9_patch branch. I'll wait at least 18 hours before releasing the patch.

@Qwaz
Copy link
Author

Qwaz commented Dec 21, 2020

Thank you for the quick fix!

rodrimati1992 added a commit that referenced this issue Dec 22, 2020
* Fixed String::retain, RVec::retain. Bumped patch version to 0.9.1 .

These methods copied their implementation from the standard library,
which had memory safety bugs discovered in
rust-lang/rust#60977 and rust-lang/rust#78498 .

This bug was reported in #44 .

Added adapted tests from std which test these bugs.

* Updated changelog for patch
@rodrimati1992
Copy link
Owner

I've released the patch for this, so I'm closing this now.
https://github.com/rodrimati1992/abi_stable_crates/releases/tag/0.9.1
https://crates.io/crates/abi_stable/0.9.1

rodrimati1992 added a commit that referenced this issue Nov 22, 2022
* Fixed String::retain, RVec::retain. Bumped patch version to 0.9.1 .

These methods copied their implementation from the standard library,
which had memory safety bugs discovered in
rust-lang/rust#60977 and rust-lang/rust#78498 .

This bug was reported in #44 .

Added adapted tests from std which test these bugs.

* Updated changelog for patch
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

No branches or pull requests

2 participants