From 3c7cbefe644b80fd7493889486c7e6ec3d23a2cc Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 6 Aug 2020 16:45:37 -0700 Subject: [PATCH 1/3] quic: resolve minor TODO in QuicSocket --- src/quic/node_quic_socket.cc | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/quic/node_quic_socket.cc b/src/quic/node_quic_socket.cc index 0e054619df899a..cf7b128bb009c7 100644 --- a/src/quic/node_quic_socket.cc +++ b/src/quic/node_quic_socket.cc @@ -472,14 +472,7 @@ void QuicSocket::OnReceive( QuicCID dcid(pdcid, pdcidlen); QuicCID scid(pscid, pscidlen); - // TODO(@jasnell): It would be fantastic if Debug() could be - // modified to accept objects with a ToString-like capability - // similar to what we can do with TraceEvents... that would - // allow us to pass the QuicCID directly to Debug and have it - // converted to hex only if the category is enabled so we can - // skip committing resources here. - std::string dcid_hex = dcid.ToString(); - Debug(this, "Received a QUIC packet for dcid %s", dcid_hex.c_str()); + Debug(this, "Received a QUIC packet for dcid %s", dcid); BaseObjectPtr session = FindSession(dcid); @@ -489,7 +482,7 @@ void QuicSocket::OnReceive( // 3. The packet is a stateless reset sent by the peer // 4. This is a malicious or malformed packet. if (!session) { - Debug(this, "There is no existing session for dcid %s", dcid_hex.c_str()); + Debug(this, "There is no existing session for dcid %s", dcid); bool is_short_header = IsShortHeader(pversion, pscid, pscidlen); // Handle possible reception of a stateless reset token... From b82bf12cd2db41988ba2b35f3a762741291a9886 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 6 Aug 2020 17:00:31 -0700 Subject: [PATCH 2/3] quic: resolve some minor TODOs --- lib/internal/quic/core.js | 2 -- src/quic/node_quic_session.cc | 9 +++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 11515ebf5fa476..fd842b3e6803b5 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -517,7 +517,6 @@ function createSecureContext(options, init_cb) { const sc_options = validateCreateSecureContextOptions(options); const { groups, earlyData } = sc_options; const sc = _createSecureContext(sc_options); - // TODO(@jasnell): Determine if it's really necessary to pass in groups here. init_cb(sc.context, groups, earlyData); return sc; } @@ -2702,7 +2701,6 @@ class QuicStream extends Duplex { } [kInspect](depth, options) { - // TODO(@jasnell): Proper custom inspect implementation const direction = this.bidirectional ? 'bidirectional' : 'unidirectional'; const initiated = this.serverInitiated ? 'server' : 'client'; return customInspect(this, { diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index 5bffad8a4fa524..999dbd71fda689 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -3378,8 +3378,10 @@ int QuicSession::OnStreamReset( // sensitivity of PATH_CHALLENGE operations (an attacker // could use a compromised PATH_CHALLENGE to trick an endpoint // into redirecting traffic). -// TODO(@jasnell): In the future, we'll want to explore whether -// we want to handle the different cases of ngtcp2_rand_ctx +// +// The ngtcp2_rand_ctx tells us what the random data is used for. +// Currently, there is only one use. In the future, we'll want to +// explore whether we want to handle the different cases uses. int QuicSession::OnRand( ngtcp2_conn* conn, uint8_t* dest, @@ -3744,8 +3746,7 @@ void QuicSessionSilentClose(const FunctionCallbackInfo& args) { session->Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); } -// TODO(addaleax): This is a temporary solution for testing and should be -// removed later. +// This is used purely for testing. void QuicSessionRemoveFromSocket(const FunctionCallbackInfo& args) { QuicSession* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); From a893264c1b8d575d25e193aee7f637cae614f813 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 6 Aug 2020 17:04:24 -0700 Subject: [PATCH 3/3] quic: limit push check to http/3 --- lib/internal/quic/core.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index fd842b3e6803b5..14ad4e2d3c18f7 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -2942,14 +2942,14 @@ class QuicStream extends Duplex { validateObject(headers, 'headers'); - // Push streams are only supported on QUIC servers, and - // only if the original stream is bidirectional. - // TODO(@jasnell): This is really an http/3 specific - // requirement so if we end up later with another - // QUIC application protocol that has a similar - // notion of push streams without this restriction, - // then we'll need to check alpn value here also. - if (!this.clientInitiated && !this.bidirectional) { + // This is a small performance optimization for http/3, + // where push streams are only supported for client + // initiated, bidirectional streams. For any other alpn, + // we'll make the attempt to push and handle the failure + // after. + if (!this.clientInitiated && + !this.bidirectional && + this.session.alpnProtocol === 'h3-29') { throw new ERR_INVALID_STATE( 'Push streams are only supported on client-initiated, ' + 'bidirectional streams');