Skip to content

quic: copy options buffer instead of detaching #61403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nodejs-github-bot merged 1 commit into from
Jan 22, 2026
Merged

quic: copy options buffer instead of detaching #61403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/quic/data.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,29 @@ Maybe<Store> Store::From(Local<ArrayBufferView> view, Local<Value> detach_key) {
return Just(Store(std::move(backing), length, offset));
}

Store Store::CopyFrom(Local<ArrayBuffer> buffer) {
v8::Isolate* isolate = v8::Isolate::GetCurrent();
auto backing = buffer->GetBackingStore();
auto length = buffer->ByteLength();
auto dest = ArrayBuffer::NewBackingStore(
isolate, length, v8::BackingStoreInitializationMode::kUninitialized);
// copy content
memcpy(dest->Data(), backing->Data(), length);
return Store(std::move(dest), length, 0);
}

Store Store::CopyFrom(Local<ArrayBufferView> view) {
v8::Isolate* isolate = v8::Isolate::GetCurrent();
auto backing = view->Buffer()->GetBackingStore();
auto length = view->ByteLength();
auto offset = view->ByteOffset();
auto dest = ArrayBuffer::NewBackingStore(
isolate, length, v8::BackingStoreInitializationMode::kUninitialized);
Copy link
Member

@addaleax addaleax Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if we could handle allocation failures in these calls too

Copy link
Member Author

@legendecas legendecas Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I think we can use BackingStoreOnFailureMode to gracefully handle the failure. Given that there are many ArrayBuffer::NewBackingStore call sites in the codebase, I could file a follow-up to bulk update.

// copy content
memcpy(dest->Data(), static_cast<char*>(backing->Data()) + offset, length);
return Store(std::move(dest), length, 0);
}

Local<Uint8Array> Store::ToUint8Array(Environment* env) const {
return !store_
? Uint8Array::New(ArrayBuffer::New(env->isolate(), 0), 0, 0)
Expand Down
8 changes: 8 additions & 0 deletions src/quic/data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ class Store final : public MemoryRetainer {
v8::Local<v8::ArrayBufferView> view,
v8::Local<v8::Value> detach_key = v8::Local<v8::Value>());

// Creates a Store from the contents of an ArrayBuffer, always copying the
// content.
static Store CopyFrom(v8::Local<v8::ArrayBuffer> buffer);

// Creates a Store from the contents of an ArrayBufferView, always copying the
// content.
static Store CopyFrom(v8::Local<v8::ArrayBufferView> view);

v8::Local<v8::Uint8Array> ToUint8Array(Environment* env) const;
inline v8::Local<v8::Uint8Array> ToUint8Array(Realm* realm) const {
return ToUint8Array(realm->env());
Expand Down
5 changes: 1 addition & 4 deletions src/quic/endpoint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,7 @@ bool SetOption(Environment* env,
env, "The %s option must be an ArrayBufferView", nameStr);
return false;
}
Store store;
if (!Store::From(value.As<ArrayBufferView>()).To(&store)) {
return false;
}
Store store = Store::CopyFrom(value.As<ArrayBufferView>());
if (store.length() != TokenSecret::QUIC_TOKENSECRET_LEN) {
Utf8Value nameStr(env->isolate(), name);
THROW_ERR_INVALID_ARG_VALUE(
Expand Down
20 changes: 4 additions & 16 deletions src/quic/tlscontext.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,10 @@ bool SetOption(Environment* env,
}
} else if constexpr (std::is_same<T, Store>::value) {
if (item->IsArrayBufferView()) {
Store store;
if (!Store::From(item.As<v8::ArrayBufferView>()).To(&store)) {
return false;
}
Store store = Store::CopyFrom(item.As<v8::ArrayBufferView>());
(options->*member).push_back(std::move(store));
} else if (item->IsArrayBuffer()) {
Store store;
if (!Store::From(item.As<ArrayBuffer>()).To(&store)) {
return false;
}
Store store = Store::CopyFrom(item.As<ArrayBuffer>());
(options->*member).push_back(std::move(store));
} else {
Utf8Value namestr(env->isolate(), name);
Expand All @@ -168,16 +162,10 @@ bool SetOption(Environment* env,
}
} else if constexpr (std::is_same<T, Store>::value) {
if (value->IsArrayBufferView()) {
Store store;
if (!Store::From(value.As<v8::ArrayBufferView>()).To(&store)) {
return false;
}
Store store = Store::CopyFrom(value.As<v8::ArrayBufferView>());
(options->*member).push_back(std::move(store));
} else if (value->IsArrayBuffer()) {
Store store;
if (!Store::From(value.As<ArrayBuffer>()).To(&store)) {
return false;
}
Store store = Store::CopyFrom(value.As<ArrayBuffer>());
(options->*member).push_back(std::move(store));
} else {
Utf8Value namestr(env->isolate(), name);
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-quic-handshake-ipv6-only.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ const serverEndpoint = await listen(mustCall((serverSession) => {
},
ipv6Only: true,
} });
// Buffer is not detached.
assert.strictEqual(certs.buffer.detached, false);

// The server must have an address to connect to after listen resolves.
assert.ok(serverEndpoint.address !== undefined);
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-quic-handshake.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ const serverEndpoint = await listen(mustCall((serverSession) => {
}).then(mustCall());
}), { keys, certs });

// Buffer is not detached.
assert.strictEqual(certs.buffer.detached, false);

// The server must have an address to connect to after listen resolves.
assert.ok(serverEndpoint.address !== undefined);

Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-quic-internal-endpoint-listen-defaults.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,23 @@ assert.strictEqual(endpoint.address, undefined);
await assert.rejects(listen(123, { keys, certs, endpoint }), {
code: 'ERR_INVALID_ARG_TYPE',
});
// Buffer is not detached.
assert.strictEqual(certs.buffer.detached, false);

await assert.rejects(listen(() => {}, 123), {
code: 'ERR_INVALID_ARG_TYPE',
});

await listen(() => {}, { keys, certs, endpoint });
// Buffer is not detached.
assert.strictEqual(certs.buffer.detached, false);

await assert.rejects(listen(() => {}, { keys, certs, endpoint }), {
code: 'ERR_INVALID_STATE',
});
// Buffer is not detached.
assert.strictEqual(certs.buffer.detached, false);

assert.ok(state.isBound);
assert.ok(state.isReceiving);
assert.ok(state.isListening);
Expand All @@ -58,6 +66,9 @@ assert.ok(endpoint.destroyed);
await assert.rejects(listen(() => {}, { keys, certs, endpoint }), {
code: 'ERR_INVALID_STATE',
});
// Buffer is not detached.
assert.strictEqual(certs.buffer.detached, false);

assert.throws(() => { endpoint.busy = true; }, {
code: 'ERR_INVALID_STATE',
});
Expand Down
Loading