Fix: Prevent memory corruption in internal memory storage from pooled buffers#3828
Conversation
… buffers The internal memory storage uses string keys and byte slice values directly in a Go map without copying them. When these strings/slices are backed by pooled buffers (from sync.Pool used by Fiber for performance), the map keys and values can become corrupted when those buffers are reused. This fix adds defensive copying of both keys and values before storing them in the map using utils.CopyString() and utils.CopyBytes(). Root Cause: - Fiber v3 uses sync.Pool extensively for byte buffer reuse - Strings created from pooled buffers point to the underlying pooled memory - When used as Go map keys without copying, these strings share the pooled buffer - When the buffer is returned to the pool and reused, the map key gets corrupted - This causes intermittent failures where sessions/CSRF tokens cannot be found Impact: - Before fix: ~8% test pass rate with repeated tests (ginkgo --repeat=50) - After fix: 100% test pass rate (200/200 passes with ginkgo --repeat=200) - Minimal performance overhead (one string copy + one slice copy per Set) The CSRF middleware already documents this issue and implements the same workaround for its internal memory storage (middleware/csrf/storage_manager.go:82).
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughGet now returns a defensive copy of stored values; Set copies both key and value before inserting into the in-memory map to avoid unsafe reuse or external mutation (changes limited to copy behavior and comments in Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant S as MemStorage
Note over C,S #d5f5e3: Set flow (new)
C->>S: Set(key, value, ttl)
S-->>S: keyCopy = copy(key)\nvalCopy = copy(value)
S-->>S: store map[keyCopy] = {data: valCopy, expiry}
S->>C: OK
Note over C,S #fce8d6: Get flow (new)
C->>S: Get(key)
S-->>S: entry = map[key]
S-->>S: return copy(entry.data)
S->>C: valueCopy
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @sixcolors, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical memory corruption bug within the internal memory storage mechanism. Previously, the system was vulnerable to data corruption when storing string keys and byte slice values directly from pooled buffers, as their underlying memory could be unexpectedly altered upon buffer reuse. The solution involves implementing defensive copying for both keys and values before they are stored, effectively isolating the map's data from external buffer reuse. This change ensures data integrity and significantly enhances the stability and reliability of the storage component with minimal performance overhead. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3828 +/- ##
=======================================
Coverage 92.25% 92.25%
=======================================
Files 115 115
Lines 9734 9734
=======================================
Hits 8980 8980
Misses 480 480
Partials 274 274
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds defensive copying of keys and values in the Set method of the memory storage implementation to prevent issues with sync.Pool reuse.
- Copies both string keys and byte slice values before storing them in the map
- Protects against unsafe memory reuse when strings/slices come from pooled resources
Comments suppressed due to low confidence (1)
internal/storage/memory/memory.go:90
- The
Deletemethod uses thekeyparameter directly without copying, which means it won't find entries stored with copied keys fromSet. SinceSetnow stores entries usingkeyCopy,Deleteshould also copy the key:delete(s.db, utils.CopyString(key))to ensure it can properly delete entries.
func (s *Storage) Delete(key string) error {
// Ain't Nobody Got Time For That
if len(key) == 0 {
return nil
}
s.mux.Lock()
delete(s.db, key)
| // Copy both key and value to avoid unsafe reuse from sync.Pool | ||
| keyCopy := utils.CopyString(key) |
There was a problem hiding this comment.
The Get method (line 57) returns v.data directly without copying, which could lead to the same sync.Pool reuse issues this change aims to prevent. If defensive copying is needed in Set, it should also be applied in Get to ensure data returned to callers cannot be unsafely modified or reused.
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a critical memory corruption bug in the Set method by copying keys and values that may originate from a sync.Pool. The analysis of the problem is excellent.
While this is a great fix, I've found several other critical vulnerabilities in internal/storage/memory/memory.go that should be addressed to make the component safe for concurrent use.
-
Unsafe Key Usage in
GetandDelete: These methods are also vulnerable to memory corruption from pooled keys. Thekeyshould be copied before map access.// Example for Delete() keyCopy := utils.CopyString(key) s.mux.Lock() delete(s.db, keyCopy) s.mux.Unlock()
-
Mutable Value Returned from
Get:Getreturns a direct reference to the internal byte slice, allowing callers to mutate stored data without a lock. It should return a copy.// Example for Get() // ... after finding entry v return utils.CopyBytes(v.data), nil
-
Conn()Exposes Internal Map: TheConn()method returns the raw internal map, completely bypassing the mutex. This is extremely unsafe and will lead to race conditions and panics. This method should likely be removed or fundamentally redesigned to return a safe, deep copy of the data.
I highly recommend addressing these critical issues to ensure the stability and correctness of the memory storage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/storage/memory/memory.go (1)
72-78: LGTM! The defensive copying correctly prevents memory corruption.The implementation properly addresses the sync.Pool corruption issue by copying both key and value before storage. The sequence is correct: early validation, copying outside the lock, then storing under lock.
Optional: Consider defensive copying in Get for consistency.
While not part of the current corruption issue, the Get method (line 57) returns
v.datadirectly without copying. If callers mutate the returned slice, it affects internal storage. For complete defensive programming, consider:func (s *Storage) Get(key string) ([]byte, error) { if len(key) == 0 { return nil, nil } 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 utils.CopyBytes(v.data), nil }This is a separate encapsulation concern, not the pooled-buffer issue this PR fixes, so it's fine to defer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/storage/memory/memory.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
internal/storage/memory/memory.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Upload results
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
Returning the stored slice directly allows callers to mutate the stored data, which defeats the purpose of the defensive copying in Set(). This completes the fix by ensuring stored data cannot be corrupted either on input (Set) or accessed mutably on output (Get).
This commit backports the defensive copying fixes from PR #3828 and #3829 to the v2 branch to prevent memory corruption from pooled buffers. Changes: 1. internal/memory/memory.go: - Added defensive copying for string keys to prevent corruption from sync.Pool - Added defensive copying for []byte values on both Set and Get - Enhanced package documentation explaining safety guarantees - Optimized GC to skip locking when no items to delete 2. internal/storage/memory/memory.go: - Added defensive copying for string keys and []byte values in Set - Added defensive copying in Get to prevent external mutation - Updated method comments for clarity - Optimized GC to skip locking when no items to delete 3. middleware/csrf/storage_manager.go: - Removed redundant utils.CopyString since storage layer now handles it - Storage layer automatically handles defensive copying 4. middleware/idempotency/idempotency.go: - Removed redundant utils.CopyBytes before marshaling - Marshaling already creates new bytes, so pre-copy was redundant These changes fix intermittent memory corruption issues when strings or byte slices backed by sync.Pool buffers are stored in maps without copying. Related PRs: - #3828 - #3829
Problem
Found while investigating an error reported in the example: https://github.com/simonnix/fiber-csrf-v3-test/blob/main/csrf-v3.sh
The internal memory storage in
internal/storage/memory/memory.gouses string keys and byte slice values directly in a Go map without copying them. When these strings/slices are backed by pooled buffers (fromsync.Poolused by Fiber for performance), the map keys and values can become corrupted when those buffers are reused.Root Cause
sync.Poolextensively for byte buffer reuseObserved Behavior
ginkgo --repeat=100CSRF-...-3599f46c33edaa9(expected:CSRF-...-3599f46c33ed, actual has extraaa9from session ID)Solution
Copy both the key (string) and value ([]byte) before storing in the map using
utils.CopyString()andutils.CopyBytes().Testing
ginkgo --repeat=50(~8% pass rate)ginkgo --repeat=200(100% pass rate)Impact
middleware/csrf/storage_manager.go:82)Files Changed
internal/storage/memory/memory.go- Added defensive copying inSet()method