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

Explicit panic instead of silent memory corruption #1377

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

guillaumebort
Copy link
Contributor

@guillaumebort guillaumebort commented Dec 12, 2023

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.

The following program demonstrates how a segfault can be currently triggered using only the safe Rust API:

fn main() {
    let platform = v8::new_default_platform(0, false).make_shared();
    v8::V8::initialize_platform(platform);
    v8::V8::initialize();

    let mut isolates = vec![];

    loop {
        isolates.push(v8::Isolate::new(Default::default()));

        // maybe drop a random isolate
        if rand::random() {
            drop(isolates.remove(rand::random::<usize>() % isolates.len()));
        }
    }
}

An alternative approach would be to strictly disallow the creation of multiple v8::OwnedIsolate instances on the same thread to ensure immediate failure. However, some tests (and probably some applications) currently rely on this behavior.

Another option is to remove the automatic enter/exit functionality and instead introduce a safe API that explicitly requires entering and exiting the Isolate. However, implementing this change would result in a significant break in the API.

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.
@CLAassistant
Copy link

CLAassistant commented Dec 12, 2023

CLA assistant check
All committers have signed the CLA.

@bartlomieju bartlomieju requested a review from mmastrac December 12, 2023 11:21
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mmastrac mmastrac merged commit 60e0859 into denoland:main Dec 12, 2023
7 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.

3 participants