Skip to content

Commit

Permalink
src: add IsolateScopes before using isolates
Browse files Browse the repository at this point in the history
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: #50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
kvakil authored and UlisesGascon committed Dec 11, 2023
1 parent 3eb9e4e commit f804b3a
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 7 deletions.
7 changes: 7 additions & 0 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@ void SetIsolateMiscHandlers(v8::Isolate* isolate, const IsolateSettings& s) {

void SetIsolateUpForNode(v8::Isolate* isolate,
const IsolateSettings& settings) {
Isolate::Scope isolate_scope(isolate);

SetIsolateErrorHandlers(isolate, settings);
SetIsolateMiscHandlers(isolate, settings);
}
Expand Down Expand Up @@ -354,6 +356,9 @@ Isolate* NewIsolate(Isolate::CreateParams* params,

SetIsolateCreateParamsForNode(params);
Isolate::Initialize(isolate, *params);

Isolate::Scope isolate_scope(isolate);

if (snapshot_data == nullptr) {
// If in deserialize mode, delay until after the deserialization is
// complete.
Expand Down Expand Up @@ -428,6 +433,8 @@ Environment* CreateEnvironment(
ThreadId thread_id,
std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
Isolate* isolate = isolate_data->isolate();

Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);

const bool use_snapshot = context.IsEmpty();
Expand Down
3 changes: 3 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,8 @@ IsolateDataSerializeInfo IsolateData::Serialize(SnapshotCreator* creator) {

void IsolateData::DeserializeProperties(const IsolateDataSerializeInfo* info) {
size_t i = 0;

v8::Isolate::Scope isolate_scope(isolate_);
HandleScope handle_scope(isolate_);

if (per_process::enabled_debug_list.enabled(DebugCategory::MKSNAPSHOT)) {
Expand Down Expand Up @@ -431,6 +433,7 @@ void IsolateData::CreateProperties() {
// One byte because our strings are ASCII and we can safely skip V8's UTF-8
// decoding step.

v8::Isolate::Scope isolate_scope(isolate_);
HandleScope handle_scope(isolate_);

#define V(PropertyName, StringValue) \
Expand Down
3 changes: 3 additions & 0 deletions src/json_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ bool JSONParser::Parse(const std::string& content) {
DCHECK(!parsed_);

Isolate* isolate = isolate_.get();
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);

Local<Context> context = Context::New(isolate);
Expand Down Expand Up @@ -45,6 +46,7 @@ bool JSONParser::Parse(const std::string& content) {
std::optional<std::string> JSONParser::GetTopLevelStringField(
std::string_view field) {
Isolate* isolate = isolate_.get();
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);

Local<Context> context = context_.Get(isolate);
Expand All @@ -70,6 +72,7 @@ std::optional<std::string> JSONParser::GetTopLevelStringField(

std::optional<bool> JSONParser::GetTopLevelBoolField(std::string_view field) {
Isolate* isolate = isolate_.get();
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);

Local<Context> context = context_.Get(isolate);
Expand Down
2 changes: 1 addition & 1 deletion src/json_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class JSONParser {
private:
// We might want a lighter-weight JSON parser for this use case. But for now
// using V8 is good enough.
RAIIIsolate isolate_;
RAIIIsolateWithoutEntering isolate_;

v8::Global<v8::Context> context_;
v8::Global<v8::Object> content_;
Expand Down
2 changes: 2 additions & 0 deletions src/node_sea.cc
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,9 @@ std::optional<std::string> GenerateCodeCache(std::string_view main_path,
RAIIIsolate raii_isolate(SnapshotBuilder::GetEmbeddedSnapshotData());
Isolate* isolate = raii_isolate.get();

v8::Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);

Local<Context> context = Context::New(isolate);
Context::Scope context_scope(context);

Expand Down
9 changes: 7 additions & 2 deletions src/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ Local<String> UnionBytes::ToStringChecked(Isolate* isolate) const {
}
}

RAIIIsolate::RAIIIsolate(const SnapshotData* data)
RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data)
: allocator_{ArrayBuffer::Allocator::NewDefaultAllocator()} {
isolate_ = Isolate::Allocate();
CHECK_NOT_NULL(isolate_);
Expand All @@ -692,9 +692,14 @@ RAIIIsolate::RAIIIsolate(const SnapshotData* data)
Isolate::Initialize(isolate_, params);
}

RAIIIsolate::~RAIIIsolate() {
RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() {
per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);
isolate_->Dispose();
}

RAIIIsolate::RAIIIsolate(const SnapshotData* data)
: isolate_{data}, isolate_scope_{isolate_.get()} {}

RAIIIsolate::~RAIIIsolate() {}

} // namespace node
22 changes: 18 additions & 4 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -969,11 +969,11 @@ void SetConstructorFunction(v8::Isolate* isolate,
SetConstructorFunctionFlag flag =
SetConstructorFunctionFlag::SET_CLASS_NAME);

// Simple RAII class to spin up a v8::Isolate instance.
class RAIIIsolate {
// Like RAIIIsolate, except doesn't enter the isolate while it's in scope.
class RAIIIsolateWithoutEntering {
public:
explicit RAIIIsolate(const SnapshotData* data = nullptr);
~RAIIIsolate();
explicit RAIIIsolateWithoutEntering(const SnapshotData* data = nullptr);
~RAIIIsolateWithoutEntering();

v8::Isolate* get() const { return isolate_; }

Expand All @@ -982,6 +982,20 @@ class RAIIIsolate {
v8::Isolate* isolate_;
};

// Simple RAII class to spin up a v8::Isolate instance and enter it
// immediately.
class RAIIIsolate {
public:
explicit RAIIIsolate(const SnapshotData* data = nullptr);
~RAIIIsolate();

v8::Isolate* get() const { return isolate_.get(); }

private:
RAIIIsolateWithoutEntering isolate_;
v8::Isolate::Scope isolate_scope_;
};

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down

0 comments on commit f804b3a

Please sign in to comment.