From 29edcaa3305d58ad1b31d7ecaf78b530f27c7f7b Mon Sep 17 00:00:00 2001 From: Jason McNeil Date: Fri, 31 Oct 2025 10:23:21 -0300 Subject: [PATCH 1/2] Add defensive copying to internal storage implementations - Add defensive copying for string keys and []byte values in internal/memory - Add defensive copying for string keys and []byte values in internal/storage/memory - Remove redundant defensive copying from CSRF middleware (storage handles it) - Remove redundant defensive copying from idempotency middleware (msgp marshaling creates new bytes) - Update package documentation to explain safety guarantees This prevents memory corruption from sync.Pool buffer reuse when keys or values backed by pooled buffers are stored in maps. The storage layers now handle defensive copying automatically, eliminating the need for middleware to perform this operation. --- internal/memory/memory.go | 42 +++++++++++++++++++++++++-- internal/storage/memory/memory.go | 12 ++++++-- middleware/csrf/storage_manager.go | 6 ++-- middleware/idempotency/idempotency.go | 3 +- 4 files changed, 51 insertions(+), 12 deletions(-) diff --git a/internal/memory/memory.go b/internal/memory/memory.go index d61c66d8d9b..ee985ea53f4 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -1,5 +1,19 @@ -// Package memory Is a slight copy of the memory storage, but far from the storage interface it can not only work with bytes -// but directly store any kind of data without having to encode it each time, which gives a huge speed advantage +// Package memory provides a high-performance in-memory storage that can store +// any type without encoding overhead. Unlike the standard storage interface, +// this storage works directly with Go types for maximum speed. +// +// # Safety Considerations +// +// This storage automatically performs defensive copying for: +// - String keys: Copied to prevent corruption from pooled buffers +// - []byte values: Copied on both Set and Get to prevent external mutation +// +// For other types (structs, ints, etc.), Go's value semantics provide natural +// protection. However, if storing pointers or slices of non-byte types, +// callers are responsible for not mutating the underlying data. +// +// This storage is primarily used internally by middleware for performance- +// critical operations where the stored data types are known and controlled. package memory import ( @@ -33,6 +47,9 @@ func New() *Storage { // Get retrieves the value stored under key, returning nil when the entry does // not exist or has expired. +// +// For []byte values, this returns a defensive copy to prevent callers from +// mutating the stored data. Other types are returned as-is. func (s *Storage) Get(key string) any { s.RLock() v, ok := s.data[key] @@ -40,19 +57,38 @@ func (s *Storage) Get(key string) any { if !ok || v.e != 0 && v.e <= utils.Timestamp() { return nil } + + // Defensive copy for byte slices to prevent external mutation + if b, ok := v.v.([]byte); ok { + return utils.CopyBytes(b) + } + return v.v } // Set stores val under key and applies the optional ttl before expiring the // entry. A non-positive ttl keeps the item forever. +// +// String keys are defensively copied to prevent corruption from pooled buffers. +// []byte values are also copied to prevent external mutation of stored data. +// Other types are stored as-is (structs are copied by value automatically). func (s *Storage) Set(key string, val any, ttl time.Duration) { var exp uint32 if ttl > 0 { exp = uint32(ttl.Seconds()) + utils.Timestamp() } + + // Defensive copies to prevent unsafe reuse from sync.Pool + keyCopy := utils.CopyString(key) + + // Copy byte slices to prevent external mutation + if b, ok := val.([]byte); ok { + val = utils.CopyBytes(b) + } + i := item{e: exp, v: val} s.Lock() - s.data[key] = i + s.data[keyCopy] = i s.Unlock() } diff --git a/internal/storage/memory/memory.go b/internal/storage/memory/memory.go index 3649850fc0b..26d4f97580c 100644 --- a/internal/storage/memory/memory.go +++ b/internal/storage/memory/memory.go @@ -53,11 +53,13 @@ func (s *Storage) Get(key string) ([]byte, error) { s.mux.RLock() v, ok := s.db[key] s.mux.RUnlock() + if !ok || v.expiry != 0 && v.expiry <= utils.Timestamp() { return nil, nil } - return v.data, nil + // Return a copy to prevent callers from mutating stored data + return utils.CopyBytes(v.data), nil } // GetWithContext retrieves the value for the given key while honoring context @@ -86,9 +88,13 @@ func (s *Storage) Set(key string, val []byte, exp time.Duration) error { expire = uint32(exp.Seconds()) + utils.Timestamp() } - e := entry{data: val, expiry: expire} + // Copy both key and value to avoid unsafe reuse from sync.Pool + keyCopy := utils.CopyString(key) + valCopy := utils.CopyBytes(val) + + e := entry{data: valCopy, expiry: expire} s.mux.Lock() - s.db[key] = e + s.db[keyCopy] = e s.mux.Unlock() return nil } diff --git a/middleware/csrf/storage_manager.go b/middleware/csrf/storage_manager.go index 2eab31417c3..6b42011ae8a 100644 --- a/middleware/csrf/storage_manager.go +++ b/middleware/csrf/storage_manager.go @@ -8,7 +8,6 @@ import ( "github.com/gofiber/fiber/v3" "github.com/gofiber/fiber/v3/internal/memory" - utils "github.com/gofiber/utils/v2" ) // msgp -file="storage_manager.go" -o="storage_manager_msgp.go" -tests=true -unexported @@ -41,7 +40,7 @@ func newStorageManager(storage fiber.Storage, redactKeys bool) *storageManager { // Use provided storage if provided storageManager.storage = storage } else { - // Fallback too memory storage + // Fallback to memory storage storageManager.memory = memory.New() } return storageManager @@ -77,8 +76,7 @@ func (m *storageManager) setRaw(ctx context.Context, key string, raw []byte, exp return nil } - // The key and value are crucial in csrf and can be references to data that might be reused (e.g., from a pool). To prevent unsafe value retention, copies of both the key and raw value are made here. - m.memory.Set(utils.CopyString(key), utils.CopyBytes(raw), exp) + m.memory.Set(key, raw, exp) return nil } diff --git a/middleware/idempotency/idempotency.go b/middleware/idempotency/idempotency.go index 05adc96d741..86d1240271a 100644 --- a/middleware/idempotency/idempotency.go +++ b/middleware/idempotency/idempotency.go @@ -135,8 +135,7 @@ func New(config ...Config) fiber.Handler { // Construct response res := &response{ StatusCode: c.Response().StatusCode(), - - Body: utils.CopyBytes(c.Response().Body()), + Body: c.Response().Body(), } { headers := make(map[string][]string) From 0e744b948f21775b0da94778f11b299784f163f4 Mon Sep 17 00:00:00 2001 From: Jason McNeil Date: Mon, 3 Nov 2025 19:42:21 -0500 Subject: [PATCH 2/2] fix(memory): correct Entry type usage and remove extraneous blank lines - Use the exported Entry type when inserting into storage (fixes incorrect `entry` identifier). - Clean up unnecessary blank lines in internal memory implementations. --- internal/memory/memory.go | 10 +++++----- internal/storage/memory/memory.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/memory/memory.go b/internal/memory/memory.go index ebcbaed2b21..d3f07e8d610 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -57,12 +57,12 @@ func (s *Storage) Get(key string) any { if !ok || v.e != 0 && v.e <= utils.Timestamp() { return nil } - + // Defensive copy for byte slices to prevent external mutation if b, ok := v.v.([]byte); ok { return utils.CopyBytes(b) } - + return v.v } @@ -77,15 +77,15 @@ func (s *Storage) Set(key string, val any, ttl time.Duration) { if ttl > 0 { exp = uint32(ttl.Seconds()) + utils.Timestamp() } - + // Defensive copies to prevent unsafe reuse from sync.Pool keyCopy := utils.CopyString(key) - + // Copy byte slices to prevent external mutation if b, ok := val.([]byte); ok { val = utils.CopyBytes(b) } - + i := item{e: exp, v: val} s.Lock() s.data[keyCopy] = i diff --git a/internal/storage/memory/memory.go b/internal/storage/memory/memory.go index b8ca079cf24..2a677eafd42 100644 --- a/internal/storage/memory/memory.go +++ b/internal/storage/memory/memory.go @@ -90,7 +90,7 @@ func (s *Storage) Set(key string, val []byte, exp time.Duration) error { keyCopy := utils.CopyString(key) valCopy := utils.CopyBytes(val) - e := entry{data: valCopy, expiry: expire} + e := Entry{data: valCopy, expiry: expire} s.mux.Lock() s.db[keyCopy] = e s.mux.Unlock()