🐛 bug: fix copying of key/values in internal/memory #3829
Conversation
- 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.
|
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/Set in internal memory storage now defensively copy keys and byte-slice values; CSRF middleware stopped copying keys/values and forwards raw data to storage; idempotency middleware stopped copying the response body and uses the original response body slice. Changes
Sequence Diagram(s)(Skipped — changes are local, focusing on defensive-copying and value flow rather than altering control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (4)
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 addresses a critical memory corruption issue stemming from the reuse of 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.
Pull Request Overview
This PR refactors defensive copying logic in memory storage implementations by moving the responsibility from callers to the storage layer itself. The changes ensure data integrity while eliminating redundant copying operations.
Key changes:
- Moved defensive copying of keys and byte values into the storage
SetandGetmethods - Removed now-redundant copying calls from middleware that uses these storage implementations
- Enhanced documentation explaining safety guarantees and when defensive copies occur
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
middleware/idempotency/idempotency.go |
Removed redundant utils.CopyBytes call for response body since storage now handles copying |
middleware/csrf/storage_manager.go |
Removed redundant utils.CopyString and utils.CopyBytes calls, fixed typo "too" → "to", removed unused utils import |
internal/storage/memory/memory.go |
Added defensive copying in Set for keys/values and in Get for values to prevent external mutation |
internal/memory/memory.go |
Added defensive copying for string keys and []byte values in Set and Get, enhanced package documentation |
Comments suppressed due to low confidence (1)
internal/memory/memory.go:99
- The
Deletemethod uses the originalkeyparameter to look up entries, butSetnow stores entries usingutils.CopyString(key). If the original key is pooled and reused, the string values may differ, causing Delete to fail to find and remove the entry. The key should be copied here as well:keyCopy := utils.CopyString(key)then usedelete(s.data, keyCopy).
func (s *Storage) Delete(key string) {
s.Lock()
delete(s.data, key)
s.Unlock()
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses potential memory corruption issues by extending defensive copying to the internal/memory and internal/storage/memory packages. The changes, which involve copying string keys and []byte values, are implemented correctly and consistently. The removal of now-redundant copying logic from the CSRF and Idempotency middleware is a logical consequence that simplifies the codebase. The new package-level documentation in internal/memory/memory.go is particularly noteworthy, as it clearly outlines the safety guarantees and responsibilities of the caller. Overall, this is a high-quality contribution that enhances the framework's robustness and maintainability.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3829 +/- ##
==========================================
- Coverage 92.26% 92.19% -0.07%
==========================================
Files 115 115
Lines 9742 9742
==========================================
- Hits 8988 8982 -6
- Misses 480 485 +5
- Partials 274 275 +1
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:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 7f2946c | Previous: 6ef6de5 | Ratio |
|---|---|---|---|
Benchmark_Memory/fiber_memory |
255927 ns/op 126443 B/op 6000 allocs/op |
141180 ns/op 24023 B/op 1000 allocs/op |
1.81 |
Benchmark_Memory/fiber_memory - ns/op |
255927 ns/op |
141180 ns/op |
1.81 |
Benchmark_Memory/fiber_memory - B/op |
126443 B/op |
24023 B/op |
5.26 |
Benchmark_Memory/fiber_memory - allocs/op |
6000 allocs/op |
1000 allocs/op |
6 |
Benchmark_Limiter - B/op |
16 B/op |
8 B/op |
2 |
Benchmark_Limiter - allocs/op |
2 allocs/op |
1 allocs/op |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
gaby
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/memory/memory.go(2 hunks)internal/storage/memory/memory.go(2 hunks)middleware/idempotency/idempotency.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/memory/memory.go
🧰 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:
middleware/idempotency/idempotency.gointernal/storage/memory/memory.go
🪛 GitHub Actions: golangci-lint
internal/storage/memory/memory.go
[error] 93-93: undefined: entry (typecheck) reported by golangci-lint during 'golangci-lint run'.
🪛 GitHub Actions: Modernize Lint
internal/storage/memory/memory.go
[error] 93-93: undefined: entry
🪛 GitHub Actions: Run govulncheck
internal/storage/memory/memory.go
[error] 93-93: govulncheck failed: undefined: entry in memory.go:93
🪛 GitHub Check: govulncheck-check
internal/storage/memory/memory.go
[failure] 93-93:
undefined: entry
🪛 GitHub Check: lint
internal/storage/memory/memory.go
[failure] 93-93:
undefined: entry (typecheck)
🪛 GitHub Check: modernize
internal/storage/memory/memory.go
[failure] 93-93:
undefined: entry
[failure] 93-93:
undefined: entry
🪛 GitHub Check: unit (1.25.x, macos-latest)
internal/storage/memory/memory.go
[failure] 93-93:
undefined: entry
🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
internal/storage/memory/memory.go
[failure] 93-93:
undefined: entry
⏰ 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). (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (1)
middleware/idempotency/idempotency.go (1)
138-138: LGTM! Redundant copy correctly removed.Removing
utils.CopyBytesbefore marshaling is appropriate sinceres.MarshalMsg(nil)on line 161 allocates new memory. The storage layer now handles defensive copying of the marshaled bytes.
- Use the exported Entry type when inserting into storage (fixes incorrect `entry` identifier). - Clean up unnecessary blank lines in internal memory implementations.
Description
This PR extends the defensive copying fix from #3828 to the storage implementation and removes now-redundant defensive copying from middleware that rely on these storage layers.
Background
PR #3828 fixed memory corruption in (the interface implementation) by adding defensive copying for string keys and values. However, the same issue exists in , which is used by cache, limiter, and CSRF middleware.
Changes
1.
internal/memory/memory.goApplies the same defensive copying pattern as #3828:
Set()usingutils.CopyString()[]bytevalues inSet()to prevent input corruption[]bytevalues inGet()to prevent output mutation2.
internal/storage/memory/memory.goGet()method (suggested by AI reviewers in Fix: Prevent memory corruption in internal memory storage from pooled buffers #3828)utils.CopyBytes(v.data)instead of direct reference3. Middleware Cleanup
Now that storage layers handle defensive copying, removed redundant operations:
CSRF (
middleware/csrf/storage_manager.go):utils.CopyString(key)andutils.CopyBytes(raw)beforem.memory.Set()Idempotency (
middleware/idempotency/idempotency.go):utils.CopyBytes(c.Response().Body())before marshalingCache (
middleware/cache/cache.go):[]bytefields in structs[]bytevalues, not fields inside structsWhy 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
[]bytevaluesRelated
internal/storage/memoryBreaking Changes
None - this is a bug fix that makes storage implementations behave correctly and simplifies middleware code.