From 054f57e215678fddebb3917ff086818537fa6715 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Mon, 23 Oct 2017 18:38:32 -0400 Subject: [PATCH 1/4] recover from client-caused panics --- irc/client.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/irc/client.go b/irc/client.go index f78ed23e..1e29b76c 100644 --- a/irc/client.go +++ b/irc/client.go @@ -184,6 +184,15 @@ func (client *Client) run() { var line string var msg ircmsg.IrcMessage + defer func() { + if r := recover(); r != nil { + client.server.logger.Error("internal", fmt.Sprintf("Client caused panic, disconnecting: %v", r)) + debug.PrintStack() + } + // ensure client connection gets closed + client.destroy() + }() + client.idletimer = NewIdleTimer(client) client.idletimer.Start() @@ -225,9 +234,6 @@ func (client *Client) run() { break } } - - // ensure client connection gets closed - client.destroy() } // From 80968d000ffd624a056e8e4dc9c0e0e472445526 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 25 Oct 2017 13:19:08 -0400 Subject: [PATCH 2/4] log panic traces via the usual logging mechanism --- irc/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/irc/client.go b/irc/client.go index 1e29b76c..fd44c883 100644 --- a/irc/client.go +++ b/irc/client.go @@ -186,8 +186,8 @@ func (client *Client) run() { defer func() { if r := recover(); r != nil { - client.server.logger.Error("internal", fmt.Sprintf("Client caused panic, disconnecting: %v", r)) - debug.PrintStack() + client.server.logger.Error("internal", + fmt.Sprintf("Client caused panic, disconnecting: %v\n%s", r, debug.Stack())) } // ensure client connection gets closed client.destroy() From 7b58bf76efb5650c96650ed24bbd30c0fd9d1e4e Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Thu, 26 Oct 2017 04:19:01 -0400 Subject: [PATCH 3/4] make error recovery configurable --- irc/client.go | 8 +++++--- irc/config.go | 3 ++- irc/getters.go | 6 ++++++ irc/server.go | 11 +++++++---- oragono.yaml | 8 ++++++++ 5 files changed, 28 insertions(+), 8 deletions(-) diff --git a/irc/client.go b/irc/client.go index fd44c883..528e1e19 100644 --- a/irc/client.go +++ b/irc/client.go @@ -185,9 +185,11 @@ func (client *Client) run() { var msg ircmsg.IrcMessage defer func() { - if r := recover(); r != nil { - client.server.logger.Error("internal", - fmt.Sprintf("Client caused panic, disconnecting: %v\n%s", r, debug.Stack())) + if client.server.RecoverFromErrors() { + if r := recover(); r != nil { + client.server.logger.Error("internal", + fmt.Sprintf("Client caused panic, disconnecting: %v\n%s", r, debug.Stack())) + } } // ensure client connection gets closed client.destroy() diff --git a/irc/config.go b/irc/config.go index 77f0039e..f1242d72 100644 --- a/irc/config.go +++ b/irc/config.go @@ -188,7 +188,8 @@ type Config struct { Logging []logger.LoggingConfig Debug struct { - StackImpact StackImpactConfig + RecoverFromErrors *bool `yaml:"recover-from-errors"` + StackImpact StackImpactConfig } Limits struct { diff --git a/irc/getters.go b/irc/getters.go index e76938d4..aa4230dc 100644 --- a/irc/getters.go +++ b/irc/getters.go @@ -23,6 +23,12 @@ func (server *Server) getPassword() []byte { return server.password } +func (server *Server) RecoverFromErrors() bool { + server.configurableStateMutex.RLock() + defer server.configurableStateMutex.RUnlock() + return server.recoverFromErrors +} + func (server *Server) ProxyAllowedFrom() []string { server.configurableStateMutex.RLock() defer server.configurableStateMutex.RUnlock() diff --git a/irc/server.go b/irc/server.go index 64432f15..dbf4498d 100644 --- a/irc/server.go +++ b/irc/server.go @@ -109,6 +109,7 @@ type Server struct { operclasses map[string]OperClass password []byte passwords *passwd.SaltedManager + recoverFromErrors bool registeredChannels map[string]*RegisteredChannel registeredChannelsMutex sync.RWMutex rehashMutex sync.Mutex @@ -1250,21 +1251,23 @@ func (server *Server) applyConfig(config *Config, initial bool) error { server.name = config.Server.Name server.nameCasefolded = casefoldedName } - server.networkName = config.Network.Name server.configurableStateMutex.Lock() + server.networkName = config.Network.Name if config.Server.Password != "" { server.password = config.Server.PasswordBytes() } else { server.password = nil } - server.configurableStateMutex.Unlock() - // apply new WebIRC command restrictions server.webirc = config.Server.WebIRC - // apply new PROXY command restrictions server.proxyAllowedFrom = config.Server.ProxyAllowedFrom + server.recoverFromErrors = true + if config.Debug.RecoverFromErrors != nil { + server.recoverFromErrors = *config.Debug.RecoverFromErrors + } + server.configurableStateMutex.Unlock() err = server.connectionLimiter.ApplyConfig(config.Server.ConnectionLimiter) if err != nil { diff --git a/oragono.yaml b/oragono.yaml index 02dc6bb3..0d72a0b3 100644 --- a/oragono.yaml +++ b/oragono.yaml @@ -267,6 +267,14 @@ logging: # debug options debug: + # when enabled, oragono will attempt to recover from certain kinds of + # client-triggered runtime errors that would normally crash the server. + # this makes the server more resilient to DoS, but could result in incorrect + # behavior. deployments that would prefer to "start from scratch", e.g., by + # letting the process crash and auto-restarting it with systemd, can set + # this to false. + recover-from-errors: true + # enabling StackImpact profiling stackimpact: # whether to use StackImpact From 6130e48a6705971af96534c719c073b5c43cc0bc Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Thu, 26 Oct 2017 05:15:55 -0400 Subject: [PATCH 4/4] always log the panic trace --- irc/client.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/irc/client.go b/irc/client.go index 528e1e19..9fc9d929 100644 --- a/irc/client.go +++ b/irc/client.go @@ -185,10 +185,13 @@ func (client *Client) run() { var msg ircmsg.IrcMessage defer func() { - if client.server.RecoverFromErrors() { - if r := recover(); r != nil { - client.server.logger.Error("internal", - fmt.Sprintf("Client caused panic, disconnecting: %v\n%s", r, debug.Stack())) + if r := recover(); r != nil { + client.server.logger.Error("internal", + fmt.Sprintf("Client caused panic: %v\n%s", r, debug.Stack())) + if client.server.RecoverFromErrors() { + client.server.logger.Error("internal", "Disconnecting client and attempting to recover") + } else { + panic(r) } } // ensure client connection gets closed