From b1376d5f71747c0d1123a7f20f2054ebc68d9828 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Thu, 7 Sep 2017 20:20:08 -0400 Subject: [PATCH 1/3] Fix a concurrency error with Server.listeners See #134; there was a `fatal error: concurrent map read and map write` due to unsynchronized accesses to `Server.listeners`. Now, `listeners` is only accessed by `NewServer` and `rehash`, so it doesn't need synchronization. --- irc/server.go | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/irc/server.go b/irc/server.go index d569258d..e5d15175 100644 --- a/irc/server.go +++ b/irc/server.go @@ -103,8 +103,7 @@ type Server struct { klines *KLineManager limits Limits listenerEventActMutex sync.Mutex - listeners map[string]ListenerInterface - listenerUpdateMutex sync.Mutex + listeners map[string]*ListenerInterface logger *logger.Manager MaxSendQBytes uint64 monitoring map[string][]*Client @@ -220,7 +219,7 @@ func NewServer(configFilename string, config *Config, logger *logger.Manager) (* Rest: config.Limits.LineLen.Rest, }, }, - listeners: make(map[string]ListenerInterface), + listeners: make(map[string]*ListenerInterface), logger: logger, MaxSendQBytes: config.Server.MaxSendQBytes, monitoring: make(map[string][]*Client), @@ -316,7 +315,7 @@ func NewServer(configFilename string, config *Config, logger *logger.Manager) (* tlsListeners := config.TLSListeners() for _, addr := range config.Server.Listen { - server.createListener(addr, tlsListeners) + server.listeners[addr] = server.createListener(addr, tlsListeners) } if len(tlsListeners) == 0 { @@ -509,14 +508,9 @@ func (server *Server) Run() { // // createListener starts the given listeners. -func (server *Server) createListener(addr string, tlsMap map[string]*tls.Config) { +func (server *Server) createListener(addr string, tlsMap map[string]*tls.Config) *ListenerInterface { config, listenTLS := tlsMap[addr] - _, alreadyExists := server.listeners[addr] - if alreadyExists { - log.Fatal(server, "listener already exists:", addr) - } - // make listener event channel listenerEventChannel := make(chan ListenerEvent, 1) @@ -538,7 +532,6 @@ func (server *Server) createListener(addr string, tlsMap map[string]*tls.Config) Events: listenerEventChannel, Listener: listener, } - server.listeners[addr] = li // start listening server.logger.Info("listeners", fmt.Sprintf("listening on %s using %s.", addr, tlsString)) @@ -558,7 +551,7 @@ func (server *Server) createListener(addr string, tlsMap map[string]*tls.Config) } select { - case event := <-server.listeners[addr].Events: + case event := <-li.Events: // this is used to confirm that whoever passed us this event has closed the existing listener correctly (in an attempt to get us to notice the event). // this is required to keep REHASH from having a very small race possibility of killing the primary listener server.listenerEventActMutex.Lock() @@ -588,9 +581,6 @@ func (server *Server) createListener(addr string, tlsMap map[string]*tls.Config) // update server ListenerInterface li.Listener = listener - server.listenerUpdateMutex.Lock() - server.listeners[addr] = li - server.listenerUpdateMutex.Unlock() // print notice server.logger.Info("listeners", fmt.Sprintf("updated listener %s using %s.", addr, tlsString)) @@ -600,6 +590,8 @@ func (server *Server) createListener(addr string, tlsMap map[string]*tls.Config) } } }() + + return &li } // @@ -1651,9 +1643,10 @@ func (server *Server) rehash() error { } server.clients.ByNickMutex.RUnlock() - // destroy old listeners + // update or destroy all existing listeners tlsListeners := config.TLSListeners() for addr := range server.listeners { + currentListener := server.listeners[addr] var exists bool for _, newaddr := range config.Server.Listen { if newaddr == addr { @@ -1665,27 +1658,29 @@ func (server *Server) rehash() error { server.listenerEventActMutex.Lock() if exists { // update old listener - server.listeners[addr].Events <- ListenerEvent{ + currentListener.Events <- ListenerEvent{ Type: UpdateListener, NewConfig: tlsListeners[addr], } } else { - // destroy nonexistent listener - server.listeners[addr].Events <- ListenerEvent{ + // destroy this listener, since it is no longer in the config + currentListener.Events <- ListenerEvent{ Type: DestroyListener, } + delete(server.listeners, addr) } // force listener to apply the event right away - server.listeners[addr].Listener.Close() - + // (this causes its Accept() call to return immediately with an error) + currentListener.Listener.Close() server.listenerEventActMutex.Unlock() } + // create new listeners that were not previously configured for _, newaddr := range config.Server.Listen { _, exists := server.listeners[newaddr] if !exists { // make new listener - server.createListener(newaddr, tlsListeners) + server.listeners[newaddr] = server.createListener(newaddr, tlsListeners) } } From d5528f6e5672d41d2e409680ec13c808f3993ef9 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Fri, 8 Sep 2017 06:02:54 -0400 Subject: [PATCH 2/3] execute rehash() in its own goroutine This prevents a deadlock: 1. rehash() is executing on the main goroutine 2. it's trying to stop a listener goroutine 3. the listener goroutine needs to hand off a new connection to newConns 4. but the main goroutine is blocked by rehash() so it can't receive it --- irc/server.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/irc/server.go b/irc/server.go index e5d15175..20ca52f1 100644 --- a/irc/server.go +++ b/irc/server.go @@ -436,10 +436,12 @@ func (server *Server) Run() { case <-server.rehashSignal: server.logger.Info("rehash", "Rehashing due to SIGHUP") - err := server.rehash() - if err != nil { - server.logger.Error("rehash", fmt.Sprintln("Failed to rehash:", err.Error())) - } + go func() { + err := server.rehash() + if err != nil { + server.logger.Error("rehash", fmt.Sprintln("Failed to rehash:", err.Error())) + } + }() case conn := <-server.newConns: // check connection limits From 0f0f2d1314bc9d8a5903227371121ce982b7faac Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Mon, 11 Sep 2017 18:40:15 -0400 Subject: [PATCH 3/3] refactor listener update/destroy code Don't close and reopen listeners --- irc/config.go | 1 + irc/server.go | 164 ++++++++++++++++++++------------------------------ 2 files changed, 66 insertions(+), 99 deletions(-) diff --git a/irc/config.go b/irc/config.go index 92973199..1c00bb11 100644 --- a/irc/config.go +++ b/irc/config.go @@ -368,6 +368,7 @@ func (conf *Config) TLSListeners() map[string]*tls.Config { tlsListeners := make(map[string]*tls.Config) for s, tlsListenersConf := range conf.Server.TLSListeners { config, err := tlsListenersConf.Config() + config.ClientAuth = tls.RequestClientCert if err != nil { log.Fatal(err) } diff --git a/irc/server.go b/irc/server.go index 20ca52f1..7ba042fe 100644 --- a/irc/server.go +++ b/irc/server.go @@ -57,26 +57,15 @@ type LineLenLimits struct { Rest int } -// ListenerInterface represents an interface for a listener. -type ListenerInterface struct { - Listener net.Listener - Events chan ListenerEvent -} - -const ( - // DestroyListener instructs the listener to destroy itself. - DestroyListener ListenerEventType = iota - // UpdateListener instructs the listener to update itself (grab new certs, etc). - UpdateListener = iota -) - -// ListenerEventType is the type of event this is. -type ListenerEventType int - -// ListenerEvent is an event that's passed to the listener. -type ListenerEvent struct { - Type ListenerEventType - NewConfig *tls.Config +// ListenerWrapper wraps a listener so it can be safely reconfigured or stopped +type ListenerWrapper struct { + listener net.Listener + tlsConfig *tls.Config + shouldStop bool + // lets the ListenerWrapper inform the server that it has stopped: + stopEvent chan bool + // protects atomic update of tlsConfig and shouldStop: + configMutex sync.Mutex } // Server is the main Oragono server. @@ -102,8 +91,7 @@ type Server struct { isupport *ISupportList klines *KLineManager limits Limits - listenerEventActMutex sync.Mutex - listeners map[string]*ListenerInterface + listeners map[string]*ListenerWrapper logger *logger.Manager MaxSendQBytes uint64 monitoring map[string][]*Client @@ -219,7 +207,7 @@ func NewServer(configFilename string, config *Config, logger *logger.Manager) (* Rest: config.Limits.LineLen.Rest, }, }, - listeners: make(map[string]*ListenerInterface), + listeners: make(map[string]*ListenerWrapper), logger: logger, MaxSendQBytes: config.Server.MaxSendQBytes, monitoring: make(map[string][]*Client), @@ -315,7 +303,7 @@ func NewServer(configFilename string, config *Config, logger *logger.Manager) (* tlsListeners := config.TLSListeners() for _, addr := range config.Server.Listen { - server.listeners[addr] = server.createListener(addr, tlsListeners) + server.listeners[addr] = server.createListener(addr, tlsListeners[addr]) } if len(tlsListeners) == 0 { @@ -510,90 +498,63 @@ func (server *Server) Run() { // // createListener starts the given listeners. -func (server *Server) createListener(addr string, tlsMap map[string]*tls.Config) *ListenerInterface { - config, listenTLS := tlsMap[addr] - - // make listener event channel - listenerEventChannel := make(chan ListenerEvent, 1) - +func (server *Server) createListener(addr string, tlsConfig *tls.Config) *ListenerWrapper { // make listener listener, err := net.Listen("tcp", addr) if err != nil { log.Fatal(server, "listen error: ", err) } + // throw our details to the server so we can be modified/killed later + wrapper := ListenerWrapper{ + listener: listener, + tlsConfig: tlsConfig, + shouldStop: false, + stopEvent: make(chan bool, 1), + } + + // TODO(slingamn) move all logging of listener status to rehash() tlsString := "plaintext" - if listenTLS { - config.ClientAuth = tls.RequestClientCert - listener = tls.NewListener(listener, config) + if tlsConfig != nil { tlsString = "TLS" } - - // throw our details to the server so we can be modified/killed later - li := ListenerInterface{ - Events: listenerEventChannel, - Listener: listener, - } - - // start listening server.logger.Info("listeners", fmt.Sprintf("listening on %s using %s.", addr, tlsString)) + var shouldStop bool + // setup accept goroutine go func() { for { conn, err := listener.Accept() + // synchronously access config data: + // whether TLS is enabled and whether we should stop listening + wrapper.configMutex.Lock() + shouldStop = wrapper.shouldStop + tlsConfig = wrapper.tlsConfig + wrapper.configMutex.Unlock() + if err == nil { + if tlsConfig != nil { + conn = tls.Server(conn, tlsConfig) + } newConn := clientConn{ Conn: conn, - IsTLS: listenTLS, + IsTLS: tlsConfig != nil, } - + // hand off the connection server.newConns <- newConn } - select { - case event := <-li.Events: - // this is used to confirm that whoever passed us this event has closed the existing listener correctly (in an attempt to get us to notice the event). - // this is required to keep REHASH from having a very small race possibility of killing the primary listener - server.listenerEventActMutex.Lock() - server.listenerEventActMutex.Unlock() - - if event.Type == DestroyListener { - // listener should already be closed, this is just for safety - listener.Close() - return - } else if event.Type == UpdateListener { - // close old listener - listener.Close() - - // make new listener - listener, err = net.Listen("tcp", addr) - if err != nil { - log.Fatal(server, "listen error: ", err) - } - - tlsString := "plaintext" - if event.NewConfig != nil { - config = event.NewConfig - config.ClientAuth = tls.RequestClientCert - listener = tls.NewListener(listener, config) - tlsString = "TLS" - } - - // update server ListenerInterface - li.Listener = listener - - // print notice - server.logger.Info("listeners", fmt.Sprintf("updated listener %s using %s.", addr, tlsString)) - } - default: - // no events waiting for us, fall-through and continue + if shouldStop { + listener.Close() + wrapper.stopEvent <- true + return } } }() - return &li + return &wrapper } // @@ -1649,32 +1610,37 @@ func (server *Server) rehash() error { tlsListeners := config.TLSListeners() for addr := range server.listeners { currentListener := server.listeners[addr] - var exists bool + var stillConfigured bool for _, newaddr := range config.Server.Listen { if newaddr == addr { - exists = true + stillConfigured = true break } } - server.listenerEventActMutex.Lock() - if exists { - // update old listener - currentListener.Events <- ListenerEvent{ - Type: UpdateListener, - NewConfig: tlsListeners[addr], - } + // pass new config information to the listener, to be picked up after + // its next Accept(). this is like sending over a buffered channel of + // size 1, but where sending a second item overwrites the buffered item + // instead of blocking. + currentListener.configMutex.Lock() + currentListener.shouldStop = !stillConfigured + currentListener.tlsConfig = tlsListeners[addr] + currentListener.configMutex.Unlock() + + if stillConfigured { + server.logger.Info("rehash", + fmt.Sprintf("now listening on %s, tls=%t.", addr, (currentListener.tlsConfig != nil)), + ) } else { - // destroy this listener, since it is no longer in the config - currentListener.Events <- ListenerEvent{ - Type: DestroyListener, - } + // tell the listener it should stop by interrupting its Accept() call: + currentListener.listener.Close() + // XXX there is no guarantee from the API when the address will actually + // free for bind(2) again; this solution "seems to work". See here: + // https://github.com/golang/go/issues/21833 + <-currentListener.stopEvent delete(server.listeners, addr) + server.logger.Info("rehash", fmt.Sprintf("stopped listening on %s.", addr)) } - // force listener to apply the event right away - // (this causes its Accept() call to return immediately with an error) - currentListener.Listener.Close() - server.listenerEventActMutex.Unlock() } // create new listeners that were not previously configured @@ -1682,7 +1648,7 @@ func (server *Server) rehash() error { _, exists := server.listeners[newaddr] if !exists { // make new listener - server.listeners[newaddr] = server.createListener(newaddr, tlsListeners) + server.listeners[newaddr] = server.createListener(newaddr, tlsListeners[newaddr]) } }