test: dtls test improvements

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Claude/Opus

Author: jasnell

src/dtls/dtls.h

@@ -58,16 +58,18 @@ void RecordTimestampStat(Stats* stats) {
   V(MESSAGES_SENT, messages_sent)                                              \
   V(RETRANSMIT_COUNT, retransmit_count)
 
-// State indices shared between C++ and JS via AliasedStruct/DataView.
+// State "indices" shared between C++ and JS via AliasedStruct/DataView. These
+// are BYTE OFFSETS into the state struct, not sequential indices: session_count
+// is a uint32, so `busy`, which follows it, sits at byte offset 8. The
+// static_asserts in dtls_endpoint.cc pin these to the actual struct layout.
 // Keep in sync with lib/internal/dtls/state.js.
 enum DTLSEndpointStateIndex {
   IDX_ENDPOINT_STATE_BOUND = 0,
-  IDX_ENDPOINT_STATE_LISTENING,
-  IDX_ENDPOINT_STATE_CLOSING,
-  IDX_ENDPOINT_STATE_DESTROYED,
-  IDX_ENDPOINT_STATE_SESSION_COUNT,
-  IDX_ENDPOINT_STATE_BUSY,
-  IDX_ENDPOINT_STATE_COUNT
+  IDX_ENDPOINT_STATE_LISTENING = 1,
+  IDX_ENDPOINT_STATE_CLOSING = 2,
+  IDX_ENDPOINT_STATE_DESTROYED = 3,
+  IDX_ENDPOINT_STATE_SESSION_COUNT = 4,
+  IDX_ENDPOINT_STATE_BUSY = 8,
 };
 
 enum DTLSSessionStateIndex {

src/dtls/dtls_endpoint.cc

@@ -19,6 +19,7 @@
 #include <openssl/err.h>
 #include <openssl/ssl.h>
 
+#include <cstddef>
 #include <cstring>
 
 namespace node {
@@ -45,6 +46,22 @@ struct SendReq {
 };
 }  // namespace
 
+// The endpoint state "indices" are byte offsets into DTLSEndpointStateData,
+// accessed from JS via a DataView. Pin them to the actual struct layout so a
+// mismatch (as once existed for `busy`, which follows a uint32) can't recur.
+static_assert(IDX_ENDPOINT_STATE_BOUND ==
+              offsetof(DTLSEndpointStateData, bound));
+static_assert(IDX_ENDPOINT_STATE_LISTENING ==
+              offsetof(DTLSEndpointStateData, listening));
+static_assert(IDX_ENDPOINT_STATE_CLOSING ==
+              offsetof(DTLSEndpointStateData, closing));
+static_assert(IDX_ENDPOINT_STATE_DESTROYED ==
+              offsetof(DTLSEndpointStateData, destroyed));
+static_assert(IDX_ENDPOINT_STATE_SESSION_COUNT ==
+              offsetof(DTLSEndpointStateData, session_count));
+static_assert(IDX_ENDPOINT_STATE_BUSY ==
+              offsetof(DTLSEndpointStateData, busy));
+
 DTLSEndpoint::DTLSEndpoint(Environment* env, Local<Object> wrap)
     : HandleWrap(env,
                  wrap,
@@ -268,6 +285,11 @@ void DTLSEndpoint::CloseGracefully() {
 
   server_context_.reset();
 
+  // Keep ourselves alive until OnClose() runs, so a garbage collection while
+  // uv_close() is in flight cannot collect the wrapper before the close is
+  // reported. Released in OnClose().
+  self_ref_ = BaseObjectPtr<DTLSEndpoint>(this);
+
   // HandleWrap::Close() calls uv_close and manages the lifecycle.
   HandleWrap::Close();
 }
@@ -293,6 +315,9 @@ void DTLSEndpoint::Destroy() {
     state_->listening = 0;
   }
 
+  // Keep ourselves alive until OnClose() (see CloseGracefully()).
+  self_ref_ = BaseObjectPtr<DTLSEndpoint>(this);
+
   HandleWrap::Close();
 }
 
@@ -402,6 +427,17 @@ void DTLSEndpoint::OnClose() {
   state_->destroyed = 1;
   DTLS_STAT_RECORD_TIMESTAMP(DTLSEndpointStats, destroyed_at);
 
+  // Release the strong self-reference taken when the close was initiated.
+  // HandleWrap::OnClose still holds its own reference for the duration of this
+  // call, so this does not free us here.
+  self_ref_.reset();
+
+  // A close initiated outside CloseGracefully()/Destroy() (e.g. an endpoint
+  // abandoned mid-construction and closed at environment teardown) takes no
+  // self-reference, so its wrapper may already be collected. There is no JS
+  // side to notify in that case; skip it rather than touch a freed wrapper.
+  if (persistent().IsEmpty()) return;
+
   Local<Function> cb = GetCallback(DTLS_CB_ENDPOINT_CLOSE);
   if (!cb.IsEmpty()) {
     Local<Value> argv[] = {};

src/dtls/dtls_endpoint.h

@@ -156,6 +156,11 @@ class DTLSEndpoint final : public HandleWrap {
   AliasedStruct<DTLSEndpointStateData> state_;
   AliasedStruct<DTLSEndpointStats> stats_;
 
+  // Strong self-reference held while a graceful close/destroy is in flight, so
+  // the wrapper is not garbage-collected before OnClose() runs and reports the
+  // close. Cleared in OnClose().
+  BaseObjectPtr<DTLSEndpoint> self_ref_;
+
   bool listening_ = false;
   uint32_t mtu_ = 1200;  // Conservative default MTU for data payload
 };

test/parallel/test-dtls-accessors.mjs

@@ -0,0 +1,104 @@
+// Flags: --experimental-dtls --no-warnings
+
+// Test: DTLSEndpoint/DTLSSession state fields and callback accessors reflect
+// what is set and the connection lifecycle.
+
+import {
+  hasCrypto, skip, mustCall, mustNotCall, mustCallAtLeast,
+} from '../common/index.mjs';
+import assert from 'node:assert';
+import * as fixtures from '../common/fixtures.mjs';
+
+const { strictEqual } = assert;
+const { readKey } = fixtures;
+
+if (!hasCrypto) {
+  skip('missing crypto');
+}
+
+if (!process.features.dtls) {
+  skip('DTLS is not enabled');
+}
+
+const { listen, connect } = await import('node:dtls');
+
+const cert = readKey('agent1-cert.pem').toString();
+const key = readKey('agent1-key.pem').toString();
+const ca = readKey('ca1-cert.pem').toString();
+
+const gotServerSession = Promise.withResolvers();
+
+const server = listen(mustCall((session) => {
+  gotServerSession.resolve(session);
+}), { cert, key, port: 0, host: '127.0.0.1' });
+
+// --- Endpoint state after listen(): bound and listening. ---
+const es = server.state;
+strictEqual(es.bound, true);
+strictEqual(es.listening, true);
+strictEqual(es.closing, false);
+strictEqual(es.destroyed, false);
+strictEqual(es.sessionCount, 0);
+
+// The busy property is settable via the endpoint and reflected in the state view.
+strictEqual(server.busy, false);
+strictEqual(es.busy, false);
+server.busy = true;
+strictEqual(server.busy, true);
+strictEqual(es.busy, true);
+server.busy = false;
+strictEqual(es.busy, false);
+
+// --- Endpoint onerror accessor. ---
+strictEqual(server.onerror, undefined);
+const onEndpointError = mustNotCall();
+server.onerror = onEndpointError;
+strictEqual(server.onerror, onEndpointError);
+
+const client = connect('127.0.0.1', server.address.port, {
+  ca: [ca],
+  rejectUnauthorized: false,
+});
+
+// --- Session state during the handshake. ---
+const cs = client.state;
+strictEqual(cs.handshaking, true);
+strictEqual(cs.open, false);
+strictEqual(cs.closing, false);
+strictEqual(cs.destroyed, false);
+strictEqual(cs.hasMessageListener, false);
+
+// --- Session callback accessors: unset, then set. ---
+strictEqual(client.onmessage, undefined);
+strictEqual(client.onerror, undefined);
+strictEqual(client.onhandshake, undefined);
+strictEqual(client.onkeylog, undefined);
+// A connect() session owns its internal endpoint.
+strictEqual(client.ownsEndpoint, true);
+
+client.onmessage = mustNotCall();
+strictEqual(typeof client.onmessage, 'function');
+// Attaching a message listener flips the shared flag.
+strictEqual(cs.hasMessageListener, true);
+
+client.onerror = mustNotCall();
+strictEqual(typeof client.onerror, 'function');
+
+client.onhandshake = mustCall();
+strictEqual(typeof client.onhandshake, 'function');
+
+client.onkeylog = mustCallAtLeast();
+strictEqual(typeof client.onkeylog, 'function');
+
+await client.opened;
+
+// --- Session state after the handshake completes. ---
+strictEqual(cs.handshaking, false);
+strictEqual(cs.open, true);
+
+const serverSession = await gotServerSession.promise;
+await serverSession.opened;
+strictEqual(es.sessionCount, 1);
+
+await client.close();
+await server.close();

test/parallel/test-dtls-alpn.mjs

@@ -53,3 +53,33 @@ await serverAlpnChecked.promise;
 
 await session.close();
 await endpoint.close();
+
+// ALPN with no protocol in common: the handshake still completes and neither
+// peer reports a negotiated protocol.
+{
+  const gotServerSession = Promise.withResolvers();
+
+  const server = listen(mustCall((s) => gotServerSession.resolve(s)), {
+    cert: serverCert.toString(),
+    key: serverKey.toString(),
+    port: 0,
+    host: '127.0.0.1',
+    alpn: ['bar'],
+  });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    ca: [ca.toString()],
+    rejectUnauthorized: false,
+    alpn: ['foo'],
+  });
+
+  await client.opened;
+  const serverSession = await gotServerSession.promise;
+  await serverSession.opened;
+
+  strictEqual(client.alpnProtocol, undefined);
+  strictEqual(serverSession.alpnProtocol, undefined);
+
+  await client.close();
+  await server.close();
+}

test/parallel/test-dtls-ciphers.mjs

@@ -0,0 +1,79 @@
+// Flags: --experimental-dtls --no-warnings
+
+// Test: cipher and ECDH-curve selection and validation.
+
+import { hasCrypto, skip, mustCall, mustNotCall } from '../common/index.mjs';
+import assert from 'node:assert';
+import * as fixtures from '../common/fixtures.mjs';
+
+const { strictEqual, throws } = assert;
+const { readKey } = fixtures;
+
+if (!hasCrypto) {
+  skip('missing crypto');
+}
+
+if (!process.features.dtls) {
+  skip('DTLS is not enabled');
+}
+
+const { listen, connect } = await import('node:dtls');
+
+const cert = readKey('agent1-cert.pem').toString();
+const key = readKey('agent1-key.pem').toString();
+const ca = readKey('ca1-cert.pem').toString();
+
+const CIPHER = 'ECDHE-RSA-AES128-GCM-SHA256';
+
+// Case 1: a specific cipher is negotiated and reported on both peers.
+{
+  const gotServerSession = Promise.withResolvers();
+
+  const server = listen(mustCall((session) => {
+    gotServerSession.resolve(session);
+  }), { cert, key, port: 0, host: '127.0.0.1', ciphers: CIPHER });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    ca: [ca],
+    rejectUnauthorized: false,
+    ciphers: CIPHER,
+  });
+
+  await client.opened;
+  const serverSession = await gotServerSession.promise;
+  await serverSession.opened;
+
+  strictEqual(client.cipher.name, CIPHER);
+  strictEqual(serverSession.cipher.name, CIPHER);
+
+  await client.close();
+  await server.close();
+}
+
+// Case 2: an invalid cipher list is rejected.
+throws(() => listen(mustNotCall(), {
+  cert, key, port: 0, host: '127.0.0.1', ciphers: 'THIS-IS-NOT-A-CIPHER',
+}), { code: 'ERR_CRYPTO_OPERATION_FAILED' });
+
+// Case 3: a valid ECDH curve completes a handshake.
+{
+  const server = listen(mustCall(), {
+    cert, key, port: 0, host: '127.0.0.1', ecdhCurve: 'P-256',
+  });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    ca: [ca],
+    rejectUnauthorized: false,
+    ecdhCurve: 'P-256',
+  });
+
+  await client.opened;
+
+  await client.close();
+  await server.close();
+}
+
+// Case 4: an invalid ECDH curve is rejected.
+throws(() => listen(mustNotCall(), {
+  cert, key, port: 0, host: '127.0.0.1', ecdhCurve: 'not-a-curve',
+}), { code: 'ERR_CRYPTO_OPERATION_FAILED' });