From ab5e9cb3c31d01490c3ea2a6447407a7cddbd945 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 30 Oct 2026 06:56:37 +0100 Subject: [PATCH 1/2] src: use per-type tags for cppgc wrappers V8 only defines CppHeapPointerTag::kDefaultTag when the sandbox is disabled, so building with --experimental-pointer-compression-shared-cage (which enables the sandbox) failed to compile. Give each cppgc-managed wrapper type its own tag, declared with the new SET_CPPGC_TAG() macro, and unwrap through v8::Object::Unwrap() with that tag instead of reading the raw pointer from the internal field. With the sandbox enabled, unwrapping an object as the wrong type now yields nullptr. The deprecated SetCppgcReference() gets its own tag. Signed-off-by: Matteo Collina --- src/README.md | 3 +++ src/cppgc_helpers-inl.h | 16 +++++--------- src/cppgc_helpers.h | 8 +++++++ src/env.cc | 2 +- src/node_contextify.h | 2 ++ src/node_v8_embedder.h | 22 +++++++++++++++++++ test/cctest/test_cppgc.cc | 2 ++ .../cctest/test_environment_shared_isolate.cc | 2 +- test/embedding/shared_embedtest.cc | 2 +- 9 files changed, 45 insertions(+), 14 deletions(-) diff --git a/src/README.md b/src/README.md index 7cdf099ff819..7120013236a1 100644 --- a/src/README.md +++ b/src/README.md @@ -1120,6 +1120,9 @@ A `cppgc`-managed native wrapper should look something like this: class MyWrap final : CPPGC_MIXIN(MyWrap) { public: SET_CPPGC_NAME(MyWrap) // Sets the heap snapshot name to "Node / MyWrap" + // Uses CppgcWrapperTag::kMyWrap from src/node_v8_embedder.h when wrapping + // and unwrapping. Each class needs its own tag. + SET_CPPGC_TAG(MyWrap) // The constructor can only be called by `cppgc::MakeGarbageCollected()`. MyWrap(Environment* env, v8::Local object); diff --git a/src/cppgc_helpers-inl.h b/src/cppgc_helpers-inl.h index 4db197d694f8..ae9c4d9a5cb8 100644 --- a/src/cppgc_helpers-inl.h +++ b/src/cppgc_helpers-inl.h @@ -15,7 +15,7 @@ void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local obj) { ptr->traced_reference_ = v8::TracedReference(isolate, obj); // Note that ptr must be of concrete type T in Wrap. auto* wrappable = static_cast(ptr); - v8::Object::Wrap(isolate, obj, wrappable); + v8::Object::Wrap(isolate, obj, wrappable); // Keep the layout consistent with BaseObjects. obj->SetAlignedPointerInInternalField( kEmbedderType, @@ -32,19 +32,13 @@ void CppgcMixin::Wrap(T* ptr, Environment* env, v8::Local obj) { template T* CppgcMixin::Unwrap(v8::Local obj) { - // We are not using v8::Object::Unwrap currently because that requires - // access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim - // with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a - // signature consistent with BaseObject::Unwrap() to avoid churn. Since - // cppgc-managed objects share the same layout as BaseObjects, just unwrap - // from the pointer in the internal field, which should be valid as long as - // the object is still alive. + // The isolate is not passed in to keep the signature consistent with + // BaseObject::Unwrap() and the ASSIGN_OR_RETURN_UNWRAP_CPPGC macro. if (obj->InternalFieldCount() != T::kInternalFieldCount) { return nullptr; } - T* ptr = static_cast(obj->GetAlignedPointerFromInternalField( - T::kSlot, EmbedderDataTag::kDefault)); - return ptr; + return v8::Object::Unwrap(v8::Isolate::GetCurrent(), + obj); } v8::Local CppgcMixin::object() const { diff --git a/src/cppgc_helpers.h b/src/cppgc_helpers.h index 3dfd31bcb29e..a78a483decbf 100644 --- a/src/cppgc_helpers.h +++ b/src/cppgc_helpers.h @@ -8,6 +8,7 @@ #include "cppgc/name-provider.h" #include "cppgc/persistent.h" #include "memory_tracker.h" +#include "node_v8_embedder.h" #include "util.h" #include "v8-cppgc.h" #include "v8-sandbox.h" @@ -37,6 +38,7 @@ class CppgcWrapperListNode; * class MyWrap final : CPPGC_MIXIN(MyWrap) { * public: * SET_CPPGC_NAME(MyWrap) // Sets the heap snapshot name to "Node / MyWrap" + * SET_CPPGC_TAG(MyWrap) // Uses CppgcWrapperTag::kMyWrap for Wrap/Unwrap * void Trace(cppgc::Visitor* visitor) const final { * CppgcMixin::Trace(visitor); * visitor->Trace(...); // Trace any additional owned traceable data @@ -126,6 +128,12 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer { } \ inline const char* MemoryInfoName() const override { return #Klass; } +// This macro sets the tag used to wrap and unwrap the class. Each class needs +// its own entry in node::CppgcWrapperTag. +#define SET_CPPGC_TAG(Klass) \ + static constexpr v8::CppHeapPointerTag kCppHeapPointerTag = \ + ToCppHeapPointerTag(CppgcWrapperTag::k##Klass); + /** * Similar to ASSIGN_OR_RETURN_UNWRAP() but works on cppgc-managed types * inheriting CppgcMixin. diff --git a/src/env.cc b/src/env.cc index c30b6e2f5049..45a810821ed5 100644 --- a/src/env.cc +++ b/src/env.cc @@ -668,7 +668,7 @@ IsolateData::~IsolateData() { void SetCppgcReference(Isolate* isolate, Local object, v8::Object::Wrappable* wrappable) { - v8::Object::Wrap( + v8::Object::Wrap( isolate, object, wrappable); } diff --git a/src/node_contextify.h b/src/node_contextify.h index be3034da39ff..41c276c308e3 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -76,6 +76,7 @@ struct ContextOptions { class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) { public: SET_CPPGC_NAME(ContextifyContext) + SET_CPPGC_TAG(ContextifyContext) void Trace(cppgc::Visitor* visitor) const final; SET_NO_MEMORY_INFO() @@ -192,6 +193,7 @@ class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) { class ContextifyScript final : CPPGC_MIXIN(ContextifyScript) { public: SET_CPPGC_NAME(ContextifyScript) + SET_CPPGC_TAG(ContextifyScript) void Trace(cppgc::Visitor* visitor) const final; SET_NO_MEMORY_INFO() diff --git a/src/node_v8_embedder.h b/src/node_v8_embedder.h index 26a98bdb706f..05e6047f75a7 100644 --- a/src/node_v8_embedder.h +++ b/src/node_v8_embedder.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include +#include "v8-sandbox.h" namespace node { @@ -19,6 +20,27 @@ enum EmbedderDataTag : uint16_t { kPerContextData, }; +// Tags for cppgc-managed wrappers passed to v8::Object::Wrap(). Each wrapper +// type needs its own tag so that, with the V8 sandbox enabled, unwrapping an +// object as the wrong type yields nullptr. +enum class CppgcWrapperTag : uint16_t { + // Used by the deprecated SetCppgcReference(), which wraps embedder types + // unknown to Node.js. + kSetCppgcReference = + static_cast(v8::CppHeapPointerTag::kFirstEmbedderWrappableTag), + kContextifyContext, + kContextifyScript, + kLast = kContextifyScript, +}; + +static_assert( + static_cast(CppgcWrapperTag::kLast) <= + static_cast(v8::CppHeapPointerTag::kLastEmbedderWrappableTag)); + +constexpr v8::CppHeapPointerTag ToCppHeapPointerTag(CppgcWrapperTag tag) { + return static_cast(tag); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc index 478098154665..f072c0c450b1 100644 --- a/test/cctest/test_cppgc.cc +++ b/test/cctest/test_cppgc.cc @@ -133,6 +133,8 @@ using node::CppgcMixin; class RealmBoundWrap final : CPPGC_MIXIN(RealmBoundWrap) { public: SET_CPPGC_NAME(RealmBoundWrap) + static constexpr v8::CppHeapPointerTag kCppHeapPointerTag = + v8::CppHeapPointerTag::kLastEmbedderWrappableTag; DEFAULT_CPPGC_TRACE() SET_NO_MEMORY_INFO() diff --git a/test/cctest/test_environment_shared_isolate.cc b/test/cctest/test_environment_shared_isolate.cc index b3807b7bb709..f017c6d3260e 100644 --- a/test/cctest/test_environment_shared_isolate.cc +++ b/test/cctest/test_environment_shared_isolate.cc @@ -298,7 +298,7 @@ TEST_P(SharedIsolateTest, EnvironmentsComeAndGoWhileSiblingsRun) { ->InstanceTemplate() ->NewInstance(context) .ToLocalChecked(); - v8::Object::Wrap( + v8::Object::Wrap( isolate_, holder, cppgc::MakeGarbageCollected( diff --git a/test/embedding/shared_embedtest.cc b/test/embedding/shared_embedtest.cc index f071f43c5b76..e5434a0b1bfc 100644 --- a/test/embedding/shared_embedtest.cc +++ b/test/embedding/shared_embedtest.cc @@ -39,7 +39,7 @@ int main(int argc, char* argv[]) { v8::Local obj = v8::Object::New(isolate); Wrappable* wrappable = cppgc::MakeGarbageCollected( isolate->GetCppHeap()->GetAllocationHandle()); - v8::Object::Wrap( + v8::Object::Wrap( isolate, obj, wrappable); v8::Local source = v8::String::NewFromUtf8Literal(isolate, "'Hello' + ', World!'"); From 76f24fb8d6f278d11529bb14244e38ef250d57ed Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 30 Oct 2026 08:54:43 +0100 Subject: [PATCH 2/2] test: fix addon tests with the V8 sandbox With the V8 sandbox, ArrayBuffer backing stores must live inside the sandbox. Let V8 allocate the counters in the cppgc-object addon instead of wrapping a static array, skip the nullptr data check for empty buffers in null-buffer-neuter, and account for the eagerly freed main thread buffer in worker-buffer-callback. Signed-off-by: Matteo Collina --- test/addons/cppgc-object/binding.cc | 18 ++++++++++-------- test/addons/null-buffer-neuter/binding.cc | 4 ++++ .../worker-buffer-callback/test-free-called.js | 6 +++++- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/test/addons/cppgc-object/binding.cc b/test/addons/cppgc-object/binding.cc index 90be16147483..5c84a5218a39 100644 --- a/test/addons/cppgc-object/binding.cc +++ b/test/addons/cppgc-object/binding.cc @@ -10,7 +10,7 @@ class CppGCed : public v8::Object::Wrappable { public: - static uint16_t states[2]; + static uint16_t* states; static constexpr int kDestructCount = 0; static constexpr int kTraceCount = 1; @@ -38,18 +38,20 @@ class CppGCed : public v8::Object::Wrappable { void Trace(cppgc::Visitor* visitor) const { states[kTraceCount]++; } }; -uint16_t CppGCed::states[] = {0, 0}; +uint16_t* CppGCed::states = nullptr; void InitModule(v8::Local exports) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); auto context = isolate->GetCurrentContext(); - auto store = v8::ArrayBuffer::NewBackingStore( - CppGCed::states, - sizeof(uint16_t) * 2, - [](void*, size_t, void*) {}, - nullptr); - auto ab = v8::ArrayBuffer::New(isolate, std::move(store)); + // Let V8 allocate the counters, as the V8 sandbox does not allow external + // backing stores. The store is leaked so that the counters outlive the + // CppHeap, whose teardown runs the destructors. + static auto* store = new std::shared_ptr( + v8::ArrayBuffer::NewBackingStore(isolate, sizeof(uint16_t) * 2)); + CppGCed::states = static_cast((*store)->Data()); + std::fill_n(CppGCed::states, 2, 0); + auto ab = v8::ArrayBuffer::New(isolate, *store); exports ->Set(context, diff --git a/test/addons/null-buffer-neuter/binding.cc b/test/addons/null-buffer-neuter/binding.cc index 7575e29fa850..576f547063f8 100644 --- a/test/addons/null-buffer-neuter/binding.cc +++ b/test/addons/null-buffer-neuter/binding.cc @@ -28,8 +28,12 @@ void Run(const v8::FunctionCallbackInfo& args) { FreeCallback, nullptr).ToLocalChecked(); + assert(node::Buffer::Length(buf) == 0); +#ifndef V8_ENABLE_SANDBOX + // With the V8 sandbox, empty buffers point into the sandbox instead. char* data = node::Buffer::Data(buf); assert(data == nullptr); +#endif } } diff --git a/test/addons/worker-buffer-callback/test-free-called.js b/test/addons/worker-buffer-callback/test-free-called.js index c15cdde432cd..aef04ced1daf 100644 --- a/test/addons/worker-buffer-callback/test-free-called.js +++ b/test/addons/worker-buffer-callback/test-free-called.js @@ -14,6 +14,10 @@ assert.strictEqual(getFreeCallCount(), 0); const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true }); +// With the V8 sandbox, the data is copied into the sandbox and freed right +// away, so the buffer created when loading the binding above is freed too. +const expected = common.hasV8Sandbox ? 2 : 1; + w.on('exit', common.mustCall(() => { - assert.strictEqual(getFreeCallCount(), 1); + assert.strictEqual(getFreeCallCount(), expected); }));