Skip to content

Commit

Permalink
src: use effective cppgc wrapper id to deduce non-cppgc id
Browse files Browse the repository at this point in the history
Previously we hard-code a wrapper id to be used in BaseObjects
to avoid accidentally triggering cppgc on these non-cppgc-managed
objects, but hard-coding can be be hacky and result in mismatch
when we start to create CppHeap ourselves. This patch makes it
more robust by deducing non-cppgc id from the effective cppgc id,
if there is one.
  • Loading branch information
joyeecheung committed Jul 5, 2023
1 parent f4eaf1e commit 168c23a
Show file tree
Hide file tree
Showing 11 changed files with 233 additions and 29 deletions.
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -1047,6 +1047,7 @@
'test/cctest/test_aliased_buffer.cc',
'test/cctest/test_base64.cc',
'test/cctest/test_base_object_ptr.cc',
'test/cctest/test_cppgc.cc',
'test/cctest/test_node_postmortem_metadata.cc',
'test/cctest/test_environment.cc',
'test/cctest/test_linked_binding.cc',
Expand Down
23 changes: 14 additions & 9 deletions src/base_object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,23 +70,28 @@ Realm* BaseObject::realm() const {
return realm_;
}

bool BaseObject::IsBaseObject(v8::Local<v8::Object> obj) {
bool BaseObject::IsBaseObject(IsolateData* isolate_data,
v8::Local<v8::Object> obj) {
if (obj->InternalFieldCount() < BaseObject::kInternalFieldCount) {
return false;
}
void* ptr =
obj->GetAlignedPointerFromInternalField(BaseObject::kEmbedderType);
return ptr == &kNodeEmbedderId;

uint16_t* ptr = static_cast<uint16_t*>(
obj->GetAlignedPointerFromInternalField(BaseObject::kEmbedderType));
return ptr == isolate_data->embedder_id_for_non_cppgc();
}

void BaseObject::TagBaseObject(v8::Local<v8::Object> object) {
void BaseObject::TagBaseObject(IsolateData* isolate_data,
v8::Local<v8::Object> object) {
DCHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
object->SetAlignedPointerInInternalField(BaseObject::kEmbedderType,
&kNodeEmbedderId);
object->SetAlignedPointerInInternalField(
BaseObject::kEmbedderType, isolate_data->embedder_id_for_non_cppgc());
}

void BaseObject::SetInternalFields(v8::Local<v8::Object> object, void* slot) {
TagBaseObject(object);
void BaseObject::SetInternalFields(IsolateData* isolate_data,
v8::Local<v8::Object> object,
void* slot) {
TagBaseObject(isolate_data, object);
object->SetAlignedPointerInInternalField(BaseObject::kSlot, slot);
}

Expand Down
13 changes: 4 additions & 9 deletions src/base_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ BaseObject::BaseObject(Realm* realm, Local<Object> object)
: persistent_handle_(realm->isolate(), object), realm_(realm) {
CHECK_EQ(false, object.IsEmpty());
CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
SetInternalFields(object, static_cast<void*>(this));
SetInternalFields(realm->isolate_data(), object, static_cast<void*>(this));
realm->AddCleanupHook(DeleteMe, static_cast<void*>(this));
realm->modify_base_object_count(1);
}
Expand Down Expand Up @@ -66,18 +66,13 @@ void BaseObject::MakeWeak() {
WeakCallbackType::kParameter);
}

// This just has to be different from the Chromium ones:
// https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=18-23;drc=5a758a97032f0b656c3c36a3497560762495501a
// Otherwise, when Node is loaded in an isolate which uses cppgc, cppgc will
// misinterpret the data stored in the embedder fields and try to garbage
// collect them.
uint16_t kNodeEmbedderId = 0x90de;

void BaseObject::LazilyInitializedJSTemplateConstructor(
const FunctionCallbackInfo<Value>& args) {
DCHECK(args.IsConstructCall());
CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount);
SetInternalFields(args.This(), nullptr);
Environment* env = Environment::GetCurrent(args);
DCHECK_NOT_NULL(env);
SetInternalFields(env->isolate_data(), args.This(), nullptr);
}

Local<FunctionTemplate> BaseObject::MakeLazilyInitializedJSTemplate(
Expand Down
11 changes: 6 additions & 5 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ namespace worker {
class TransferData;
}

extern uint16_t kNodeEmbedderId;

class BaseObject : public MemoryRetainer {
public:
enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount };
Expand Down Expand Up @@ -74,10 +72,13 @@ class BaseObject : public MemoryRetainer {
// was also passed to the `BaseObject()` constructor initially.
// This may return `nullptr` if the C++ object has not been constructed yet,
// e.g. when the JS object used `MakeLazilyInitializedJSTemplate`.
static inline void SetInternalFields(v8::Local<v8::Object> object,
static inline void SetInternalFields(IsolateData* isolate_data,
v8::Local<v8::Object> object,
void* slot);
static inline bool IsBaseObject(v8::Local<v8::Object> object);
static inline void TagBaseObject(v8::Local<v8::Object> object);
static inline bool IsBaseObject(IsolateData* isolate_data,
v8::Local<v8::Object> object);
static inline void TagBaseObject(IsolateData* isolate_data,
v8::Local<v8::Object> object);
static void LazilyInitializedJSTemplateConstructor(
const v8::FunctionCallbackInfo<v8::Value>& args);
static inline BaseObject* FromJSObject(v8::Local<v8::Value> object);
Expand Down
8 changes: 8 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,14 @@ inline uv_loop_t* IsolateData::event_loop() const {
return event_loop_;
}

inline uint16_t* IsolateData::embedder_id_for_cppgc() const {
return &(wrapper_data_->cppgc_id);
}

inline uint16_t* IsolateData::embedder_id_for_non_cppgc() const {
return &(wrapper_data_->non_cppgc_id);
}

inline NodeArrayBufferAllocator* IsolateData::node_allocator() const {
return node_allocator_;
}
Expand Down
47 changes: 47 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "tracing/agent.h"
#include "tracing/traced_value.h"
#include "util-inl.h"
#include "v8-cppgc.h"
#include "v8-profiler.h"

#include <algorithm>
Expand All @@ -28,6 +29,7 @@
#include <iostream>
#include <limits>
#include <memory>
#include <unordered_map>

namespace node {

Expand Down Expand Up @@ -497,6 +499,11 @@ void IsolateData::CreateProperties() {
contextify::ContextifyContext::InitializeGlobalTemplates(this);
}

constexpr uint16_t kDefaultCppGCEmebdderID = 0x90de;
Mutex IsolateData::isolate_data_mutex_;
std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
IsolateData::wrapper_data_map_;

IsolateData::IsolateData(Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
Expand All @@ -510,6 +517,46 @@ IsolateData::IsolateData(Isolate* isolate,
snapshot_data_(snapshot_data) {
options_.reset(
new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
v8::CppHeap* cpp_heap = isolate->GetCppHeap();

uint16_t cppgc_id = kDefaultCppGCEmebdderID;
if (cpp_heap != nullptr) {
// The general convention of the wrappable layout for cppgc in the
// ecosystem is:
// [ 0 ] -> embedder id
// [ 1 ] -> wrappable instance
// If the Isolate includes a CppHeap attached by another embedder,
// And if they also use the field 0 for the ID, we DCHECK that
// the layout matches our layout, and record the embedder ID for cppgc
// to avoid accidentally enabling cppgc on non-cppgc-managed wrappers .
v8::WrapperDescriptor descriptor = cpp_heap->wrapper_descriptor();
if (descriptor.wrappable_type_index == BaseObject::kEmbedderType) {
cppgc_id = descriptor.embedder_id_for_garbage_collected;
DCHECK_EQ(descriptor.wrappable_instance_index, BaseObject::kSlot);
}
// If the CppHeap uses the slot we use to put non-cppgc-traced BaseObject
// for embedder ID, V8 could accidentally enable cppgc on them. So
// safe guard against this.
DCHECK_NE(descriptor.wrappable_type_index, BaseObject::kSlot);
}
// We do not care about overflow since we just want this to be different
// from the cppgc id.
uint16_t non_cppgc_id = cppgc_id + 1;

{
// GC could still be run after the IsolateData is destroyed, so we store
// the ids in a static map to ensure pointers to them are still valid
// then. In practice there should be very few variants of the cppgc id
// in one process so the size of this map should be very small.
node::Mutex::ScopedLock lock(isolate_data_mutex_);
auto it = wrapper_data_map_.find(cppgc_id);
if (it == wrapper_data_map_.end()) {
auto pair = wrapper_data_map_.emplace(
cppgc_id, new PerIsolateWrapperData{cppgc_id, non_cppgc_id});
it = pair.first;
}
wrapper_data_ = it->second.get();
}

if (snapshot_data == nullptr) {
CreateProperties();
Expand Down
14 changes: 14 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,19 @@ struct IsolateDataSerializeInfo {
const IsolateDataSerializeInfo& i);
};

struct PerIsolateWrapperData {
uint16_t cppgc_id;
uint16_t non_cppgc_id;
};

class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
public:
IsolateData(v8::Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform = nullptr,
ArrayBufferAllocator* node_allocator = nullptr,
const SnapshotData* snapshot_data = nullptr);

SET_MEMORY_INFO_NAME(IsolateData)
SET_SELF_SIZE(IsolateData)
void MemoryInfo(MemoryTracker* tracker) const override;
Expand All @@ -139,6 +145,9 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
bool is_building_snapshot() const { return is_building_snapshot_; }
void set_is_building_snapshot(bool value) { is_building_snapshot_ = value; }

uint16_t* embedder_id_for_cppgc() const;
uint16_t* embedder_id_for_non_cppgc() const;

inline uv_loop_t* event_loop() const;
inline MultiIsolatePlatform* platform() const;
inline const SnapshotData* snapshot_data() const;
Expand Down Expand Up @@ -223,6 +232,11 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
std::shared_ptr<PerIsolateOptions> options_;
worker::Worker* worker_context_ = nullptr;
bool is_building_snapshot_ = false;
PerIsolateWrapperData* wrapper_data_;

static Mutex isolate_data_mutex_;
static std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
wrapper_data_map_;
};

struct ContextInfo {
Expand Down
11 changes: 7 additions & 4 deletions src/node_messaging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ class SerializerDelegate : public ValueSerializer::Delegate {
}

Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object) override {
if (BaseObject::IsBaseObject(object)) {
if (BaseObject::IsBaseObject(env_->isolate_data(), object)) {
return WriteHostObject(
BaseObjectPtr<BaseObject> { Unwrap<BaseObject>(object) });
}
Expand Down Expand Up @@ -520,7 +520,8 @@ Maybe<bool> Message::Serialize(Environment* env,
serializer.TransferArrayBuffer(id, ab);
continue;
} else if (entry->IsObject() &&
BaseObject::IsBaseObject(entry.As<Object>())) {
BaseObject::IsBaseObject(env->isolate_data(),
entry.As<Object>())) {
// Check if the source MessagePort is being transferred.
if (!source_port.IsEmpty() && entry == source_port) {
ThrowDataCloneException(
Expand Down Expand Up @@ -1287,7 +1288,8 @@ JSTransferable::NestedTransferables() const {
Local<Value> value;
if (!list->Get(context, i).ToLocal(&value))
return Nothing<BaseObjectList>();
if (value->IsObject() && BaseObject::IsBaseObject(value.As<Object>()))
if (value->IsObject() &&
BaseObject::IsBaseObject(env()->isolate_data(), value.As<Object>()))
ret.emplace_back(Unwrap<BaseObject>(value));
}
return Just(ret);
Expand Down Expand Up @@ -1342,7 +1344,8 @@ BaseObjectPtr<BaseObject> JSTransferable::Data::Deserialize(
if (!env->messaging_deserialize_create_object()
->Call(context, Null(env->isolate()), 1, &info)
.ToLocal(&ret) ||
!ret->IsObject() || !BaseObject::IsBaseObject(ret.As<Object>())) {
!ret->IsObject() ||
!BaseObject::IsBaseObject(env->isolate_data(), ret.As<Object>())) {
return {};
}

Expand Down
6 changes: 4 additions & 2 deletions src/node_snapshotable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1159,14 +1159,16 @@ void DeserializeNodeInternalFields(Local<Object> holder,

StartupData SerializeNodeContextInternalFields(Local<Object> holder,
int index,
void* env) {
void* callback_data) {
// We only do one serialization for the kEmbedderType slot, the result
// contains everything necessary for deserializing the entire object,
// including the fields whose index is bigger than kEmbedderType
// (most importantly, BaseObject::kSlot).
// For Node.js this design is enough for all the native binding that are
// serializable.
if (index != BaseObject::kEmbedderType || !BaseObject::IsBaseObject(holder)) {
Environment* env = static_cast<Environment*>(callback_data);
if (index != BaseObject::kEmbedderType ||
!BaseObject::IsBaseObject(env->isolate_data(), holder)) {
return StartupData{nullptr, 0};
}

Expand Down
1 change: 1 addition & 0 deletions test/cctest/node_test_fixture.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ void NodeTestEnvironment::SetUp() {
}

void NodeTestEnvironment::TearDown() {
cppgc::ShutdownProcess();
v8::V8::Dispose();
v8::V8::DisposePlatform();
NodeZeroIsolateTestFixture::platform->Shutdown();
Expand Down
Loading

0 comments on commit 168c23a

Please sign in to comment.