Skip to content

Commit

Permalink
Explicit panic instead of silent memory corruption
Browse files Browse the repository at this point in the history
Due to the automatic entry and exit behavior of Isolate upon creation and drop, it is crucial to ensure that v8::OwnedIsolate instances are dropped in the reverse order of their creation. Dropping them in the incorrect order can result in the corruption of the thread-local stack managed by v8, leading to memory corruption and potential segfaults. This introduces a check to verify the `this == Isolate::GetCurrent()` requirement before invoking the exit function. If the requirement is not met, a clean panic is triggered to provide explicit error handling instead of allowing silent memory corruption.
  • Loading branch information
guillaumebort committed Dec 12, 2023
1 parent 811cce2 commit 53b56bf
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 1 deletion.
2 changes: 2 additions & 0 deletions src/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ void v8__Isolate__Enter(v8::Isolate* isolate) { isolate->Enter(); }

void v8__Isolate__Exit(v8::Isolate* isolate) { isolate->Exit(); }

v8::Isolate* v8__Isolate__GetCurrent() { return v8::Isolate::GetCurrent(); }

void v8__Isolate__MemoryPressureNotification(v8::Isolate* isolate,
v8::MemoryPressureLevel level) {
isolate->MemoryPressureNotification(level);
Expand Down
9 changes: 8 additions & 1 deletion src/isolate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ extern "C" {
fn v8__Isolate__GetNumberOfDataSlots(this: *const Isolate) -> u32;
fn v8__Isolate__Enter(this: *mut Isolate);
fn v8__Isolate__Exit(this: *mut Isolate);
fn v8__Isolate__GetCurrent() -> *mut Isolate;
fn v8__Isolate__MemoryPressureNotification(this: *mut Isolate, level: u8);
fn v8__Isolate__ClearKeptObjects(isolate: *mut Isolate);
fn v8__Isolate__LowMemoryNotification(isolate: *mut Isolate);
Expand Down Expand Up @@ -534,7 +535,8 @@ extern "C" {
/// synchronize.
///
/// rusty_v8 note: Unlike in the C++ API, the Isolate is entered when it is
/// constructed and exited when dropped.
/// constructed and exited when dropped. Because of that v8::OwnedIsolate
/// instances must be dropped in the reverse order of creation
#[repr(C)]
#[derive(Debug)]
pub struct Isolate(Opaque);
Expand Down Expand Up @@ -1518,6 +1520,11 @@ impl Drop for OwnedIsolate {
snapshot_creator.is_none(),
"If isolate was created using v8::Isolate::snapshot_creator, you should use v8::OwnedIsolate::create_blob before dropping an isolate."
);
// Safety: We need to check `this == Isolate::GetCurrent()` before calling exit()
assert!(
self.cxx_isolate.as_mut() as *mut Isolate == v8__Isolate__GetCurrent(),
"v8::OwnedIsolate instances must be dropped in the reverse order of creation. They are entered upon creation and exited upon being dropped."
);
self.exit();
self.cxx_isolate.as_mut().clear_scope_and_annex();
self.cxx_isolate.as_mut().dispose();
Expand Down
11 changes: 11 additions & 0 deletions tests/test_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,17 @@ fn microtasks() {
}
}

#[test]
#[should_panic(
expected = "v8::OwnedIsolate instances must be dropped in the reverse order of creation. They are entered upon creation and exited upon being dropped."
)]
fn isolate_drop_order() {
let isolate1 = v8::Isolate::new(Default::default());
let isolate2 = v8::Isolate::new(Default::default());
drop(isolate1);
drop(isolate2);
}

#[test]
fn get_isolate_from_handle() {
extern "C" {
Expand Down

0 comments on commit 53b56bf

Please sign in to comment.