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 use after free of native function return value. #30

Merged
merged 1 commit into from
May 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/v8/isolate_scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ extern "C" fn free_external_data<T>(arg1: *mut ::std::os::raw::c_void) {
}

impl<'isolate> V8IsolateScope<'isolate> {
/// Create an isolate scope by performing the following:
/// 1. Enter the isolate
/// 2. Create a scope handler.
pub(crate) fn new(isolate: &'isolate V8Isolate) -> V8IsolateScope<'isolate> {
let inner_isolate_scope = unsafe { v8_IsolateEnter(isolate.inner_isolate) };
let inner_handlers_scope = unsafe { v8_NewHandlersScope(isolate.inner_isolate) };
Expand All @@ -52,6 +55,18 @@ impl<'isolate> V8IsolateScope<'isolate> {
}
}

/// Create a dummy isolate scope. This should be used only in case we know that
/// the isolate is already entered and we already have a scope handler. For example,
/// when calling a native function we can create a dummy isolate scope because we
/// know we already entered the isolate and created a scope handler.
pub(crate) fn new_dummy(isolate: &'isolate V8Isolate) -> V8IsolateScope<'isolate> {
V8IsolateScope {
isolate,
inner_handlers_scope: std::ptr::null_mut(),
inner_isolate_scope: std::ptr::null_mut(),
}
}

/// Creating a new context for JS code invocation.
#[must_use]
pub fn new_context(&self, globals: Option<&V8LocalObjectTemplate>) -> V8Context {
Expand Down Expand Up @@ -282,8 +297,12 @@ impl<'isolate> V8IsolateScope<'isolate> {
impl<'isolate> Drop for V8IsolateScope<'isolate> {
fn drop(&mut self) {
unsafe {
v8_FreeHandlersScope(self.inner_handlers_scope);
v8_IsolateExit(self.inner_isolate_scope);
if !self.inner_handlers_scope.is_null() {
v8_FreeHandlersScope(self.inner_handlers_scope);
}
if !self.inner_isolate_scope.is_null() {
v8_IsolateExit(self.inner_isolate_scope);
}
}
}
}
8 changes: 7 additions & 1 deletion src/v8/v8_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,13 @@ pub(crate) extern "C" fn load_module<
inner_isolate: unsafe { v8_ContextRefGetIsolate(v8_ctx_ref) },
no_release: true,
};
let isolate_scope = V8IsolateScope::new(&isolate);

// We know that if we reach here we already entered the isolate and have a handlers scope, so
// we can and must create a dummy isolate scope. If we create a regular isolate scope
// all the local handlers will be free when this isolate scope will be release including the
// return value.
// Users can use this isolate score as if it was a regular isolate scope.
let isolate_scope = V8IsolateScope::new_dummy(&isolate);
let ctx_scope = V8ContextScope {
inner_ctx_ref: v8_ctx_ref,
exit_on_drop: false,
Expand Down
7 changes: 6 additions & 1 deletion src/v8/v8_native_function_template.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ pub(crate) extern "C" fn native_basic_function<
no_release: true,
};

let isolate_scope = V8IsolateScope::new(&isolate);
// We know that if we reach here we already entered the isolate and have a handlers scope, so
// we can and must create a dummy isolate scope. If we create a regular isolate scope
// all the local handlers will be free when this isolate scope will be release including the
// return value.
// Users can use this isolate scope as if it was a regular isolate scope.
let isolate_scope = V8IsolateScope::new_dummy(&isolate);

let inner_ctx_ref = unsafe { v8_GetCurrentCtxRef(inner_isolate) };
let ctx_scope = V8ContextScope {
Expand Down