From f099b3493601524271df8c97568484b2862a97c3 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 18 May 2025 11:44:35 -0700 Subject: [PATCH] src: make a number of minor improvements to buffer When using Buffer::New with an externally allocated buffer and v8 sandbox enabled, defer to copy instead of wrapping the buffer. Also, when immediately memcpy'ing into the full buffer, allocate the buffer with kUninitialized as a slight perf optimization. --- src/crypto/crypto_cipher.cc | 15 ++++++++++++--- src/crypto/crypto_sig.cc | 5 ++++- src/node_blob.cc | 15 +++++++++------ src/node_buffer.cc | 17 +++++++++++++++-- src/node_http2.cc | 5 ++++- src/node_sqlite.cc | 4 +++- src/quic/streams.cc | 4 +++- src/stream_base.cc | 3 ++- src/udp_wrap.cc | 4 +++- 9 files changed, 55 insertions(+), 17 deletions(-) diff --git a/src/crypto/crypto_cipher.cc b/src/crypto/crypto_cipher.cc index a2f4493e1b8ec8..40c091d4ce2d29 100644 --- a/src/crypto/crypto_cipher.cc +++ b/src/crypto/crypto_cipher.cc @@ -708,7 +708,10 @@ CipherBase::UpdateResult CipherBase::Update( *out = ArrayBuffer::NewBackingStore(env()->isolate(), 0); } else if (static_cast(buf_len) != (*out)->ByteLength()) { std::unique_ptr old_out = std::move(*out); - *out = ArrayBuffer::NewBackingStore(env()->isolate(), buf_len); + *out = ArrayBuffer::NewBackingStore( + env()->isolate(), + buf_len, + BackingStoreInitializationMode::kUninitialized); memcpy((*out)->Data(), old_out->Data(), buf_len); } @@ -804,7 +807,10 @@ bool CipherBase::Final(std::unique_ptr* out) { *out = ArrayBuffer::NewBackingStore(env()->isolate(), 0); } else if (static_cast(out_len) != (*out)->ByteLength()) { std::unique_ptr old_out = std::move(*out); - *out = ArrayBuffer::NewBackingStore(env()->isolate(), out_len); + *out = ArrayBuffer::NewBackingStore( + env()->isolate(), + out_len, + BackingStoreInitializationMode::kUninitialized); memcpy((*out)->Data(), old_out->Data(), out_len); } @@ -880,7 +886,10 @@ bool PublicKeyCipher::Cipher( if (buf.size() == 0) { *out = ArrayBuffer::NewBackingStore(env->isolate(), 0); } else { - *out = ArrayBuffer::NewBackingStore(env->isolate(), buf.size()); + *out = ArrayBuffer::NewBackingStore( + env->isolate(), + buf.size(), + BackingStoreInitializationMode::kUninitialized); memcpy((*out)->Data(), buf.get(), buf.size()); } diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc index 8518bdfd53b2d0..11007fbaeb862f 100644 --- a/src/crypto/crypto_sig.cc +++ b/src/crypto/crypto_sig.cc @@ -99,7 +99,10 @@ std::unique_ptr Node_SignFinal(Environment* env, [[likely]] { CHECK_LE(sig_buf.len, sig->ByteLength()); if (sig_buf.len < sig->ByteLength()) { - auto new_sig = ArrayBuffer::NewBackingStore(env->isolate(), sig_buf.len); + auto new_sig = ArrayBuffer::NewBackingStore( + env->isolate(), + sig_buf.len, + BackingStoreInitializationMode::kUninitialized); if (sig_buf.len > 0) [[likely]] { memcpy(new_sig->Data(), sig->Data(), sig_buf.len); } diff --git a/src/node_blob.cc b/src/node_blob.cc index d887485b9deed8..e8c0795fcaac60 100644 --- a/src/node_blob.cc +++ b/src/node_blob.cc @@ -21,6 +21,7 @@ using v8::Array; using v8::ArrayBuffer; using v8::ArrayBufferView; using v8::BackingStore; +using v8::BackingStoreInitializationMode; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; @@ -82,8 +83,8 @@ void Concat(const FunctionCallbackInfo& args) { } } - std::shared_ptr store = - ArrayBuffer::NewBackingStore(env->isolate(), total); + std::shared_ptr store = ArrayBuffer::NewBackingStore( + env->isolate(), total, BackingStoreInitializationMode::kUninitialized); uint8_t* ptr = static_cast(store->Data()); for (size_t n = 0; n < views.size(); n++) { uint8_t* from = @@ -210,8 +211,8 @@ void Blob::New(const FunctionCallbackInfo& args) { } // If the ArrayBuffer is not detachable, we will copy from it instead. - std::shared_ptr store = - ArrayBuffer::NewBackingStore(isolate, byte_length); + std::shared_ptr store = ArrayBuffer::NewBackingStore( + isolate, byte_length, BackingStoreInitializationMode::kUninitialized); uint8_t* ptr = static_cast(buf->Data()) + byte_offset; std::copy(ptr, ptr + byte_length, static_cast(store->Data())); return DataQueue::CreateInMemoryEntryFromBackingStore( @@ -375,8 +376,10 @@ void Blob::Reader::Pull(const FunctionCallbackInfo& args) { size_t total = 0; for (size_t n = 0; n < count; n++) total += vecs[n].len; - std::shared_ptr store = - ArrayBuffer::NewBackingStore(env->isolate(), total); + std::shared_ptr store = ArrayBuffer::NewBackingStore( + env->isolate(), + total, + BackingStoreInitializationMode::kUninitialized); auto ptr = static_cast(store->Data()); for (size_t n = 0; n < count; n++) { std::copy(vecs[n].base, vecs[n].base + vecs[n].len, ptr); diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 1303cb57f0b7ff..8ec282107e1d99 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -329,7 +329,8 @@ MaybeLocal New(Isolate* isolate, if (actual > 0) [[likely]] { if (actual < length) { std::unique_ptr old_store = std::move(store); - store = ArrayBuffer::NewBackingStore(isolate, actual); + store = ArrayBuffer::NewBackingStore( + isolate, actual, BackingStoreInitializationMode::kUninitialized); memcpy(store->Data(), old_store->Data(), actual); } Local buf = ArrayBuffer::New(isolate, std::move(store)); @@ -416,7 +417,7 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { CHECK(bs); - memcpy(bs->Data(), data, length); + if (length > 0) memcpy(bs->Data(), data, length); Local ab = ArrayBuffer::New(isolate, std::move(bs)); @@ -506,6 +507,17 @@ 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) { @@ -520,6 +532,7 @@ MaybeLocal New(Environment* env, if (Buffer::New(env, ab, 0, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); +#endif } namespace { diff --git a/src/node_http2.cc b/src/node_http2.cc index 4415ea096d0ea0..449ecdba807945 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2104,7 +2104,10 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { [[likely]] { // Shrink to the actual amount of used data. std::unique_ptr old_bs = std::move(bs); - bs = ArrayBuffer::NewBackingStore(env()->isolate(), nread); + bs = ArrayBuffer::NewBackingStore( + env()->isolate(), + nread, + BackingStoreInitializationMode::kUninitialized); memcpy(bs->Data(), old_bs->Data(), nread); } else { // This is a very unlikely case, and should only happen if the ReadStart() diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 0295ee4ff2f739..a58fc909c89f5b 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -19,6 +19,7 @@ namespace sqlite { using v8::Array; using v8::ArrayBuffer; +using v8::BackingStoreInitializationMode; using v8::BigInt; using v8::Boolean; using v8::ConstructorBehavior; @@ -103,7 +104,8 @@ using v8::Value; static_cast(sqlite3_##from##_bytes(__VA_ARGS__)); \ auto data = reinterpret_cast( \ sqlite3_##from##_blob(__VA_ARGS__)); \ - auto store = ArrayBuffer::NewBackingStore((isolate), size); \ + auto store = ArrayBuffer::NewBackingStore( \ + (isolate), size, BackingStoreInitializationMode::kUninitialized); \ memcpy(store->Data(), data, size); \ auto ab = ArrayBuffer::New((isolate), std::move(store)); \ (result) = Uint8Array::New(ab, 0, size); \ diff --git a/src/quic/streams.cc b/src/quic/streams.cc index 2e16b49f9bc40c..26ceadbfee0977 100644 --- a/src/quic/streams.cc +++ b/src/quic/streams.cc @@ -18,6 +18,7 @@ namespace node { using v8::Array; using v8::ArrayBuffer; using v8::ArrayBufferView; +using v8::BackingStoreInitializationMode; using v8::BigInt; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -1198,7 +1199,8 @@ void Stream::ReceiveData(const uint8_t* data, STAT_INCREMENT_N(Stats, bytes_received, len); STAT_RECORD_TIMESTAMP(Stats, received_at); - auto backing = ArrayBuffer::NewBackingStore(env()->isolate(), len); + auto backing = ArrayBuffer::NewBackingStore( + env()->isolate(), len, BackingStoreInitializationMode::kUninitialized); memcpy(backing->Data(), data, len); inbound_->append(DataQueue::CreateInMemoryEntryFromBackingStore( std::move(backing), 0, len)); diff --git a/src/stream_base.cc b/src/stream_base.cc index 0bf2642599ee91..566204e667c8e8 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -708,7 +708,8 @@ void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { CHECK_LE(static_cast(nread), bs->ByteLength()); if (static_cast(nread) != bs->ByteLength()) { std::unique_ptr old_bs = std::move(bs); - bs = ArrayBuffer::NewBackingStore(isolate, nread); + bs = ArrayBuffer::NewBackingStore( + isolate, nread, BackingStoreInitializationMode::kUninitialized); memcpy(bs->Data(), old_bs->Data(), nread); } diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 7cdb3340855423..e720aa20884181 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -35,6 +35,7 @@ using errors::TryCatchScope; using v8::Array; using v8::ArrayBuffer; using v8::BackingStore; +using v8::BackingStoreInitializationMode; using v8::Boolean; using v8::Context; using v8::DontDelete; @@ -759,7 +760,8 @@ void UDPWrap::OnRecv(ssize_t nread, } else if (static_cast(nread) != bs->ByteLength()) { CHECK_LE(static_cast(nread), bs->ByteLength()); std::unique_ptr old_bs = std::move(bs); - bs = ArrayBuffer::NewBackingStore(isolate, nread); + bs = ArrayBuffer::NewBackingStore( + isolate, nread, BackingStoreInitializationMode::kUninitialized); memcpy(bs->Data(), old_bs->Data(), nread); }