From 9004d69c6f8eff499032ebeadcd21bdb6428745c Mon Sep 17 00:00:00 2001
From: Klaus Post <klauspost@gmail.com>
Date: Thu, 30 Jun 2022 10:48:50 -0700
Subject: [PATCH] Make ReqInfo concurrency safe (#15204)

Some read/writes of ReqInfo did not get appropriate locks, leading to races.

Make sure reading and writing holds appropriate locks.
---
 cmd/utils.go               | 2 ++
 internal/logger/audit.go   | 3 ++-
 internal/logger/logger.go  | 2 ++
 internal/logger/reqinfo.go | 4 ++--
 internal/rest/client.go    | 6 +++---
 5 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/cmd/utils.go b/cmd/utils.go
index 2148a78d3..7370cb8f3 100644
--- a/cmd/utils.go
+++ b/cmd/utils.go
@@ -792,6 +792,8 @@ func likelyUnescapeGeneric(p string, escapeFn func(string) (string, error)) stri
 func updateReqContext(ctx context.Context, objects ...ObjectV) context.Context {
 	req := logger.GetReqInfo(ctx)
 	if req != nil {
+		req.Lock()
+		defer req.Unlock()
 		req.Objects = make([]logger.ObjectVersion, 0, len(objects))
 		for _, ov := range objects {
 			req.Objects = append(req.Objects, logger.ObjectVersion{
diff --git a/internal/logger/audit.go b/internal/logger/audit.go
index 213775e78..bd456bdd3 100644
--- a/internal/logger/audit.go
+++ b/internal/logger/audit.go
@@ -149,7 +149,6 @@ func GetAuditEntry(ctx context.Context) *audit.Entry {
 			DeploymentID: xhttp.GlobalDeploymentID,
 			Time:         time.Now().UTC(),
 		}
-		SetAuditEntry(ctx, r)
 		return r
 	}
 	return nil
@@ -168,6 +167,8 @@ func AuditLog(ctx context.Context, w http.ResponseWriter, r *http.Request, reqCl
 		if reqInfo == nil {
 			return
 		}
+		reqInfo.RLock()
+		defer reqInfo.RUnlock()
 
 		entry = audit.ToEntry(w, r, reqClaims, xhttp.GlobalDeploymentID)
 		// indicates all requests for this API call are inbound
diff --git a/internal/logger/logger.go b/internal/logger/logger.go
index 68b342c45..b35e4471c 100644
--- a/internal/logger/logger.go
+++ b/internal/logger/logger.go
@@ -290,6 +290,8 @@ func errToEntry(ctx context.Context, err error, errKind ...interface{}) log.Entr
 	if req == nil {
 		req = &ReqInfo{API: "SYSTEM"}
 	}
+	req.RLock()
+	defer req.RUnlock()
 
 	API := "SYSTEM"
 	if req.API != "" {
diff --git a/internal/logger/reqinfo.go b/internal/logger/reqinfo.go
index c2b62ae26..05c012883 100644
--- a/internal/logger/reqinfo.go
+++ b/internal/logger/reqinfo.go
@@ -41,6 +41,7 @@ type ObjectVersion struct {
 }
 
 // ReqInfo stores the request info.
+// Reading/writing directly to struct requires appropriate R/W lock.
 type ReqInfo struct {
 	RemoteHost   string          // Client Host/IP
 	Host         string          // Node Host/IP
@@ -111,7 +112,7 @@ func (r *ReqInfo) GetTags() []KeyVal {
 	}
 	r.RLock()
 	defer r.RUnlock()
-	return append([]KeyVal(nil), r.tags...)
+	return append(make([]KeyVal, 0, len(r.tags)), r.tags...)
 }
 
 // GetTagsMap - returns the user defined tags in a map structure
@@ -145,7 +146,6 @@ func GetReqInfo(ctx context.Context) *ReqInfo {
 			return r
 		}
 		r = &ReqInfo{}
-		SetReqInfo(ctx, r)
 		return r
 	}
 	return nil
diff --git a/internal/rest/client.go b/internal/rest/client.go
index 9425bc41b..9d264fc1b 100644
--- a/internal/rest/client.go
+++ b/internal/rest/client.go
@@ -148,7 +148,7 @@ func (c *Client) Call(ctx context.Context, method string, values url.Values, bod
 				atomic.AddUint64(&networkErrsCounter, 1)
 			}
 			if c.MarkOffline() {
-				logger.LogIf(context.Background(), fmt.Errorf("Marking %s temporary offline; caused by %w", c.url.String(), err))
+				logger.LogIf(ctx, fmt.Errorf("Marking %s temporary offline; caused by %w", c.url.String(), err))
 			}
 		}
 		return nil, &NetworkError{err}
@@ -171,7 +171,7 @@ func (c *Client) Call(ctx context.Context, method string, values url.Values, bod
 		// fully it should make sure to respond with '412'
 		// instead, see cmd/storage-rest-server.go for ideas.
 		if c.HealthCheckFn != nil && resp.StatusCode == http.StatusPreconditionFailed {
-			logger.LogIf(context.Background(), fmt.Errorf("Marking %s temporary offline; caused by PreconditionFailed with disk ID mismatch", c.url.String()))
+			logger.LogIf(ctx, fmt.Errorf("Marking %s temporary offline; caused by PreconditionFailed with disk ID mismatch", c.url.String()))
 			c.MarkOffline()
 		}
 		defer xhttp.DrainBody(resp.Body)
@@ -183,7 +183,7 @@ func (c *Client) Call(ctx context.Context, method string, values url.Values, bod
 					atomic.AddUint64(&networkErrsCounter, 1)
 				}
 				if c.MarkOffline() {
-					logger.LogIf(context.Background(), fmt.Errorf("Marking %s temporary offline; caused by %w", c.url.String(), err))
+					logger.LogIf(ctx, fmt.Errorf("Marking %s temporary offline; caused by %w", c.url.String(), err))
 				}
 			}
 			return nil, err