src: add IsolateScopes before using isolates

kvakil · RafaelGSS · commit faf6a04ba62b · 2023-11-29T09:46:33.000-03:00
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 <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git 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<InspectorParentHandle> 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
@@ -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
@@ -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);
@@ -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);
@@ -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);
diff --git 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<v8::Context> context_;
   v8::Global<v8::Object> content_;
diff --git a/src/node_sea.cc b/src/node_sea.cc
@@ -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);
 
diff --git a/src/util.cc b/src/util.cc
@@ -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_);
@@ -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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -678,7 +678,7 @@ Local<String> UnionBytes::ToStringChecked(Isolate* isolate) const {`
`678`	`678`	`}`
`679`	`679`	`}`
`680`	`680`
`681`		`-RAIIIsolate::RAIIIsolate(const SnapshotData* data)`
	`681`	`+RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data)`
`682`	`682`	`: allocator_{ArrayBuffer::Allocator::NewDefaultAllocator()} {`
`683`	`683`	`isolate_ = Isolate::Allocate();`
`684`	`684`	`CHECK_NOT_NULL(isolate_);`
`@@ -692,9 +692,14 @@ RAIIIsolate::RAIIIsolate(const SnapshotData* data)`
`692`	`692`	`Isolate::Initialize(isolate_, params);`
`693`	`693`	`}`
`694`	`694`
`695`		`-RAIIIsolate::~RAIIIsolate() {`
	`695`	`+RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() {`
`696`	`696`	`per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);`
`697`	`697`	`isolate_->Dispose();`
`698`	`698`	`}`
`699`	`699`
	`700`	`+RAIIIsolate::RAIIIsolate(const SnapshotData* data)`
	`701`	`+ : isolate_{data}, isolate_scope_{isolate_.get()} {}`
	`702`	`+`
	`703`	`+RAIIIsolate::~RAIIIsolate() {}`
	`704`	`+`
`700`	`705`	`} // namespace node`