From 309aa5ff3b521c5767dbe33d7d53828d679db2ea Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 25 Apr 2022 12:10:39 +0100 Subject: [PATCH] fix deadlock on concurrent http3.Server.Serve and Close calls (#3387) --- http3/server.go | 97 +++++++++++++++++++++++--------------------- http3/server_test.go | 17 ++++++++ 2 files changed, 68 insertions(+), 46 deletions(-) diff --git a/http3/server.go b/http3/server.go index 3897bbb9ffb..e9eafda2fa3 100644 --- a/http3/server.go +++ b/http3/server.go @@ -160,12 +160,11 @@ type Server struct { mutex sync.RWMutex listeners map[*quic.EarlyListener]listenerInfo - closed utils.AtomicBool + closed bool altSvcHeader string - loggerOnce sync.Once - logger utils.Logger + logger utils.Logger } // ListenAndServe listens on the UDP address s.Addr and calls s.Handler to handle HTTP/3 requests on incoming connections. @@ -203,55 +202,54 @@ func (s *Server) Serve(conn net.PacketConn) error { // Make sure you use http3.ConfigureTLSConfig to configure a tls.Config // and use it to construct a http3-friendly QUIC listener. // Closing the server does close the listener. -func (s *Server) ServeListener(listener quic.EarlyListener) error { - return s.serveImpl(func() (quic.EarlyListener, error) { return listener, nil }) -} - -func (s *Server) serveConn(tlsConf *tls.Config, conn net.PacketConn) error { - return s.serveImpl(func() (quic.EarlyListener, error) { - baseConf := ConfigureTLSConfig(tlsConf) - quicConf := s.QuicConfig - if quicConf == nil { - quicConf = &quic.Config{} - } else { - quicConf = s.QuicConfig.Clone() - } - if s.EnableDatagrams { - quicConf.EnableDatagrams = true - } +func (s *Server) ServeListener(ln quic.EarlyListener) error { + if s.Server == nil { + return errors.New("use of http3.Server without http.Server") + } - var ln quic.EarlyListener - var err error - if conn == nil { - ln, err = quicListenAddr(s.Addr, baseConf, quicConf) - } else { - ln, err = quicListen(conn, baseConf, quicConf) - } - if err != nil { - return nil, err - } - return ln, nil - }) + if err := s.addListener(&ln); err != nil { + return err + } + err := s.serveListener(ln) + s.removeListener(&ln) + return err } -func (s *Server) serveImpl(startListener func() (quic.EarlyListener, error)) error { - if s.closed.Get() { - return http.ErrServerClosed - } +func (s *Server) serveConn(tlsConf *tls.Config, conn net.PacketConn) error { if s.Server == nil { return errors.New("use of http3.Server without http.Server") } - s.loggerOnce.Do(func() { - s.logger = utils.DefaultLogger.WithPrefix("server") - }) - ln, err := startListener() + baseConf := ConfigureTLSConfig(tlsConf) + quicConf := s.QuicConfig + if quicConf == nil { + quicConf = &quic.Config{} + } else { + quicConf = s.QuicConfig.Clone() + } + if s.EnableDatagrams { + quicConf.EnableDatagrams = true + } + + var ln quic.EarlyListener + var err error + if conn == nil { + ln, err = quicListenAddr(s.Addr, baseConf, quicConf) + } else { + ln, err = quicListen(conn, baseConf, quicConf) + } if err != nil { return err } - s.addListener(&ln) - defer s.removeListener(&ln) + if err := s.addListener(&ln); err != nil { + return err + } + err = s.serveListener(ln) + s.removeListener(&ln) + return err +} +func (s *Server) serveListener(ln quic.EarlyListener) error { for { conn, err := ln.Accept(context.Background()) if err != nil { @@ -327,8 +325,16 @@ func (s *Server) generateAltSvcHeader() { // We store a pointer to interface in the map set. This is safe because we only // call trackListener via Serve and can track+defer untrack the same pointer to // local variable there. We never need to compare a Listener from another caller. -func (s *Server) addListener(l *quic.EarlyListener) { +func (s *Server) addListener(l *quic.EarlyListener) error { s.mutex.Lock() + defer s.mutex.Unlock() + + if s.closed { + return http.ErrServerClosed + } + if s.logger == nil { + s.logger = utils.DefaultLogger.WithPrefix("server") + } if s.listeners == nil { s.listeners = make(map[*quic.EarlyListener]listenerInfo) } @@ -341,8 +347,7 @@ func (s *Server) addListener(l *quic.EarlyListener) { s.listeners[l] = listenerInfo{} } s.generateAltSvcHeader() - - s.mutex.Unlock() + return nil } func (s *Server) removeListener(l *quic.EarlyListener) { @@ -553,11 +558,11 @@ func (s *Server) handleRequest(conn quic.Connection, str quic.Stream, decoder *q // Close the server immediately, aborting requests and sending CONNECTION_CLOSE frames to connected clients. // Close in combination with ListenAndServe() (instead of Serve()) may race if it is called before a UDP socket is established. func (s *Server) Close() error { - s.closed.Set(true) - s.mutex.Lock() defer s.mutex.Unlock() + s.closed = true + var err error for ln := range s.listeners { if cerr := (*ln).Close(); cerr != nil && err == nil { diff --git a/http3/server_test.go b/http3/server_test.go index f64669722b9..4b99775a60c 100644 --- a/http3/server_test.go +++ b/http3/server_test.go @@ -9,6 +9,7 @@ import ( "io" "net" "net/http" + "runtime" "sync/atomic" "time" @@ -775,6 +776,22 @@ var _ = Describe("Server", func() { Expect(serv.ListenAndServe()).To(MatchError(http.ErrServerClosed)) }) + It("handles concurrent Serve and Close", func() { + addr, err := net.ResolveUDPAddr("udp", "localhost:0") + Expect(err).ToNot(HaveOccurred()) + c, err := net.ListenUDP("udp", addr) + Expect(err).ToNot(HaveOccurred()) + done := make(chan struct{}) + go func() { + defer GinkgoRecover() + defer close(done) + s.Serve(c) + }() + runtime.Gosched() + s.Close() + Eventually(done).Should(BeClosed()) + }) + Context("ConfigureTLSConfig", func() { var tlsConf *tls.Config var ch *tls.ClientHelloInfo