From 4652302d3fd4f336e889efd011e5b99cbb8e1923 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 21 Aug 2026 12:06:32 +0000 Subject: [PATCH 1/2] src: support building with the V8 sandbox With V8_ENABLE_SANDBOX every ArrayBuffer backing store has to be allocated inside the sandbox, so memory that Node.js or a library allocated itself cannot be wrapped and has to be copied in. Several places already special-cased this, each with its own #ifdef, but a build with the sandbox enabled still failed to compile (`kMaxSafeBufferSizeForSandbox`), aborted in `crypto.randomUUID()` (`SecureBuffer` was UNREACHABLE) and threw from every caller of the malloc-owning `Buffer::New()` (v8.serialize, string transcoding, IPC, the public `Buffer::New(isolate, data, length)`), and trace_events, SEA assets and FFI still wrapped outside memory unconditionally. Add `AdoptIntoBackingStore()`, which wraps the memory as before in regular builds and copies it into an isolate-allocated backing store, running the deleter right away, when the sandbox is enabled, and use it at all of those sites so the #ifdef lives in one place. The trace category flag cannot be copied since JS polls it, so under the sandbox `getCategoryEnabledBuffer()` returns nothing and lib falls back to `isTraceCategoryEnabled()`; FFI zero-copy views throw ERR_OPERATION_FAILED there for the same reason. Signed-off-by: Shelley Vohr --- lib/ffi.js | 9 ++- lib/internal/http.js | 8 +-- lib/internal/trace_events.js | 19 +++++- lib/internal/util/debuglog.js | 10 +-- src/crypto/crypto_dh.cc | 22 ++----- src/crypto/crypto_util.cc | 49 ++++----------- src/crypto/crypto_x509.cc | 23 ++----- src/ffi/data.cc | 26 ++++++-- src/node_buffer.cc | 63 +++++++++---------- src/node_sea.cc | 4 +- src/node_sqlite.cc | 19 ++---- src/node_trace_events.cc | 5 +- src/util.cc | 21 +++++++ src/util.h | 12 ++++ test/cctest/test_dataqueue.cc | 43 +++++++------ test/cctest/test_environment.cc | 4 ++ test/cctest/test_linked_binding.cc | 16 ++--- test/common/README.md | 7 +++ test/common/index.js | 2 + test/common/index.mjs | 2 + test/ffi/test-ffi-memory.js | 16 ++++- test/parallel/test-crypto-secure-heap.js | 4 ++ ...race-events-get-category-enabled-buffer.js | 11 ++-- 23 files changed, 218 insertions(+), 177 deletions(-) diff --git a/lib/ffi.js b/lib/ffi.js index cbd188793200..ce8345f155fb 100644 --- a/lib/ffi.js +++ b/lib/ffi.js @@ -256,11 +256,10 @@ function exportString(str, data, len, encoding = 'utf8') { throw new ERR_OUT_OF_RANGE('len', `>= ${requiredLength}`, len); } - const targetBuffer = toBuffer(data, len, false); - const dataLength = sourceBuffer.length; - - sourceBuffer.copy(targetBuffer, 0, 0, dataLength); - targetBuffer.fill(0, dataLength, dataLength + terminatorSize); + const terminated = Buffer.allocUnsafe(requiredLength); + sourceBuffer.copy(terminated); + terminated.fill(0, sourceBuffer.length); + exportBytes(terminated, data, len); } function exportBuffer(source, data, len) { diff --git a/lib/internal/http.js b/lib/internal/http.js index 304ef04d6638..d4efd7ad3adb 100644 --- a/lib/internal/http.js +++ b/lib/internal/http.js @@ -10,7 +10,7 @@ const { const { setUnrefTimeout } = require('internal/timers'); const { - getCategoryEnabledBuffer, + categoryEnabledChecker, trace, nodeTraceEventCategory, kAsyncBegin, @@ -44,11 +44,7 @@ function getNextTraceEventId() { return ++traceEventId; } -const httpEnabled = getCategoryEnabledBuffer('node.http'); - -function isTraceHTTPEnabled() { - return httpEnabled[0] > 0; -} +const isTraceHTTPEnabled = categoryEnabledChecker('node.http'); const traceEventCategory = nodeTraceEventCategory('node.http'); diff --git a/lib/internal/trace_events.js b/lib/internal/trace_events.js index d240f4ad589e..77959e4d5825 100644 --- a/lib/internal/trace_events.js +++ b/lib/internal/trace_events.js @@ -1,6 +1,11 @@ 'use strict'; -const { getCategoryEnabledBuffer, trace, usePerfetto } = internalBinding('trace_events'); +const { + getCategoryEnabledBuffer, + isTraceCategoryEnabled, + trace, + usePerfetto, +} = internalBinding('trace_events'); const { CHAR_UPPERCASE_B, CHAR_LOWERCASE_B, @@ -17,6 +22,16 @@ if (usePerfetto) { nodeTraceEventCategory = (category) => `node,${category}`; } +// The returned function reads the category's enabled flag directly when the +// binding can expose it to JS, and asks the tracing controller otherwise. +function categoryEnabledChecker(category) { + const buffer = getCategoryEnabledBuffer(category); + if (buffer === undefined) { + return () => isTraceCategoryEnabled(category); + } + return () => buffer[0] > 0; +} + // The async events describe the execution of a single asynchronous operation, and are // used to measure the time spent in a single asynchronous operation. // Async events may overlap with each other. Different events do not have @@ -44,7 +59,7 @@ const kTraceInstant = CHAR_LOWERCASE_N; module.exports = { usePerfetto, - getCategoryEnabledBuffer, + categoryEnabledChecker, trace, nodeTraceEventCategory, kAsyncBegin, diff --git a/lib/internal/util/debuglog.js b/lib/internal/util/debuglog.js index 1cb3ba4df520..2eeab81dcff4 100644 --- a/lib/internal/util/debuglog.js +++ b/lib/internal/util/debuglog.js @@ -16,7 +16,7 @@ const { } = primordials; const { inspect, format, formatWithOptions } = require('internal/util/inspect'); const { - getCategoryEnabledBuffer, + categoryEnabledChecker, trace, nodeTraceEventCategory, kAsyncBegin, @@ -388,14 +388,14 @@ function debugWithTimer(set, cb) { } const traceCategory = nodeTraceEventCategory(`node.${StringPrototypeToLowerCase(set)}`); - let traceCategoryBuffer; + let traceCategoryEnabled; let debugLogCategoryEnabled = false; let timerFlags = kNone; function ensureTimerFlagsAreUpdated() { timerFlags &= ~kSkipTrace; - if (traceCategoryBuffer[0] === 0) { + if (!traceCategoryEnabled()) { timerFlags |= kSkipTrace; } } @@ -469,7 +469,7 @@ function debugWithTimer(set, cb) { } emitWarningIfNeeded(set); debugLogCategoryEnabled = testEnabled(set); - traceCategoryBuffer = getCategoryEnabledBuffer(traceCategory); + traceCategoryEnabled = categoryEnabledChecker(traceCategory); timerFlags = kNone; @@ -477,7 +477,7 @@ function debugWithTimer(set, cb) { timerFlags |= kSkipLog; } - if (traceCategoryBuffer[0] === 0) { + if (!traceCategoryEnabled()) { timerFlags |= kSkipTrace; } diff --git a/src/crypto/crypto_dh.cc b/src/crypto/crypto_dh.cc index 92780cfeeebf..58f6fe59ad4e 100644 --- a/src/crypto/crypto_dh.cc +++ b/src/crypto/crypto_dh.cc @@ -22,8 +22,6 @@ using ncrypto::DHPointer; using ncrypto::EVPKeyCtxPointer; using ncrypto::EVPKeyPointer; using v8::ArrayBuffer; -using v8::BackingStoreInitializationMode; -using v8::BackingStoreOnFailureMode; using v8::ConstructorBehavior; using v8::Context; using v8::DontDelete; @@ -60,21 +58,8 @@ MaybeLocal DataPointerToBuffer(Environment* env, DataPointer&& data) { struct Flag { bool secure; }; -#ifdef V8_ENABLE_SANDBOX - auto backing = ArrayBuffer::NewBackingStore( + auto backing = AdoptIntoBackingStore( env->isolate(), - data.size(), - BackingStoreInitializationMode::kUninitialized, - BackingStoreOnFailureMode::kReturnNull); - if (!backing) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return MaybeLocal(); - } - if (data.size() > 0) { - memcpy(backing->Data(), data.get(), data.size()); - } -#else - auto backing = ArrayBuffer::NewBackingStore( data.get(), data.size(), [](void* data, size_t len, void* ptr) { @@ -83,7 +68,10 @@ MaybeLocal DataPointerToBuffer(Environment* env, DataPointer&& data) { }, new Flag{data.isSecure()}); data.release(); -#endif // V8_ENABLE_SANDBOX + if (!backing) { + THROW_ERR_MEMORY_ALLOCATION_FAILED(env); + return MaybeLocal(); + } auto ab = ArrayBuffer::New(env->isolate(), std::move(backing)); return Buffer::New(env, ab, 0, ab->ByteLength()).FromMaybe(Local()); diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 133a5c7f7f1d..64188f86a365 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -32,8 +32,6 @@ using v8::Array; using v8::ArrayBuffer; using v8::ArrayBufferView; using v8::BackingStore; -using v8::BackingStoreInitializationMode; -using v8::BackingStoreOnFailureMode; using v8::BigInt; using v8::Context; using v8::EscapableHandleScope; @@ -446,33 +444,18 @@ std::unique_ptr ByteSource::ReleaseToBackingStore( // It's ok for allocated_data_ to be nullptr but // only if size_ is zero. CHECK_IMPLIES(size_ > 0, allocated_data_ != nullptr); -#ifdef V8_ENABLE_SANDBOX - // If the v8 sandbox is enabled, then all array buffers must be allocated - // via the isolate. External buffers are not allowed. So, instead of wrapping - // the allocated data we'll copy it instead. - - // TODO(@jasnell): It would be nice to use an abstracted utility to do this - // branch instead of duplicating the V8_ENABLE_SANDBOX check each time. - std::unique_ptr ptr = ArrayBuffer::NewBackingStore( + std::unique_ptr ptr = AdoptIntoBackingStore( env->isolate(), + allocated_data_, size(), - BackingStoreInitializationMode::kUninitialized, - BackingStoreOnFailureMode::kReturnNull); + [](void* data, size_t length, void*) { + OPENSSL_clear_free(data, length); + }, + nullptr); if (!ptr) { THROW_ERR_MEMORY_ALLOCATION_FAILED(env); return nullptr; } - memcpy(ptr->Data(), allocated_data_, size()); - OPENSSL_clear_free(allocated_data_, size_); -#else - std::unique_ptr ptr = ArrayBuffer::NewBackingStore( - allocated_data_, - size(), - [](void* data, size_t length, void* deleter_data) { - OPENSSL_clear_free(deleter_data, length); - }, allocated_data_); -#endif // V8_ENABLE_SANDBOX - CHECK(ptr); allocated_data_ = nullptr; data_ = nullptr; size_ = 0; @@ -837,17 +820,6 @@ namespace { // initialized, SecureBuffer will automatically use it. void SecureBuffer(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); -#ifdef V8_ENABLE_SANDBOX - // The v8 sandbox is enabled, so we cannot use the secure heap because - // the sandbox requires that all array buffers be allocated via the isolate. - // That is fundamentally incompatible with the secure heap which allocates - // in openssl's secure heap area. Instead we'll just throw an error here. - // - // That said, we really shouldn't get here in the first place since the - // option to enable the secure heap is only available when the sandbox - // is disabled. - UNREACHABLE(); -#else CHECK(args[0]->IsUint32()); uint32_t len = args[0].As()->Value(); @@ -857,7 +829,10 @@ void SecureBuffer(const FunctionCallbackInfo& args) { } auto released = data.release(); - std::shared_ptr store = ArrayBuffer::NewBackingStore( + // Under V8_ENABLE_SANDBOX this ends up as a plain copy, which is fine: + // --secure-heap is unavailable there, so SecureAlloc() is OPENSSL_malloc(). + std::shared_ptr store = AdoptIntoBackingStore( + env->isolate(), released.data, released.len, [](void* data, size_t len, void* deleter_data) { @@ -871,10 +846,12 @@ void SecureBuffer(const FunctionCallbackInfo& args) { true); }, nullptr); + if (!store) { + return THROW_ERR_MEMORY_ALLOCATION_FAILED(env); + } Local buffer = ArrayBuffer::New(env->isolate(), store); args.GetReturnValue().Set(Uint8Array::New(buffer, 0, len)); -#endif // V8_ENABLE_SANDBOX } void SecureHeapUsed(const FunctionCallbackInfo& args) { diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc index 6ed7d7d25db0..66bf83cc3a4f 100644 --- a/src/crypto/crypto_x509.cc +++ b/src/crypto/crypto_x509.cc @@ -27,8 +27,6 @@ using ncrypto::X509View; using v8::Array; using v8::ArrayBuffer; using v8::ArrayBufferView; -using v8::BackingStoreInitializationMode; -using v8::BackingStoreOnFailureMode; using v8::Boolean; using v8::Context; using v8::Date; @@ -140,29 +138,18 @@ MaybeLocal ToBuffer(Environment* env, BIOPointer* bio) { BUF_MEM* mem = *bio; if (!mem) [[unlikely]] return {}; -#ifdef V8_ENABLE_SANDBOX - // If the v8 sandbox is enabled, then all array buffers must be allocated - // via the isolate. External buffers are not allowed. So, instead of wrapping - // the BIOPointer we'll copy it instead. - auto backing = ArrayBuffer::NewBackingStore( + auto backing = AdoptIntoBackingStore( env->isolate(), - mem->length, - BackingStoreInitializationMode::kUninitialized, - BackingStoreOnFailureMode::kReturnNull); - if (!backing) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return MaybeLocal(); - } - memcpy(backing->Data(), mem->data, mem->length); -#else - auto backing = ArrayBuffer::NewBackingStore( mem->data, mem->length, [](void*, size_t, void* data) { BIOPointer free_me(static_cast(data)); }, bio->release()); -#endif // V8_ENABLE_SANDBOX + if (!backing) { + THROW_ERR_MEMORY_ALLOCATION_FAILED(env); + return MaybeLocal(); + } auto ab = ArrayBuffer::New(env->isolate(), std::move(backing)); Local ret; if (!Buffer::New(env, ab, 0, ab->ByteLength()).ToLocal(&ret)) return {}; diff --git a/src/ffi/data.cc b/src/ffi/data.cc index 308bde60cae2..74a2e7d77f7f 100644 --- a/src/ffi/data.cc +++ b/src/ffi/data.cc @@ -535,6 +535,17 @@ void ToString(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(out); } +// Foreign memory lies outside the V8 sandbox and cannot back an ArrayBuffer. +static bool ZeroCopyUnavailable(Environment* env) { +#ifdef V8_ENABLE_SANDBOX + THROW_ERR_OPERATION_FAILED( + env, "Zero-copy views are not available when the V8 sandbox is enabled"); + return true; +#else + return false; +#endif +} + void ToBuffer(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -580,9 +591,12 @@ void ToBuffer(const FunctionCallbackInfo& args) { return; } + bool copy = args.Length() < 3 || args[2]->IsUndefined() || + args[2]->BooleanValue(isolate); + if (!copy && ZeroCopyUnavailable(env)) return; + Local buf; - if (args.Length() < 3 || args[2]->IsUndefined() || - args[2]->BooleanValue(isolate)) { + if (copy) { if (!Buffer::Copy(env, reinterpret_cast(ptr), len).ToLocal(&buf)) { return; } @@ -642,10 +656,12 @@ void ToArrayBuffer(const FunctionCallbackInfo& args) { return; } - Local ab; + bool copy = args.Length() < 3 || args[2]->IsUndefined() || + args[2]->BooleanValue(isolate); + if (!copy && ZeroCopyUnavailable(env)) return; - if (args.Length() < 3 || args[2]->IsUndefined() || - args[2]->BooleanValue(isolate)) { + Local ab; + if (copy) { std::unique_ptr store = ArrayBuffer::NewBackingStore(isolate, len); memcpy(store->Data(), reinterpret_cast(ptr), len); diff --git a/src/node_buffer.cc b/src/node_buffer.cc index a0453800d84a..f94f310335bd 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -88,7 +88,7 @@ namespace { class CallbackInfo : public Cleanable { public: - static inline Local CreateTrackedArrayBuffer( + static inline MaybeLocal CreateTrackedArrayBuffer( Environment* env, char* data, size_t length, @@ -114,7 +114,7 @@ class CallbackInfo : public Cleanable { Environment* const env_; }; -Local CallbackInfo::CreateTrackedArrayBuffer( +MaybeLocal CallbackInfo::CreateTrackedArrayBuffer( Environment* env, char* data, size_t length, @@ -124,10 +124,18 @@ Local CallbackInfo::CreateTrackedArrayBuffer( CHECK_IMPLIES(data == nullptr, length == 0); CallbackInfo* self = new CallbackInfo(env, callback, data, hint); - std::unique_ptr bs = - ArrayBuffer::NewBackingStore(data, length, [](void*, size_t, void* arg) { + std::unique_ptr bs = AdoptIntoBackingStore( + env->isolate(), + data, + length, + [](void*, size_t, void* arg) { static_cast(arg)->OnBackingStoreFree(); - }, self); + }, + self); + if (!bs) { + THROW_ERR_MEMORY_ALLOCATION_FAILED(env); + return MaybeLocal(); + } Local ab = ArrayBuffer::New(env->isolate(), std::move(bs)); // V8 simply ignores the BackingStore deleter callback if data == nullptr, @@ -135,7 +143,7 @@ Local CallbackInfo::CreateTrackedArrayBuffer( if (data == nullptr) { ab->Detach(Local()).Check(); self->OnBackingStoreFree(); // This calls `callback` asynchronously. - } else { + } else if (ab->Data() == data) { // Store the ArrayBuffer so that we can detach it later. self->persistent_.Reset(env->isolate(), ab); self->persistent_.SetWeak(); @@ -144,7 +152,6 @@ Local CallbackInfo::CreateTrackedArrayBuffer( return ab; } - CallbackInfo::CallbackInfo(Environment* env, FreeCallback callback, char* data, @@ -481,11 +488,13 @@ MaybeLocal New(Environment* env, return Local(); } - Local ab = - CallbackInfo::CreateTrackedArrayBuffer(env, data, length, callback, hint); - if (ab->SetPrivate(env->context(), + Local ab; + if (!CallbackInfo::CreateTrackedArrayBuffer(env, data, length, callback, hint) + .ToLocal(&ab) || + ab->SetPrivate(env->context(), env->untransferable_object_private_symbol(), - True(env->isolate())).IsNothing()) { + True(env->isolate())) + .IsNothing()) { return Local(); } MaybeLocal maybe_ui = Buffer::New(env, ab, 0, length); @@ -529,32 +538,24 @@ MaybeLocal New(Environment* env, } } -#if defined(V8_ENABLE_SANDBOX) - // When v8 sandbox is enabled, external backing stores are not supported - // since all arraybuffer allocations are expected to be done by the isolate. - // Since this violates the contract of this function, let's free the data and - // throw an error. - free(data); - THROW_ERR_OPERATION_FAILED( - env->isolate(), - "Wrapping external data is not supported when the v8 sandbox is enabled"); - return MaybeLocal(); -#else EscapableHandleScope handle_scope(env->isolate()); - auto free_callback = [](void* data, size_t length, void* deleter_data) { - free(data); - }; - std::unique_ptr bs = - ArrayBuffer::NewBackingStore(data, length, free_callback, nullptr); - + std::unique_ptr bs = AdoptIntoBackingStore( + env->isolate(), + data, + length, + [](void* data, size_t, void*) { free(data); }, + nullptr); + if (!bs) { + THROW_ERR_MEMORY_ALLOCATION_FAILED(env); + return MaybeLocal(); + } Local ab = ArrayBuffer::New(env->isolate(), std::move(bs)); Local obj; if (Buffer::New(env, ab, 0, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); -#endif } namespace { @@ -1609,9 +1610,7 @@ inline size_t CheckNumberToSize(Local number) { double maxSize = static_cast(std::numeric_limits::max()); CHECK(value >= 0 && value < maxSize); size_t size = static_cast(value); -#ifdef V8_ENABLE_SANDBOX - CHECK_LE(size, kMaxSafeBufferSizeForSandbox); -#endif + CHECK_LE(size, ArrayBuffer::kMaxByteLength); return size; } diff --git a/src/node_sea.cc b/src/node_sea.cc index 03d487f1dba7..66265382eabb 100644 --- a/src/node_sea.cc +++ b/src/node_sea.cc @@ -846,11 +846,13 @@ void GetAsset(const FunctionCallbackInfo& args) { } // We cast away the constness here, the JS land should ensure that // the data is not mutated. - std::unique_ptr store = ArrayBuffer::NewBackingStore( + std::unique_ptr store = AdoptIntoBackingStore( + args.GetIsolate(), const_cast(it->second.data()), it->second.size(), [](void*, size_t, void*) {}, nullptr); + CHECK(store); Local ab = ArrayBuffer::New(args.GetIsolate(), std::move(store)); args.GetReturnValue().Set(ab); } diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index af211ab5fc04..1100ed3ccea4 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -26,7 +26,6 @@ namespace sqlite { using v8::Array; using v8::ArrayBuffer; using v8::BackingStoreInitializationMode; -using v8::BackingStoreOnFailureMode; using v8::BigInt; using v8::Boolean; using v8::ConstructorBehavior; @@ -1993,27 +1992,17 @@ void DatabaseSync::Serialize(const FunctionCallbackInfo& args) { return; } - // V8 sandbox forbids external backing stores so allocate inside the - // sandbox and copy. Without sandbox wrap the output directly using - // sqlite3_free as the destructor to avoid the copy. -#ifdef V8_ENABLE_SANDBOX - auto free_data = OnScopeLeave([&] { sqlite3_free(data); }); - auto store = ArrayBuffer::NewBackingStore( + auto store = AdoptIntoBackingStore( env->isolate(), + data, size, - BackingStoreInitializationMode::kUninitialized, - BackingStoreOnFailureMode::kReturnNull); + [](void* ptr, size_t, void*) { sqlite3_free(ptr); }, + nullptr); if (!store) { THROW_ERR_MEMORY_ALLOCATION_FAILED(env); return; } - memcpy(store->Data(), data, size); Local ab = ArrayBuffer::New(env->isolate(), std::move(store)); -#else - auto store = ArrayBuffer::NewBackingStore( - data, size, [](void* ptr, size_t, void*) { sqlite3_free(ptr); }, nullptr); - Local ab = ArrayBuffer::New(env->isolate(), std::move(store)); -#endif args.GetReturnValue().Set(Uint8Array::New(ab, 0, size)); } diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index fec72919eb83..db0a37b85e13 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -133,7 +133,9 @@ static void SetTraceCategoryStateUpdateHandler( static void GetCategoryEnabledBuffer(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); - + // The flag lives outside the V8 sandbox and cannot back an ArrayBuffer + // there; lib/internal/trace_events.js falls back to isTraceCategoryEnabled(). +#ifndef V8_ENABLE_SANDBOX Isolate* isolate = args.GetIsolate(); node::Utf8Value category_name(isolate, args[0]); @@ -150,6 +152,7 @@ static void GetCategoryEnabledBuffer(const FunctionCallbackInfo& args) { v8::Local u8 = v8::Uint8Array::New(ab, 0, 1); args.GetReturnValue().Set(u8); +#endif } void NodeCategorySet::Initialize(Local target, diff --git a/src/util.cc b/src/util.cc index ce45c1ad4ede..7f74f9fb876b 100644 --- a/src/util.cc +++ b/src/util.cc @@ -87,6 +87,7 @@ namespace node { using v8::ArrayBuffer; using v8::ArrayBufferView; +using v8::BackingStore; using v8::Context; using v8::FunctionTemplate; using v8::Isolate; @@ -670,6 +671,26 @@ Local UnionBytes::ToStringChecked(Isolate* isolate) const { } } +std::unique_ptr AdoptIntoBackingStore( + Isolate* isolate, + void* data, + size_t byte_length, + BackingStore::DeleterCallback deleter, + void* deleter_data) { +#ifdef V8_ENABLE_SANDBOX + std::unique_ptr store = ArrayBuffer::NewBackingStore( + isolate, + byte_length, + v8::BackingStoreInitializationMode::kUninitialized, + v8::BackingStoreOnFailureMode::kReturnNull); + if (store && byte_length > 0) memcpy(store->Data(), data, byte_length); + if (data != nullptr) deleter(data, byte_length, deleter_data); + return store; +#else + return ArrayBuffer::NewBackingStore(data, byte_length, deleter, deleter_data); +#endif +} + RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data) : allocator_{ArrayBuffer::Allocator::NewDefaultAllocator()} { isolate_ = Isolate::Allocate(); diff --git a/src/util.h b/src/util.h index 621d2fbf4ee6..ec7fbad43798 100644 --- a/src/util.h +++ b/src/util.h @@ -641,6 +641,18 @@ class ArrayBufferViewContents { bool was_detached_ = false; }; +// Creates a BackingStore with the contents of |data|. |deleter| runs once V8 +// no longer needs |data|: on release of the store when it can reference +// |data| directly, or before this returns when V8 requires backing stores +// inside its sandbox and |data| was copied. Returns nullptr if that copy +// could not be allocated; |deleter| has run in that case too. +std::unique_ptr AdoptIntoBackingStore( + v8::Isolate* isolate, + void* data, + size_t byte_length, + v8::BackingStore::DeleterCallback deleter, + void* deleter_data); + class Utf8Value : public MaybeStackBuffer { public: explicit Utf8Value(v8::Isolate* isolate, v8::Local value); diff --git a/test/cctest/test_dataqueue.cc b/test/cctest/test_dataqueue.cc index 7c75bf9bfc42..785c6b906ba6 100644 --- a/test/cctest/test_dataqueue.cc +++ b/test/cctest/test_dataqueue.cc @@ -10,12 +10,27 @@ using node::DataQueue; using v8::ArrayBuffer; using v8::BackingStore; +// Backing stores must lie inside V8's sandbox when it is enabled, so copy +// the test data into memory from the default allocator rather than wrap it. +static std::shared_ptr MakeStore(const char* data) { + static ArrayBuffer::Allocator* const allocator = + ArrayBuffer::Allocator::NewDefaultAllocator(); + size_t len = strlen(data); + void* copy = allocator->AllocateUninitialized(len); + CHECK_NOT_NULL(copy); + memcpy(copy, data, len); + return ArrayBuffer::NewBackingStore( + copy, + len, + [](void* p, size_t n, void*) { allocator->Free(p, n); }, + nullptr); +} + TEST(DataQueue, InMemoryEntry) { char buffer[] = "hello world"; size_t len = strlen(buffer); - std::shared_ptr store = ArrayBuffer::NewBackingStore( - &buffer, len, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store = MakeStore(buffer); // We can create an InMemoryEntry from a v8::BackingStore. std::unique_ptr entry = @@ -87,11 +102,9 @@ TEST(DataQueue, IdempotentDataQueue) { size_t len2 = strlen(buffer2); size_t len3 = strlen(buffer3); - std::shared_ptr store1 = ArrayBuffer::NewBackingStore( - &buffer1, len1, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store1 = MakeStore(buffer1); - std::shared_ptr store2 = ArrayBuffer::NewBackingStore( - &buffer2, len2, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store2 = MakeStore(buffer2); std::vector> list; list.push_back( @@ -124,8 +137,7 @@ TEST(DataQueue, IdempotentDataQueue) { // The size is known to be the sum of the in memory-entries. CHECK_EQ(data_queue->size().value(), len1 + len2); - std::shared_ptr store3 = ArrayBuffer::NewBackingStore( - &buffer3, len3, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store3 = MakeStore(buffer3); // Trying to append a new entry does not crash, but returns std::nullopt. CHECK(!data_queue @@ -416,14 +428,11 @@ TEST(DataQueue, NonIdempotentDataQueue) { size_t len2 = strlen(buffer2); size_t len3 = strlen(buffer3); - std::shared_ptr store1 = ArrayBuffer::NewBackingStore( - &buffer1, len1, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store1 = MakeStore(buffer1); - std::shared_ptr store2 = ArrayBuffer::NewBackingStore( - &buffer2, len2, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store2 = MakeStore(buffer2); - std::shared_ptr store3 = ArrayBuffer::NewBackingStore( - &buffer3, len3, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store3 = MakeStore(buffer3); // We can create an non-idempotent DataQueue from a list of entries. std::shared_ptr data_queue = DataQueue::Create(); @@ -550,11 +559,9 @@ TEST(DataQueue, DataQueueEntry) { size_t len1 = strlen(buffer1); size_t len2 = strlen(buffer2); - std::shared_ptr store1 = ArrayBuffer::NewBackingStore( - &buffer1, len1, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store1 = MakeStore(buffer1); - std::shared_ptr store2 = ArrayBuffer::NewBackingStore( - &buffer2, len2, [](void*, size_t, void*) {}, nullptr); + std::shared_ptr store2 = MakeStore(buffer2); std::vector> list; list.push_back( diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index fb1bcc2ef90c..6768f76296e3 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -492,7 +492,11 @@ TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) { } CHECK_EQ(callback_calls, 1); +#ifdef V8_ENABLE_SANDBOX + CHECK_EQ(ab->ByteLength(), sizeof(hello)); +#else CHECK_EQ(ab->ByteLength(), 0); +#endif } #if HAVE_INSPECTOR diff --git a/test/cctest/test_linked_binding.cc b/test/cctest/test_linked_binding.cc index 10507950becf..be097abd90bb 100644 --- a/test/cctest/test_linked_binding.cc +++ b/test/cctest/test_linked_binding.cc @@ -250,8 +250,8 @@ napi_value NapiLinkedWithInstanceData(napi_env env, napi_value exports) { napi_value key, value; CHECK_EQ(napi_create_string_utf8(env, "hello", NAPI_AUTO_LENGTH, &key), napi_ok); - CHECK_EQ(napi_create_external_arraybuffer( - env, instance_data, 1, nullptr, nullptr, &value), + CHECK_EQ(napi_create_bigint_uint64( + env, reinterpret_cast(instance_data), &value), napi_ok); CHECK_EQ(napi_set_property(env, exports, key, value), napi_ok); return nullptr; @@ -290,9 +290,9 @@ TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiInstanceDataTest) { .ToLocalChecked(); v8::Local completion_value = script->Run(context).ToLocalChecked(); - CHECK(completion_value->IsArrayBuffer()); - instance_data = - static_cast(completion_value.As()->Data()); + CHECK(completion_value->IsBigInt()); + instance_data = reinterpret_cast( + completion_value.As()->Uint64Value()); CHECK_NE(instance_data, nullptr); CHECK_EQ(*instance_data, 0); } @@ -328,9 +328,9 @@ TEST_F(LinkedBindingTest, .ToLocalChecked(); v8::Local completion_value = script->Run(context).ToLocalChecked(); - CHECK(completion_value->IsArrayBuffer()); - instance_data = - static_cast(completion_value.As()->Data()); + CHECK(completion_value->IsBigInt()); + instance_data = reinterpret_cast( + completion_value.As()->Uint64Value()); CHECK_NE(instance_data, nullptr); CHECK_EQ(*instance_data, 0); } diff --git a/test/common/README.md b/test/common/README.md index 6c8fcf42847a..a6e290f2907f 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -257,6 +257,13 @@ Indicates if [internationalization][] is supported. Indicates whether `IPv6` is supported on this platform. +### `hasV8Sandbox` + +* [\][] + +Indicates whether V8 was built with its sandbox enabled, in which case +`ArrayBuffer`s cannot reference memory outside of it. + ### `hasSQLite` * [\][] diff --git a/test/common/index.js b/test/common/index.js index e37b354f8259..741e08445abe 100755 --- a/test/common/index.js +++ b/test/common/index.js @@ -36,6 +36,7 @@ const bits = ['arm64', 'loong64', 'mips', 'mipsel', 'ppc64', 'riscv64', 's390x', .includes(process.arch) ? 64 : 32; const hasIntl = !!process.config.variables.v8_enable_i18n_support; const hasTemporal = !!process.config.variables.v8_enable_temporal_support; +const hasV8Sandbox = !!process.config.variables.v8_enable_sandbox; // small-icu doesn't support non-English locales const hasFullICU = (() => { @@ -1018,6 +1019,7 @@ const common = { getTTYfd, hasIntl, hasTemporal, + hasV8Sandbox, hasFullICU, hasCrypto, hasDtls, diff --git a/test/common/index.mjs b/test/common/index.mjs index 108cae290999..9f81a2b920ee 100644 --- a/test/common/index.mjs +++ b/test/common/index.mjs @@ -24,6 +24,7 @@ const { hasLocalStorage, hasIntl, hasTemporal, + hasV8Sandbox, hasIPv6, isAIX, isAlive, @@ -83,6 +84,7 @@ export { hasLocalStorage, hasIntl, hasTemporal, + hasV8Sandbox, hasIPv6, isAIX, isAlive, diff --git a/test/ffi/test-ffi-memory.js b/test/ffi/test-ffi-memory.js index 72d37efacd5a..f092782e5b19 100644 --- a/test/ffi/test-ffi-memory.js +++ b/test/ffi/test-ffi-memory.js @@ -77,7 +77,9 @@ test('ffi supports unaligned memory access', () => { })); }); -test('ffi toBuffer supports copy and zero-copy views', () => { +const zeroCopy = { skip: common.hasV8Sandbox && 'zero-copy views are unavailable with the V8 sandbox' }; + +test('ffi toBuffer supports copy and zero-copy views', zeroCopy, () => { withAllocations(common.mustCall((alloc) => { const ptr = alloc(8); ffi.exportBuffer(Buffer.from([1, 2, 3, 4]), ptr, 4); @@ -99,7 +101,7 @@ test('ffi toBuffer supports copy and zero-copy views', () => { })); }); -test('ffi toArrayBuffer supports copy and zero-copy views', () => { +test('ffi toArrayBuffer supports copy and zero-copy views', zeroCopy, () => { withAllocations(common.mustCall((alloc) => { const ptr = alloc(4); ffi.exportBuffer(Buffer.from([10, 20, 30, 40]), ptr, 4); @@ -121,6 +123,16 @@ test('ffi toArrayBuffer supports copy and zero-copy views', () => { })); }); +test('ffi zero-copy views throw with the V8 sandbox', { skip: !common.hasV8Sandbox }, () => { + withAllocations(common.mustCall((alloc) => { + const ptr = alloc(4); + ffi.exportBuffer(Buffer.from([1, 2, 3, 4]), ptr, 4); + assert.deepStrictEqual([...ffi.toBuffer(ptr, 4)], [1, 2, 3, 4]); + assert.throws(() => ffi.toBuffer(ptr, 4, false), { code: 'ERR_OPERATION_FAILED' }); + assert.throws(() => ffi.toArrayBuffer(ptr, 4, false), { code: 'ERR_OPERATION_FAILED' }); + })); +}); + test('ffi getRawPointer returns raw addresses for byte sources', () => { const buffer = Buffer.from([1, 2, 3]); const arrayBuffer = new Uint8Array([4, 5, 6, 7]).buffer; diff --git a/test/parallel/test-crypto-secure-heap.js b/test/parallel/test-crypto-secure-heap.js index 8bd93c5281da..58bacbc3f071 100644 --- a/test/parallel/test-crypto-secure-heap.js +++ b/test/parallel/test-crypto-secure-heap.js @@ -13,6 +13,10 @@ if (common.isASan) { common.skip('ASan does not play well with secure heap allocations'); } +if (common.hasV8Sandbox) { + common.skip('--secure-heap is not available with the V8 sandbox'); +} + if (process.features.openssl_is_boringssl) { common.skip('BoringSSL does not support secure heap'); } diff --git a/test/parallel/test-trace-events-get-category-enabled-buffer.js b/test/parallel/test-trace-events-get-category-enabled-buffer.js index 79d2a1cf30da..929f1ae3f2a8 100644 --- a/test/parallel/test-trace-events-get-category-enabled-buffer.js +++ b/test/parallel/test-trace-events-get-category-enabled-buffer.js @@ -15,31 +15,30 @@ common.skipIfPerfettoEnabled(); const { createTracing, getEnabledCategories } = require('trace_events'); const assert = require('assert'); -const binding = require('internal/test/binding'); -const getCategoryEnabledBuffer = binding.internalBinding('trace_events').getCategoryEnabledBuffer; +const { categoryEnabledChecker } = require('internal/trace_events'); it('should track enabled/disabled categories', () => { const random = Math.random().toString().slice(2); const category = `node.${random}`; - const buffer = getCategoryEnabledBuffer(category); + const isEnabled = categoryEnabledChecker(category); const tracing = createTracing({ categories: [category], }); - assert.ok(buffer[0] === 0, `the buffer[0] should start with value 0, got: ${buffer[0]}`); + assert.strictEqual(isEnabled(), false); tracing.enable(); let currentCategories = getEnabledCategories(); assert.ok(currentCategories.includes(category), `the getEnabledCategories should include ${category}, got: ${currentCategories}`); - assert.ok(buffer[0] > 0, `the buffer[0] should be greater than 0, got: ${buffer[0]}`); + assert.strictEqual(isEnabled(), true); tracing.disable(); currentCategories = getEnabledCategories(); assert.ok(currentCategories === undefined, `the getEnabledCategories should return undefined, got: ${currentCategories}`); - assert.ok(buffer[0] === 0, `the buffer[0] should be 0, got: ${buffer[0]}`); + assert.strictEqual(isEnabled(), false); }); From 965693dfac8a21e8aba76761d99ea791d8dd8ccd Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 21 Aug 2026 12:42:35 +0000 Subject: [PATCH 2/2] build: enable the V8 sandbox in shared-cage builds V8 defaults `v8_enable_sandbox` to on whenever the shared pointer compression cage and the external code space are enabled, and that is the configuration embedders that use the sandbox build with. Now that the sandbox builds and passes the tests, follow that default for `--experimental-pointer-compression-shared-cage` so the configuration is reachable from `configure`. Multi-cage pointer compression builds stay without it: there every IsolateGroup gets its own sandbox, and `NodeArrayBufferAllocator` always allocates from the default one. Signed-off-by: Shelley Vohr --- configure.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/configure.py b/configure.py index 3f4f9984d5a8..4ac2af3d796f 100755 --- a/configure.py +++ b/configure.py @@ -834,7 +834,7 @@ action='store_true', dest='pointer_compression_shared_cage', default=None, - help='[Experimental] Use V8 pointer compression with shared cage (requires --experimental-enable-pointer-compression)') + help='[Experimental] Use V8 pointer compression with a shared cage and enable the V8 sandbox (requires --experimental-enable-pointer-compression)') parser.add_argument('--v8-options', action='store', @@ -2201,16 +2201,10 @@ def configure_v8(o, configs): flavor not in ('aix', 'os400', 'zos') and o['variables']['target_arch'] in maglev_enabled_architectures) o['variables']['v8_enable_pointer_compression'] = 1 if options.enable_pointer_compression else 0 - # Using the sandbox requires always allocating array buffer backing stores in the sandbox. - # We currently have many backing stores tied to pointers from C++ land that are not - # even necessarily dynamic (e.g. in static storage) for fast communication between JS and C++. - # Until we manage to get rid of all those, v8_enable_sandbox cannot be used. - # Note that enabling pointer compression without enabling sandbox is unsupported by V8, - # so this can be broken at any time. - o['variables']['v8_enable_sandbox'] = 0 - # We set v8_enable_pointer_compression_shared_cage to 0 always, even when - # pointer compression is enabled so that we don't accidentally enable shared - # cage mode when pointer compression is on. + # Like V8's own default, the sandbox goes with the shared pointer compression + # cage. Multi-cage builds give every IsolateGroup its own sandbox, which the + # array buffer allocator does not know about yet. + o['variables']['v8_enable_sandbox'] = 1 if options.pointer_compression_shared_cage else 0 o['variables']['v8_enable_pointer_compression_shared_cage'] = 1 if options.pointer_compression_shared_cage else 0 o['variables']['v8_enable_external_code_space'] = 1 if options.enable_pointer_compression else 0 o['variables']['v8_enable_31bit_smis_on_64bit_arch'] = 1 if options.enable_pointer_compression else 0