Panic safety issue in SliceDeque::drain_filter

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

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.

	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
	}