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(neon): Tag Root with an instance id and verify before using #847

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Jan 26, 2022

Overview

Root are Neon's mechanism for holding persistent references to values on the JavaScript heap. They are both Send and Sync to allow holding references across Rust threads. However, the current implementation has a soundness hole.

Resolves #843

Problem

When using Node worker threads, there may be multiple JavaScript runtimes present in the process. As shown in #843, it is possible to use a Root with a Context from the wrong runtime and cause a crash/undefined behavior.

Solution

This PR resolves the issue by panicking when attempting to dereference from the wrong module instance. In the future, we may want to add try_ variants to the Root methods to allow error handling without a panic. However, in most cases, this will be a logic error that applications cannot recover.

Approach

On Node-API 6+, we use instance data to store a unique identifier initialized from a global counter. Each Root stores the identifier for comparing later when being used.

On previous versions, the ThreadId is used. This may not be as robust and is sensitive to V8 implementation details.

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

This is so straightforward and non-clever (in a good way). LGTM!

@kjvalencik kjvalencik merged commit 6ece4cd into next/0.10 Feb 25, 2022
@kjvalencik kjvalencik deleted the kv/root-tagging branch February 25, 2022 18:38
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.

2 participants