@@ -207,16 +207,16 @@ class Chttp2ServerListener : public Server::ListenerInterface {
207207 grpc_pollset_set* const interested_parties_;
208208 };
209209
210- ActiveConnection (grpc_pollset* accepting_pollset, AcceptorPtr acceptor,
210+ ActiveConnection (RefCountedPtr<Chttp2ServerListener> listener,
211+ grpc_pollset* accepting_pollset, AcceptorPtr acceptor,
211212 EventEngine* event_engine, const ChannelArgs& args,
212213 MemoryOwner memory_owner);
213214
214215 void Orphan () override ;
215216
216217 void SendGoAway ();
217218
218- void Start (RefCountedPtr<Chttp2ServerListener> listener,
219- OrphanablePtr<grpc_endpoint> endpoint, const ChannelArgs& args);
219+ void Start (OrphanablePtr<grpc_endpoint> endpoint, const ChannelArgs& args);
220220
221221 // Needed to be able to grab an external ref in
222222 // Chttp2ServerListener::OnAccept()
@@ -228,6 +228,9 @@ class Chttp2ServerListener : public Server::ListenerInterface {
228228
229229 RefCountedPtr<Chttp2ServerListener> listener_;
230230 Mutex mu_ ABSL_ACQUIRED_AFTER (&listener_->mu_);
231+ // Was ActiveConnection::Start() invoked? Used to determine whether
232+ // tcp_server needs to be unreffed.
233+ bool connection_started_ ABSL_GUARDED_BY (&mu_) = false;
231234 // Set by HandshakingState before the handshaking begins and reset when
232235 // handshaking is done.
233236 OrphanablePtr<HandshakingState> handshaking_state_ ABSL_GUARDED_BY (&mu_);
@@ -390,11 +393,16 @@ Chttp2ServerListener::ActiveConnection::HandshakingState::HandshakingState(
390393}
391394
392395Chttp2ServerListener::ActiveConnection::HandshakingState::~HandshakingState () {
396+ bool connection_started = false ;
397+ {
398+ MutexLock lock (&connection_->mu_ );
399+ connection_started = connection_->connection_started_ ;
400+ }
393401 if (accepting_pollset_ != nullptr ) {
394402 grpc_pollset_set_del_pollset (interested_parties_, accepting_pollset_);
395403 }
396404 grpc_pollset_set_destroy (interested_parties_);
397- if (connection_->listener_ != nullptr &&
405+ if (connection_started && connection_->listener_ != nullptr &&
398406 connection_->listener_ ->tcp_server_ != nullptr ) {
399407 grpc_tcp_server_unref (connection_->listener_ ->tcp_server_ );
400408 }
@@ -566,10 +574,12 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone(
566574//
567575
568576Chttp2ServerListener::ActiveConnection::ActiveConnection (
577+ RefCountedPtr<Chttp2ServerListener> listener,
569578 grpc_pollset* accepting_pollset, AcceptorPtr acceptor,
570579 EventEngine* event_engine, const ChannelArgs& args,
571580 MemoryOwner memory_owner)
572- : handshaking_state_(memory_owner.MakeOrphanable<HandshakingState>(
581+ : listener_(std::move(listener)),
582+ handshaking_state_ (memory_owner.MakeOrphanable<HandshakingState>(
573583 Ref (), accepting_pollset, std::move(acceptor), args)),
574584 event_engine_(event_engine) {
575585 GRPC_CLOSURE_INIT (&on_close_, ActiveConnection::OnClose, this ,
@@ -626,12 +636,11 @@ void Chttp2ServerListener::ActiveConnection::SendGoAway() {
626636}
627637
628638void Chttp2ServerListener::ActiveConnection::Start (
629- RefCountedPtr<Chttp2ServerListener> listener,
630639 OrphanablePtr<grpc_endpoint> endpoint, const ChannelArgs& args) {
631- listener_ = std::move (listener);
632640 RefCountedPtr<HandshakingState> handshaking_state_ref;
633641 {
634642 MutexLock lock (&mu_);
643+ connection_started_ = true ;
635644 // If the Connection is already shutdown at this point, it implies the
636645 // owning Chttp2ServerListener and all associated ActiveConnections have
637646 // been orphaned.
@@ -863,34 +872,32 @@ void Chttp2ServerListener::OnAccept(void* arg, grpc_endpoint* tcp,
863872 auto memory_owner = self->memory_quota_ ->CreateMemoryOwner ();
864873 EventEngine* const event_engine = self->args_ .GetObject <EventEngine>();
865874 auto connection = memory_owner.
MakeOrphanable <ActiveConnection>(
866- accepting_pollset, std::move (acceptor ), event_engine, args ,
867- std::move (memory_owner));
875+ self-> RefAsSubclass <Chttp2ServerListener>( ), accepting_pollset ,
876+ std::move (acceptor), event_engine, args, std::move ( memory_owner));
868877 // Hold a ref to connection to allow starting handshake outside the
869878 // critical region
870879 RefCountedPtr<ActiveConnection> connection_ref = connection->Ref ();
871- RefCountedPtr<Chttp2ServerListener> listener_ref;
872880 {
873881 MutexLock lock (&self->mu_ );
874882 // Shutdown the the connection if listener's stopped serving or if the
875883 // connection manager has changed.
876884 if (!self->shutdown_ && self->is_serving_ &&
877885 connection_manager == self->connection_manager_ ) {
878- // The ref for both the listener and tcp_server need to be taken in the
879- // critical region after having made sure that the listener has not been
880- // Orphaned, so as to avoid heap-use-after-free issues where `Ref()` is
881- // invoked when the listener is already shutdown. Note that the listener
882- // holds a ref to the tcp_server but this ref is given away when the
883- // listener is orphaned (shutdown). A connection needs the tcp_server to
884- // outlast the handshake since the acceptor needs it.
886+ // The ref for the tcp_server needs to be taken in the critical region
887+ // after having made sure that the listener has not been Orphaned, so as
888+ // to avoid heap-use-after-free issues where `Ref()` is invoked when the
889+ // listener is already shutdown. Note that the listener holds a ref to the
890+ // tcp_server but this ref is given away when the listener is orphaned
891+ // (shutdown). A connection needs the tcp_server to outlast the handshake
892+ // since the acceptor needs it.
885893 if (self->tcp_server_ != nullptr ) {
886894 grpc_tcp_server_ref (self->tcp_server_ );
887895 }
888- listener_ref = self->RefAsSubclass <Chttp2ServerListener>();
889896 self->connections_ .emplace (connection.get (), std::move (connection));
890897 }
891898 }
892- if (connection == nullptr && listener_ref != nullptr ) {
893- connection_ref->Start (std::move (listener_ref), std::move ( endpoint), args);
899+ if (connection == nullptr ) {
900+ connection_ref->Start (std::move (endpoint), args);
894901 }
895902}
896903
0 commit comments