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

Fixing Potential Double Free Issue #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kuzeyardabulut
Copy link

@kuzeyardabulut kuzeyardabulut commented Aug 2, 2023

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

jsonpath/src/ffi/mod.rs

Lines 13 to 18 in 1a84c5a

fn to_char_ptr(v: &str) -> *const c_char {
let s = CString::new(v).unwrap_or_else(|_| panic!("invalid string: {}", v));
let ptr = s.as_ptr();
std::mem::forget(s);
ptr
}

If a panic happens within CString::new(), the std::mem::forget(s) will create a double free vulnerability.

jsonpath/src/ffi/mod.rs

Lines 48 to 61 in 1a84c5a

#[allow(deprecated)]
let node = unsafe { Box::from_raw(path_ptr as *mut parser::Node) };
let json_str = to_str(json_ptr, INVALID_JSON);
let json = serde_json::from_str(json_str)
.unwrap_or_else(|_| panic!("invalid json string: {}", json_str));
#[allow(deprecated)]
let mut selector = select::Selector::default();
let found = selector.compiled_path(&node).value(&json).select().unwrap();
std::mem::forget(node);
let result = serde_json::to_string(&found)
.unwrap_or_else(|_| panic!("json serialize error: {:?}", found));
to_char_ptr(result.as_str())

If a panic!() occurs between the Box::from_raw function, including the Box::from_raw 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.

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.

1 participant