From f4da4ab4cc956610d0dc9c21da619cbeef79ae95 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Sun, 12 Nov 2023 01:54:16 +0000 Subject: [PATCH] src: add IsolateScopes before using isolates 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: https://github.com/nodejs/build/issues/3204#issuecomment-1790213488 Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292 PR-URL: https://github.com/nodejs/node/pull/50680 Refs: https://github.com/v8/v8/commit/475c8cdf9a951bb06da3084794a0f659f8ef36c2 Reviewed-By: Ben Noordhuis Reviewed-By: Darshan Sen Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung Reviewed-By: James M Snell --- src/api/environment.cc | 7 +++++++ src/env.cc | 3 +++ src/json_parser.cc | 3 +++ src/json_parser.h | 2 +- src/node_sea.cc | 2 ++ src/util.cc | 9 +++++++-- src/util.h | 22 ++++++++++++++++++---- 7 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 6a6164b6d29443..a37a1b01fffb2a 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -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); } @@ -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. @@ -428,6 +433,8 @@ Environment* CreateEnvironment( ThreadId thread_id, std::unique_ptr inspector_parent_handle) { Isolate* isolate = isolate_data->isolate(); + + Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); const bool use_snapshot = context.IsEmpty(); diff --git a/src/env.cc b/src/env.cc index 4bfdfae2354741..a429d5526d0af6 100644 --- a/src/env.cc +++ b/src/env.cc @@ -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)) { @@ -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) \ diff --git a/src/json_parser.cc b/src/json_parser.cc index 7b405cfc3b331e..1b445193bc8ceb 100644 --- a/src/json_parser.cc +++ b/src/json_parser.cc @@ -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::New(isolate); @@ -45,6 +46,7 @@ bool JSONParser::Parse(const std::string& content) { std::optional JSONParser::GetTopLevelStringField( std::string_view field) { Isolate* isolate = isolate_.get(); + v8::Isolate::Scope isolate_scope(isolate); v8::HandleScope handle_scope(isolate); Local context = context_.Get(isolate); @@ -70,6 +72,7 @@ std::optional JSONParser::GetTopLevelStringField( std::optional JSONParser::GetTopLevelBoolField(std::string_view field) { Isolate* isolate = isolate_.get(); + v8::Isolate::Scope isolate_scope(isolate); v8::HandleScope handle_scope(isolate); Local context = context_.Get(isolate); diff --git a/src/json_parser.h b/src/json_parser.h index a4c5b3e96dd23d..9e6c00ef8d8712 100644 --- a/src/json_parser.h +++ b/src/json_parser.h @@ -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 context_; v8::Global content_; diff --git a/src/node_sea.cc b/src/node_sea.cc index 521f2f670b28c8..d1ab5051032d76 100644 --- a/src/node_sea.cc +++ b/src/node_sea.cc @@ -414,7 +414,9 @@ std::optional 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::New(isolate); Context::Scope context_scope(context); diff --git a/src/util.cc b/src/util.cc index 19fb91c959a205..38b1f9be72747e 100644 --- a/src/util.cc +++ b/src/util.cc @@ -678,7 +678,7 @@ Local 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_); @@ -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 diff --git a/src/util.h b/src/util.h index 5340182423681e..28873dbe4024df 100644 --- a/src/util.h +++ b/src/util.h @@ -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_; } @@ -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