🐛 Backport defensive copying fixes from #3828 and #3829 to v2#3888
🐛 Backport defensive copying fixes from #3828 and #3829 to v2#3888ReneWerner87 merged 1 commit intov2from
Conversation
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
WalkthroughThe PR enhances the in-memory storage layer by implementing defensive copying for both keys and values in Get/Set operations to prevent external mutations. Additionally, a GC optimization skips locking when no keys expire, and dependent middleware layers are adjusted to leverage the storage layer's copying behavior instead of duplicating the work. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Middleware as Middleware
participant Storage as In-Memory Storage
participant Memory as Internal Memory
rect rgb(230, 240, 250)
Note over Middleware,Storage: Set Operation (Defensive Copy)
Middleware->>Storage: Set(key, value)
Storage->>Memory: CopyString(key)
Storage->>Memory: CopyBytes(value)
Memory->>Memory: Store(copied_key, copied_value)
end
rect rgb(240, 230, 250)
Note over Middleware,Storage: Get Operation (Defensive Copy)
Middleware->>Storage: Get(key)
Storage->>Memory: Retrieve(key)
Memory->>Storage: CopyBytes(stored_value)
Storage->>Middleware: Return copied_value
end
rect rgb(250, 240, 230)
Note over Storage,Memory: GC Optimization
Storage->>Memory: gc()
alt No Expired Keys
Memory->>Storage: Return (skip locking)
else Expired Keys Found
Memory->>Storage: Lock → Cleanup → Unlock
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 backports critical defensive copying fixes to the 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively backports critical defensive copying fixes to prevent memory corruption from pooled buffers. The changes are well-implemented across the internal/memory and internal/storage/memory packages, correctly adding defensive copies for keys and values to ensure data integrity. The accompanying documentation updates are excellent, clearly outlining the safety guarantees. Furthermore, the removal of redundant copy operations in the middleware and the garbage collection optimization are valuable improvements. Overall, this is a high-quality and important contribution. I have one minor suggestion to improve comment clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/storage/memory/memory.go (2)
108-113: Consider making Close idempotent to avoid potential blocking on repeated calls
Closesends once on an unbuffereddonechannel and assumes it’s only ever called a single time; a second call would block indefinitely if the GC goroutine has already exited. If there’s any chance of multipleClosecalls, consider guarding with aselect/default, async.Once, or closing the channel instead of sending.
154-159: Conn documentation clarifies mutability constraints but concurrency caveat remainsThe updated comment makes it explicit that callers must not modify the returned map, which is important given the live backing
db. IfConnis ever used outside single-threaded test scenarios, you might consider returning a shallow copy instead to avoid accidental concurrent map access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/memory/memory.go(4 hunks)internal/storage/memory/memory.go(7 hunks)middleware/csrf/storage_manager.go(1 hunks)middleware/idempotency/idempotency.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
middleware/csrf/storage_manager.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/csrf/storage_manager.go
🧬 Code graph analysis (3)
internal/memory/memory.go (3)
internal/storage/memory/memory.go (1)
Storage(14-19)utils/time.go (1)
Timestamp(13-13)utils/convert.go (2)
CopyBytes(21-25)CopyString(16-18)
middleware/idempotency/idempotency.go (1)
client.go (1)
Response(38-38)
internal/storage/memory/memory.go (1)
utils/convert.go (2)
CopyBytes(21-25)CopyString(16-18)
🔇 Additional comments (14)
middleware/csrf/storage_manager.go (1)
57-58: Delegating key/value copying to the memory storage is correct
internal/memory.Storage.Setnow defensively copies both the string key and[]bytevalue, so dropping the extrautils.CopyStringhere is safe and removes redundant work. The comment accurately documents that this branch relies on the storage layer for defensive copying.middleware/idempotency/idempotency.go (1)
120-120: Using the response body slice directly is safe in this contextHere the body slice is only used to build the msgpack-encoded
bsin the same call and is never stored itself; onlybsis written tocfg.Storage. This makes the extra defensive copy unnecessary while keeping the cached data independent of fasthttp’s internal buffers.internal/storage/memory/memory.go (5)
46-61: Get now safely isolates stored byte slices from callersThe updated
Getmethod correctly treats missing/expired entries asniland returns aCopyBytesclone ofv.data, preventing callers from mutating the underlying storage buffer. This aligns with the defensive-copying goal without changing the external API.
63-83: Set’s defensive copying of key and value fixes sync.Pool aliasing risksCopying the string key with
CopyStringand the[]bytevalue withCopyBytesbefore inserting intodbensures neither map keys nor values can reference sync.Pool-backed buffers or other mutable slices. TTL computation and locking behavior remain consistent with the previous implementation.
87-97: Delete comment now matches behaviorThe new comment accurately states that
Deleteremoves the value stored for the key; implementation is unchanged and still correctly guards with a cheap key-length check and a write lock.
99-106: Reset comment clarifies full map wipe semanticsThe comment now explicitly documents that all keys and values are cleared by swapping in a new map under lock, which matches the existing implementation.
135-138: GC fast-path correctly avoids unnecessary lockingThe
len(expired) == 0check allows skipping the write lock when nothing needs deletion, while the subsequent double-checked deletion unders.mux.Lock()preserves correctness. This is a safe and useful contention reduction.internal/memory/memory.go (7)
1-16: Package-level documentation accurately describes safety and usageThe expanded comment clearly explains defensive copying for string keys and
[]bytevalues, and properly scopes responsibility for other pointer/slice types. This makes the storage’s guarantees and limitations much clearer to internal callers.
34-35: Expiry field comment remains accurate and helps bound TTL expectationsDocumenting the
uint32max timestamp is useful context for anyone configuring very long TTLs, and it still matches howeis computed and compared in GC.
47-66: Get’s byte-slice copy strikes a good balance between safety and performanceReturning
nilfor missing/expired entries preserves existing semantics, while only copying when the stored value is a[]byteprevents corruption from external mutation without penalizing other value types. The atomic timestamp check integrates cleanly with the existing GC scheme.
68-92: Set correctly centralizes defensive copying for keys and byte valuesCopying the string key via
CopyStringand cloning[]bytevalues before storage ensures no map entry can reference sync.Pool-backed or otherwise mutable buffers, which directly addresses the original corruption issue. TTL handling (positive => expiring, non-positive => forever) remains consistent with the new documentation.
94-99: Delete documentation now matches its straightforward behaviorThe comment now explicitly states that the key and its value are removed under a write lock, which matches the implementation and helps make the API clearer.
101-107: Reset behavior is clearly documented and implementation is soundSwapping in a fresh map under lock is an efficient way to drop all entries, and the doc comment now makes this “clear all keys” behavior explicit.
125-128: GC optimization avoids unnecessary locking when there’s nothing to purgeThe
len(expired) == 0check reuses the collectedexpiredslice and safely skips the write lock when no items are due for deletion, keeping the double-checked removal logic intact for non-empty cases.
Description
This PR backports the defensive copying fixes from PR #3828 and #3829 to the v2 branch to prevent memory corruption from pooled buffers.
Background
PR #3828 and #3829 fixed critical memory corruption issues in the main branch where strings or byte slices backed by
sync.Poolbuffers were stored in maps without copying, causing intermittent data corruption when buffers were reused.Changes
1.
internal/memory/memory.gosync.Pool[]bytevalues on bothSetandGet2.
internal/storage/memory/memory.go[]bytevalues inSetGetto prevent external mutation3.
middleware/csrf/storage_manager.goutils.CopyStringsince storage layer now handles it4.
middleware/idempotency/idempotency.goutils.CopyBytesbefore marshalingWhy This Matters
The Problem
Fiber uses
sync.Poolfor buffer reuse. When strings or[]byteslices backed by pooled buffers are stored directly in maps without copying:The Solution
Defensive copying at the storage layer ensures:
Performance Impact
[]bytevaluesTesting
✅ All memory storage tests pass (
internal/memoryandinternal/storage/memory)✅ All CSRF middleware tests pass
✅ All idempotency middleware tests pass
Related PRs
Breaking Changes
None - this is a bug fix that makes storage implementations behave correctly.