ibabushkin / arenavec

MIT License
2 stars 1 forks source link

panic safety bug may happen in 3 functions #1

Open JOE1994 opened 3 years ago

JOE1994 commented 3 years ago

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

Issue Description

[derive(Clone)]

struct Foo(usize, Option); impl Drop for Foo { fn drop(&mut self) { println!("Dropping {:?}", self.0); if self.0 == 1 && ATOMIC_TRUE.compare_and_swap(true, false, SeqCst) { println!("THIS WILL PANIC {:?}", self.1.as_ref().unwrap()); } } }

static ATOMIC_TRUE: AtomicBool = AtomicBool::new(true); const DEFAULT_CAPACITY: usize = 4096 << 8; fn main() { let arena = Arena::init_capacity(ArenaBacking::SystemAllocation, DEFAULT_CAPACITY).unwrap();

let mut vec: SliceVec<Foo> = SliceVec::new(arena.inner());
vec.push(Foo(0, Some(12)));
vec.push(Foo(1, None));
assert_eq!(vec.len(), 2);

vec.resize(1, Foo(99, Some(78)));

}


#### Program Output
The message `Dropping 1` is printed twice, indicating the same object was dropped twice.

Dropping 1 thread 'main' panicked at 'called Option::unwrap() on a None value', examples/arenavec.rs:14:62 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace Dropping 99 Dropping 0 Dropping 1



## Suggested Fix
* `common::Slice::<T, H>::new`:
  Move `res.len = len;` to after all writes are done.
* `common::SliceVec::<T, H>::resize_with` & `common::SliceVec::<T, H>::resize`:
  Move `self.slice.len = len;` to before `drop_in_place()`.

Thank you for checking out this issue!
Shnatsel commented 3 years ago

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.