From d6034186d661ab69f33af81f8ce649375dafadd7 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 30 Jun 2020 09:15:31 -0700 Subject: [PATCH] quic: cleanups for QuicSocket PR-URL: https://github.com/nodejs/node/pull/34137 Reviewed-By: Anna Henningsen Reviewed-By: David Carlier --- src/env.h | 1 - src/quic/node_quic_session.cc | 2 +- src/quic/node_quic_socket-inl.h | 8 +-- src/quic/node_quic_socket.cc | 101 ++++++++++++++++++++------------ src/quic/node_quic_socket.h | 51 +++++++++------- 5 files changed, 96 insertions(+), 67 deletions(-) diff --git a/src/env.h b/src/env.h index 2add16c8eddaa7..0a25a0df4b80f0 100644 --- a/src/env.h +++ b/src/env.h @@ -450,7 +450,6 @@ constexpr size_t kFsStatsBufferLength = #if defined(NODE_EXPERIMENTAL_QUIC) && NODE_EXPERIMENTAL_QUIC # define QUIC_ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) \ V(quic_on_socket_close_function, v8::Function) \ - V(quic_on_socket_error_function, v8::Function) \ V(quic_on_socket_server_busy_function, v8::Function) \ V(quic_on_session_cert_function, v8::Function) \ V(quic_on_session_client_hello_function, v8::Function) \ diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index 027b5c9ae6abc4..60e1471c480ce9 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -2323,7 +2323,7 @@ bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) { socket->ReceiveStart(); // Step 4: Update ngtcp2 - auto& local_address = socket->local_address(); + auto local_address = socket->local_address(); if (nat_rebinding) { ngtcp2_addr addr; ngtcp2_addr_init( diff --git a/src/quic/node_quic_socket-inl.h b/src/quic/node_quic_socket-inl.h index 8156fd04ad7bf5..38f3ad927180f0 100644 --- a/src/quic/node_quic_socket-inl.h +++ b/src/quic/node_quic_socket-inl.h @@ -79,8 +79,8 @@ void QuicSocket::AssociateStatelessResetToken( token_map_[token] = session; } -const SocketAddress& QuicSocket::local_address() { - CHECK(preferred_endpoint_); +SocketAddress QuicSocket::local_address() const { + DCHECK(preferred_endpoint_); return preferred_endpoint_->local_address(); } @@ -221,10 +221,6 @@ void QuicSocket::AddEndpoint( endpoint_->ReceiveStart(); } -void QuicSocket::SessionReady(BaseObjectPtr session) { - listener_->OnSessionReady(session); -} - } // namespace quic } // namespace node diff --git a/src/quic/node_quic_socket.cc b/src/quic/node_quic_socket.cc index 6c341d529c16f2..e3112887d8e4fb 100644 --- a/src/quic/node_quic_socket.cc +++ b/src/quic/node_quic_socket.cc @@ -83,10 +83,10 @@ bool IsShortHeader( } } // namespace -QuicPacket::QuicPacket(const char* diagnostic_label, size_t len) : - data_{0}, - len_(len), - diagnostic_label_(diagnostic_label) { +QuicPacket::QuicPacket(const char* diagnostic_label, size_t len) + : data_{0}, + len_(len), + diagnostic_label_(diagnostic_label) { CHECK_LE(len, MAX_PKTLEN); } @@ -100,8 +100,6 @@ const char* QuicPacket::diagnostic_label() const { diagnostic_label_ : "unspecified"; } -void QuicPacket::MemoryInfo(MemoryTracker* tracker) const {} - QuicSocketListener::~QuicSocketListener() { if (socket_) socket_->RemoveListener(this); @@ -174,10 +172,10 @@ QuicEndpoint::QuicEndpoint( QuicState* quic_state, Local wrap, QuicSocket* listener, - Local udp_wrap) : - BaseObject(quic_state->env(), wrap), - listener_(listener), - quic_state_(quic_state) { + Local udp_wrap) + : BaseObject(quic_state->env(), wrap), + listener_(listener), + quic_state_(quic_state) { MakeWeak(); udp_ = static_cast( udp_wrap->GetAlignedPointerFromInternalField( @@ -187,7 +185,9 @@ QuicEndpoint::QuicEndpoint( strong_ptr_.reset(udp_->GetAsyncWrap()); } -void QuicEndpoint::MemoryInfo(MemoryTracker* tracker) const {} +QuicEndpoint::~QuicEndpoint() { + udp_->set_listener(nullptr); +} uv_buf_t QuicEndpoint::OnAlloc(size_t suggested_size) { return AllocatedBuffer::AllocateManaged(env(), suggested_size).release(); @@ -229,6 +229,14 @@ void QuicEndpoint::OnAfterBind() { listener_->OnBind(this); } +template +void QuicSocketStatsTraits::ToString(const QuicSocket& ptr, Fn&& add_field) { +#define V(_n, name, label) \ + add_field(label, ptr.GetStat(&QuicSocketStats::name)); + SOCKET_STATS(V) +#undef V +} + QuicSocket::QuicSocket( QuicState* quic_state, Local wrap, @@ -240,17 +248,17 @@ QuicSocket::QuicSocket( QlogMode qlog, const uint8_t* session_reset_secret, bool disable_stateless_reset) - : AsyncWrap(quic_state->env(), wrap, AsyncWrap::PROVIDER_QUICSOCKET), - StatsBase(quic_state->env(), wrap), - alloc_info_(MakeAllocator()), - options_(options), - max_connections_(max_connections), - max_connections_per_host_(max_connections_per_host), - max_stateless_resets_per_host_(max_stateless_resets_per_host), - retry_token_expiration_(retry_token_expiration), - qlog_(qlog), - server_alpn_(NGHTTP3_ALPN_H3), - quic_state_(quic_state) { + : AsyncWrap(quic_state->env(), wrap, AsyncWrap::PROVIDER_QUICSOCKET), + StatsBase(quic_state->env(), wrap), + alloc_info_(MakeAllocator()), + options_(options), + max_connections_(max_connections), + max_connections_per_host_(max_connections_per_host), + max_stateless_resets_per_host_(max_stateless_resets_per_host), + retry_token_expiration_(retry_token_expiration), + qlog_(qlog), + server_alpn_(NGHTTP3_ALPN_H3), + quic_state_(quic_state) { MakeWeak(); PushListener(&default_listener_); @@ -279,15 +287,13 @@ QuicSocket::~QuicSocket() { if (listener == listener_) RemoveListener(listener_); - DebugStats(); -} + // In a clean shutdown, all QuicSessions associated with the QuicSocket + // would have been destroyed explicitly. However, if the QuicSocket is + // garbage collected / freed before Destroy having been called, there + // may be sessions remaining. This is not really a good thing. + Debug(this, "Destroying with %d sessions remaining", sessions_.size()); -template -void QuicSocketStatsTraits::ToString(const QuicSocket& ptr, Fn&& add_field) { -#define V(_n, name, label) \ - add_field(label, ptr.GetStat(&QuicSocketStats::name)); - SOCKET_STATS(V) -#undef V + DebugStats(); } void QuicSocket::MemoryInfo(MemoryTracker* tracker) const { @@ -310,7 +316,6 @@ void QuicSocket::Listen( const std::string& alpn, uint32_t options) { CHECK(sc); - CHECK(!server_secure_context_); CHECK(!is_flag_set(QUICSOCKET_FLAGS_SERVER_LISTENING)); Debug(this, "Starting to listen"); server_session_config_.Set(quic_state(), preferred_address); @@ -323,6 +328,7 @@ void QuicSocket::Listen( } void QuicSocket::OnError(QuicEndpoint* endpoint, ssize_t error) { + // TODO(@jasnell): What should we do with the endpoint? Debug(this, "Reading data from UDP socket failed. Error %" PRId64, error); listener_->OnError(error); } @@ -341,7 +347,7 @@ void QuicSocket::OnEndpointDone(QuicEndpoint* endpoint) { } void QuicSocket::OnBind(QuicEndpoint* endpoint) { - const SocketAddress& local_address = endpoint->local_address(); + SocketAddress local_address = endpoint->local_address(); bound_endpoints_[local_address] = BaseObjectWeakPtr(endpoint); Debug(this, "Endpoint %s bound", local_address); @@ -545,6 +551,13 @@ void QuicSocket::OnReceive( IncrementStat(&QuicSocketStats::packets_ignored); return; } + + // The QuicSession was destroyed while it was being set up. There's + // no further processing we can do here. + if (session->is_destroyed()) { + IncrementStat(&QuicSocketStats::packets_ignored); + return; + } } CHECK(session); @@ -683,6 +696,8 @@ bool QuicSocket::SendRetry( } // Shutdown a connection prematurely, before a QuicSession is created. +// This should only be called t the start of a session before the crypto +// keys have been established. void QuicSocket::ImmediateConnectionClose( const QuicCID& scid, const QuicCID& dcid, @@ -819,6 +834,18 @@ BaseObjectPtr QuicSocket::AcceptInitialPacket( listener_->OnSessionReady(session); + // It's possible that the session was destroyed while processing + // the ready callback. If it was, then we need to send an early + // CONNECTION_CLOSE. + if (session->is_destroyed()) { + ImmediateConnectionClose( + QuicCID(hd.scid), + QuicCID(hd.dcid), + local_addr, + remote_addr, + NGTCP2_CONNECTION_REFUSED); + } + return session; } @@ -826,9 +853,9 @@ QuicSocket::SendWrap::SendWrap( QuicState* quic_state, Local req_wrap_obj, size_t total_length) - : ReqWrap(quic_state->env(), req_wrap_obj, PROVIDER_QUICSOCKET), - total_length_(total_length), - quic_state_(quic_state) { + : ReqWrap(quic_state->env(), req_wrap_obj, PROVIDER_QUICSOCKET), + total_length_(total_length), + quic_state_(quic_state) { } std::string QuicSocket::SendWrap::MemoryInfoName() const { @@ -1093,7 +1120,7 @@ void QuicSocketStopListening(const FunctionCallbackInfo& args) { socket->StopListening(); } -void QuicSocketset_server_busy(const FunctionCallbackInfo& args) { +void QuicSocketSetServerBusy(const FunctionCallbackInfo& args) { QuicSocket* socket; ASSIGN_OR_RETURN_UNWRAP(&socket, args.Holder()); CHECK_EQ(args.Length(), 1); @@ -1164,7 +1191,7 @@ void QuicSocket::Initialize( QuicSocketSetDiagnosticPacketLoss); env->SetProtoMethod(socket, "setServerBusy", - QuicSocketset_server_busy); + QuicSocketSetServerBusy); env->SetProtoMethod(socket, "stopListening", QuicSocketStopListening); diff --git a/src/quic/node_quic_socket.h b/src/quic/node_quic_socket.h index 08ccd74dcb52f0..2fcf0fb7f19d31 100644 --- a/src/quic/node_quic_socket.h +++ b/src/quic/node_quic_socket.h @@ -33,6 +33,9 @@ using v8::Value; namespace quic { +class QuicSocket; +class QuicEndpoint; + enum QuicSocketOptions : uint32_t { // When enabled the QuicSocket will validate the address // using a RETRY packet to the peer. @@ -80,9 +83,6 @@ struct QuicSocketStatsTraits { static void ToString(const Base& ptr, Fn&& add_field); }; -class QuicSocket; -class QuicEndpoint; - // This is the generic interface for objects that control QuicSocket // instances. The default `JSQuicSocketListener` emits events to // JavaScript @@ -96,7 +96,7 @@ class QuicSocketListener { virtual void OnEndpointDone(QuicEndpoint* endpoint); virtual void OnDestroy(); - QuicSocket* socket() { return socket_.get(); } + QuicSocket* socket() const { return socket_.get(); } private: BaseObjectWeakPtr socket_; @@ -104,7 +104,7 @@ class QuicSocketListener { friend class QuicSocket; }; -class JSQuicSocketListener : public QuicSocketListener { +class JSQuicSocketListener final : public QuicSocketListener { public: void OnError(ssize_t code) override; void OnSessionReady(BaseObjectPtr session) override; @@ -121,17 +121,21 @@ constexpr size_t MAX_PKTLEN = std::max(NGTCP2_MAX_PKTLEN_IPV4, NGTCP2_MAX_PKTLEN_IPV6); // A serialized QuicPacket to be sent by a QuicSocket instance. +// QuicPackets are intended to be transient. They are created, +// filled with the contents of a serialized packet, and passed +// off immediately to the QuicSocket to be sent. As soon as +// the packet is sent, it is freed. class QuicPacket : public MemoryRetainer { public: // Creates a new QuicPacket. By default the packet will be // stack allocated with a max size of NGTCP2_MAX_PKTLEN_IPV4. // If a larger packet size is specified, it will be heap - // allocated. Generally speaking, a QUIC packet should never - // be larger than the current MTU to avoid IP fragmentation. + // allocated. A QUIC packet should never be larger than the + // current MTU to avoid IP fragmentation. // - // The content of a QuicPacket is provided by ngtcp2. The - // typical use pattern is to create a QuicPacket instance - // and then pass a pointer to it's internal buffer and max + // The content of a QuicPacket is provided by ngtcp2 and is + // opaque for us. The typical use pattern is to create a QuicPacket + // instance and then pass a pointer to it's internal buffer and max // size in to an ngtcp2 function that serializes the data. // ngtcp2 will fill the buffer as much as possible then return // the number of bytes serialized. User code is then responsible @@ -159,17 +163,22 @@ class QuicPacket : public MemoryRetainer { QuicPacket(const char* diagnostic_label, size_t len); QuicPacket(const QuicPacket& other); + uint8_t* data() { return data_; } + size_t length() const { return len_; } + uv_buf_t buf() const { return uv_buf_init( const_cast(reinterpret_cast(&data_)), length()); } + inline void set_length(size_t len); + const char* diagnostic_label() const; - void MemoryInfo(MemoryTracker* tracker) const override; + SET_NO_MEMORY_INFO(); SET_MEMORY_INFO_NAME(QuicPacket); SET_SELF_SIZE(QuicPacket); @@ -198,8 +207,8 @@ class QuicEndpointListener { // A QuicEndpoint wraps a UDPBaseWrap. A single QuicSocket may // have multiple QuicEndpoints, the lifecycles of which are // attached to the QuicSocket. -class QuicEndpoint : public BaseObject, - public UDPListener { +class QuicEndpoint final : public BaseObject, + public UDPListener { public: static void Initialize( Environment* env, @@ -212,9 +221,10 @@ class QuicEndpoint : public BaseObject, QuicSocket* listener, Local udp_wrap); - const SocketAddress& local_address() const { - local_address_ = udp_->GetSockName(); - return local_address_; + ~QuicEndpoint() override; + + SocketAddress local_address() const { + return udp_->GetSockName(); } // Implementation for UDPListener @@ -242,17 +252,16 @@ class QuicEndpoint : public BaseObject, void IncrementPendingCallbacks() { pending_callbacks_++; } void DecrementPendingCallbacks() { pending_callbacks_--; } - bool has_pending_callbacks() { return pending_callbacks_ > 0; } + bool has_pending_callbacks() const { return pending_callbacks_ > 0; } inline void WaitForPendingCallbacks(); QuicState* quic_state() const { return quic_state_.get(); } - void MemoryInfo(MemoryTracker* tracker) const override; + SET_NO_MEMORY_INFO(); SET_MEMORY_INFO_NAME(QuicEndpoint) SET_SELF_SIZE(QuicEndpoint) private: - mutable SocketAddress local_address_; BaseObjectWeakPtr listener_; UDPWrapBase* udp_; BaseObjectPtr strong_ptr_; @@ -298,7 +307,7 @@ class QuicSocket : public AsyncWrap, // Returns the default/preferred local address. Additional // QuicEndpoint instances may be associated with the // QuicSocket bound to other local addresses. - inline const SocketAddress& local_address(); + inline SocketAddress local_address() const; void MaybeClose(); @@ -342,8 +351,6 @@ class QuicSocket : public AsyncWrap, std::unique_ptr packet, BaseObjectPtr session = BaseObjectPtr()); - inline void SessionReady(BaseObjectPtr session); - inline void set_server_busy(bool on); inline void set_diagnostic_packet_loss(double rx = 0.0, double tx = 0.0);