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.

Sign up for GitHub

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

Jump to bottom

Panic safety issue in SliceDeque::drain_filter #90

Open
ammaraskar opened this issue Feb 19, 2021 · 1 comment · May be fixed by #91
Open

Panic safety issue in SliceDeque::drain_filter #90

ammaraskar opened this issue Feb 19, 2021 · 1 comment · May be fixed by #91

Comments

@ammaraskar
Copy link

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed a panic safety issue in the DrainFilter returned by the SliceDeque::drain_filter function:

slice_deque/src/lib.rs

Lines 3017 to 3039 in 045fb28

unsafe {
while self.idx != self.old_len {
let i = self.idx;
self.idx += 1;
let v = slice::from_raw_parts_mut(
self.deq.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];
// This is safe because self.deq has length 0
// thus its elements will not have Drop::drop
// called on them in the event of a panic.
ptr::copy_nonoverlapping(src, dst, 1);
}
}
None
}

Notably, the code increments self.idx before it calls self.pred which can potentially panic. This means for example, that it can leave the SliceDeque in an inconsistent state resulting in a double drop. Here is an example:

#![forbid(unsafe_code)]

use slice_deque::SliceDeque;

struct DropDetector(u32);

impl Drop for DropDetector {
    fn drop(&mut self) {
        println!("Dropping {}", self.0);
    }
}

fn main() {
    let mut deq = SliceDeque::new();
    deq.push_back(DropDetector(1));
    deq.push_back(DropDetector(2));
    deq.push_back(DropDetector(3));

    let drained = deq
        .drain_filter(|x| {
            if x.0 == 1 {
                true
            } else if x.0 == 2 {
                false
            } else {
                panic!("predicate panicked!");
            }
        })
        .collect::<SliceDeque<_>>();;
}

This outputs:

thread 'main' panicked at 'predicate panicked!', src/main.rs:40:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Dropping 1
Dropping 2
Dropping 2

Return code 101
@ammaraskar ammaraskar mentioned this issue Mar 29, 2021
Merged
@Shnatsel
Copy link

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

This was referenced Mar 30, 2021
Open
Open
LiquidityC referenced this issue in LiquidityC/slice_ring_buffer Mar 30, 2021
@LiquidityC
Update DrainFilter.idx after calling DrainFilter.pred
15567c9 
Removes security issue where a panic! from DrainFilter.pred
would leave DrainFilter.idx incremented and risking a double drop.

Fixes: #90
This was referenced Mar 31, 2021
Closed
Open
This was referenced Jun 13, 2021
Open
Merged
@github-actions github-actions bot mentioned this issue Nov 7, 2021
Open
@github-actions github-actions bot mentioned this issue Apr 15, 2022
Closed
@github-actions github-actions bot mentioned this issue Oct 8, 2022
Closed
@est31 est31 mentioned this issue Nov 26, 2022
Closed
@LiquidityC LiquidityC mentioned this issue Dec 27, 2022
Closed
@github-actions github-actions bot mentioned this issue Apr 4, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update DrainFilter.idx after calling DrainFilter.pred LiquidityC/slice_ring_buffer
2 participants
@Shnatsel @ammaraskar