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

fix(napi): potential double free issue #1679

Merged
merged 2 commits into from Nov 2, 2023

Conversation

kuzeyardabulut
Copy link
Contributor

Hi,
I found a memory-safety/soundness issue in this crate while scanning Rust code for potential vulnerabilities. This PR contains a fix for the issue.

Issue Description

let mut temp = unsafe { Vec::from_raw_parts(inner, length, length) };
temp.push(item);
// Inner Vec has been reallocated, so we need to update the pointer
if temp.as_mut_ptr() != inner {
self.inner.store(temp.as_mut_ptr(), Ordering::Relaxed);
}
std::mem::forget(temp);

let mut temp = unsafe { Vec::from_raw_parts(inner, length, length) };
f(temp.as_mut_slice());
// Inner Vec has been reallocated, so we need to update the pointer
if temp.as_mut_ptr() != inner {
self.inner.store(temp.as_mut_ptr(), Ordering::Relaxed);
}
self.length.store(temp.len(), Ordering::Relaxed);
std::mem::forget(temp);

If a panic!() occurs between the Vec::from_raw_parts function, including the Vec::from_raw_parts function itself, and std::mem::forget, a double free vulnerability emerges.

Fix

In Rust, std::mem::forget does not actually free the memory, instead it simply allows the memory to leak. This can lead to double free when the data object goes out of scope and its destructor is called automatically. The modification here uses std::mem::ManuallyDrop to wrap data. This ensures that data will not be automatically dropped when it goes out of scope, thus avoiding a double free scenario. With ManuallyDrop, we explicitly state that the data variable should not be dropped, thus avoiding any potential double free issues.

@Brooooooklyn Brooooooklyn changed the title Fixing Potential Double Free Issue fix(napi): potential double free issue Nov 2, 2023
@Brooooooklyn Brooooooklyn merged commit 3a1a280 into napi-rs:main Nov 2, 2023
38 of 42 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants