From cd152f404ad81a1fdabb94acd00f2b4225ff93c2 Mon Sep 17 00:00:00 2001
From: Andreas Auernhammer <aead@mail.de>
Date: Mon, 25 Jun 2018 22:51:49 +0200
Subject: [PATCH] replace os.Exit with panic for logger.CriticalIf (#6065)

This commit prevents complete server failures caused by
`logger.CriticalIf` calls. Instead of calling `os.Exit(1)`
the function now executes a panic with a special value
indicating that a critical error happend. At the top HTTP
handler layer panics are recovered and if its a critical
error the client gets an InternalServerError status code.

Further this allows unit tests to cover critical-error code
paths.
---
 cmd/gateway-main.go     |  2 +-
 cmd/generic-handlers.go | 17 +++++++++++++++++
 cmd/logger/logger.go    | 10 ++++++----
 cmd/server-main.go      |  2 +-
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go
index 60c9ef845..746f86d78 100644
--- a/cmd/gateway-main.go
+++ b/cmd/gateway-main.go
@@ -197,7 +197,7 @@ func StartGateway(ctx *cli.Context, gw Gateway) {
 		getCert = globalTLSCerts.GetCertificate
 	}
 
-	globalHTTPServer = xhttp.NewServer([]string{gatewayAddr}, registerHandlers(router, globalHandlers...), getCert)
+	globalHTTPServer = xhttp.NewServer([]string{gatewayAddr}, criticalErrorHandler{registerHandlers(router, globalHandlers...)}, getCert)
 	globalHTTPServer.UpdateBytesReadFunc = globalConnStats.incInputBytes
 	globalHTTPServer.UpdateBytesWrittenFunc = globalConnStats.incOutputBytes
 	go func() {
diff --git a/cmd/generic-handlers.go b/cmd/generic-handlers.go
index d40be534a..9f58c27e2 100644
--- a/cmd/generic-handlers.go
+++ b/cmd/generic-handlers.go
@@ -738,3 +738,20 @@ func (s securityHeaderHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 	header.Set("Content-Security-Policy", "block-all-mixed-content") // prevent mixed (HTTP / HTTPS content)
 	s.handler.ServeHTTP(w, r)
 }
+
+// criticalErrorHandler handles critical server failures caused by
+// `panic(logger.ErrCritical)` as done by `logger.CriticalIf`.
+//
+// It should be always the first / highest HTTP handler.
+type criticalErrorHandler struct{ handler http.Handler }
+
+func (h criticalErrorHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	defer func() {
+		if err := recover(); err == logger.ErrCritical { // handle
+			writeErrorResponse(w, ErrInternalError, r.URL)
+		} else if err != nil {
+			panic(err) // forward other panic calls
+		}
+	}()
+	h.handler.ServeHTTP(w, r)
+}
diff --git a/cmd/logger/logger.go b/cmd/logger/logger.go
index b815452a6..3b1603222 100644
--- a/cmd/logger/logger.go
+++ b/cmd/logger/logger.go
@@ -362,13 +362,15 @@ func LogIf(ctx context.Context, err error) {
 	fmt.Println(output)
 }
 
-// CriticalIf :
-// Like LogIf with exit
-// It'll be called for fatal error conditions during run-time
+// ErrCritical is the value panic'd whenever CriticalIf is called.
+var ErrCritical struct{}
+
+// CriticalIf logs the provided error on the console. It fails the
+// current go-routine by causing a `panic(ErrCritical)`.
 func CriticalIf(ctx context.Context, err error) {
 	if err != nil {
 		LogIf(ctx, err)
-		os.Exit(1)
+		panic(ErrCritical)
 	}
 }
 
diff --git a/cmd/server-main.go b/cmd/server-main.go
index 47cb6e05b..5092b4651 100644
--- a/cmd/server-main.go
+++ b/cmd/server-main.go
@@ -288,7 +288,7 @@ func serverMain(ctx *cli.Context) {
 		getCert = globalTLSCerts.GetCertificate
 	}
 
-	globalHTTPServer = xhttp.NewServer([]string{globalMinioAddr}, handler, getCert)
+	globalHTTPServer = xhttp.NewServer([]string{globalMinioAddr}, criticalErrorHandler{handler}, getCert)
 	globalHTTPServer.UpdateBytesReadFunc = globalConnStats.incInputBytes
 	globalHTTPServer.UpdateBytesWrittenFunc = globalConnStats.incOutputBytes
 	go func() {