🧹 chore: Use sync.Pool for static path sanitization #3972
Conversation
|
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. WalkthroughAdds pooled buffer handling to static middleware sanitization: uses a sync.Pool for temporary buffers, normalizes backslashes to slashes, repeatedly URL-unescapes, rejects NUL bytes, detaches pooled storage to a string before returning, and adds tests + a benchmark for backslash paths. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 @gaby, 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 enhances the performance and memory efficiency of the static file serving middleware by implementing a buffer pooling mechanism for path sanitization. The change specifically targets the handling of paths containing backslashes, ensuring that the conversion to forward slashes is done with reduced memory allocations, thereby improving overall resource utilization. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3972 +/- ##
==========================================
- Coverage 91.59% 91.55% -0.04%
==========================================
Files 119 119
Lines 10194 10213 +19
==========================================
+ Hits 9337 9351 +14
- Misses 543 546 +3
- Partials 314 316 +2
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:
|
![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 introduces a sync.Pool to reuse buffers for path sanitization, which is a good optimization to reduce memory allocations. My review identifies a couple of issues in the implementation. The pool's New function is configured with an incorrect return type, which prevents the optimization from working as intended. Additionally, there is no limit on the size of buffers returned to the pool, creating a risk of memory bloat. I've also included a suggestion to improve the new benchmark test for better accuracy.
There was a problem hiding this comment.
Pull request overview
This PR introduces buffer pooling for static path sanitization to improve performance by reusing memory when normalizing backslashes in file paths. The implementation uses a sync.Pool to reduce allocations during path processing.
Key Changes:
- Adds a
sync.Poolfor byte buffer reuse in backslash normalization - Replaces direct buffer allocation with pooled buffer acquisition
- Includes regression test and benchmark for backslash path handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| middleware/static/static.go | Introduces sync.Pool for buffer reuse in sanitizePath function with pooled buffer acquisition and management |
| middleware/static/static_test.go | Adds regression test Test_Static_BackslashRequests and benchmark Benchmark_Static_BackslashRequests to validate backslash path handling |
![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
🧹 Nitpick comments (1)
middleware/static/static.go (1)
44-53: Consider simplifying the nil check.After a successful type assertion to
*[]byte,itemcannot benil(the assertion would have failed). The checkitem == nilon line 45 is redundant. However, this is a minor clarity issue.🔎 Proposed simplification
- item, ok := sanitizeBufPool.Get().(*[]byte) - if !ok || item == nil { - item = new([]byte) - } - bufPtr = item + bufPtr, _ = sanitizeBufPool.Get().(*[]byte) + if bufPtr == nil { + bufPtr = new([]byte) + }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/static/static.gomiddleware/static/static_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
middleware/static/static_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/static/static_test.gomiddleware/static/static.go
🧠 Learnings (1)
📚 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/static/static_test.go
🧬 Code graph analysis (1)
middleware/static/static_test.go (2)
middleware/static/static.go (1)
New(108-254)constants.go (1)
MIMETextCSSCharsetUTF8(37-37)
⏰ 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: Agent
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: lint
🔇 Additional comments (3)
middleware/static/static.go (1)
55-65: LGTM!The backslash replacement, string conversion (which safely copies before pool return), and buffer reset logic are correctly implemented. The else branch appropriately maintains the existing fast path using
utils.UnsafeStringwhen no backslashes are present.middleware/static/static_test.go (2)
399-416: LGTM!The test correctly invokes
t.Parallel()per coding guidelines. The 3-iteration loop is a sensible regression test to validate that pooled buffer reuse doesn't corrupt subsequent requests. Assertions cover status, content type, and body content appropriately.
1145-1162: LGTM!The benchmark is well-structured with
b.ReportAllocs()andb.ResetTimer()correctly positioned. Usingb.Loop()aligns with modern Go benchmarking patterns. Properly closing the response body prevents resource leaks during benchmark execution.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
middleware/static/static.go (2)
97-100: Consider adding a size limit when returning buffers to the pool.The code returns buffers of any size to the pool (line 99), which can cause memory bloat if very long paths are processed. Large buffers remain in the pool indefinitely since
sync.Pooldoesn't shrink or impose size limits.🔎 Proposed fix
if buf != nil { s = string([]byte(s)) // detach from pooled storage before reuse - sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + if cap(buf) <= 4096 { // Avoid pooling overly large buffers + sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + } }
98-98: The detach operation may be unnecessary.At line 98,
shas already been processed bypathpkg.Clean("/" + s)at line 80, which creates a new string through concatenation. This meanssis already detached frombuf's underlying storage, making the explicit detachstring([]byte(s))redundant and causing an unnecessary allocation.🔎 Proposed simplification
You could safely remove line 98 since
sis guaranteed to be detached after line 80's string concatenation:if buf != nil { - s = string([]byte(s)) // detach from pooled storage before reuse sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional }However, keeping it provides defensive safety at the cost of a small allocation, so this is optional.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/static/static.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/static/static.go
🧠 Learnings (2)
📚 Learning: 2024-10-02T23:02:12.306Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-02T23:02:12.306Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/static/static.go
📚 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/static/static.go
🧬 Code graph analysis (1)
middleware/static/static.go (1)
app.go (1)
New(522-635)
⏰ 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)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
middleware/static/static.go (2)
22-30: LGTM: Pool type is now consistent.The pool correctly returns
[]byte(line 27) which matches the type assertion at line 44. The initial capacity of 64 bytes is reasonable for typical static file paths.
51-58: LGTM: Backslash sanitization logic is correct.The buffer copy and in-place backslash-to-forward-slash replacement is efficient. Using
utils.UnsafeStringat line 58 aligns with fasthttp's zero-allocation philosophy and is safe here since the string is immediately processed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
middleware/static/static.go (2)
70-85: Resource leak: pooled buffer not returned on early error returns.When the path contains backslashes,
bufis obtained from the pool. If the unescape fails (line 74) or a null byte is detected (line 84), the function returns early without returningbufto the pool, causing buffer leakage.🔎 Proposed fix
// repeatedly unescape until it no longer changes, catching errors for strings.IndexByte(s, '%') >= 0 { us, err := url.PathUnescape(s) if err != nil { + if buf != nil { + sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + } return nil, ErrInvalidPath } if us == s { break } s = us } // reject any null bytes if strings.IndexByte(s, '\x00') >= 0 { + if buf != nil { + sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + } return nil, ErrInvalidPath }
89-98: Resource leak: pooled buffer not returned on filesystem validation returns.Similar to the previous issue, early returns at lines 92 and 95 do not return
bufto the pool when a path with backslashes triggers pool usage.🔎 Proposed fix
if filesystem != nil { s = utils.TrimLeft(s, '/') if s == "" { + if buf != nil { + sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + } return []byte("/"), nil } if !fs.ValidPath(s) { + if buf != nil { + sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + } return nil, ErrInvalidPath } s = "/" + s }Consider extracting the buffer return logic to a helper function or using
deferwith a cleanup function to reduce repetition across all return paths.
🧹 Nitpick comments (1)
middleware/static/static.go (1)
104-107: Add size limit to prevent memory bloat from large buffers.The pool returns buffers of any size. A request with a very long path allocates a large buffer that gets stored in the pool indefinitely. Since
sync.Pooldoesn't shrink, this can lead to memory bloat over time.🔎 Proposed fix with size limit
if buf != nil { s = string([]byte(s)) // detach from pooled storage before reuse - sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + if cap(buf) <= 4096 { // Avoid pooling overly large buffers + sanitizeBufPool.Put(buf[:0]) //nolint:staticcheck // []byte reuse is intentional + } }Note: The detachment
string([]byte(s))on line 105 is defensive but likely redundant sincesis already a new allocation afterpathpkg.Clean("/" + s)on line 87. However, keeping it as a safety measure is reasonable.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/static/static.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/static/static.go
🧠 Learnings (2)
📚 Learning: 2024-10-02T23:02:12.306Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-02T23:02:12.306Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/static/static.go
📚 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/static/static.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). (4)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: lint
🔇 Additional comments (2)
middleware/static/static.go (2)
22-30: LGTM on pool type consistency.The pool's
Newfunction now correctly returns[]byte, matching the type assertion on line 45. The initial capacity of 64 bytes is reasonable for typical paths.
44-67: Buffer pooling logic is correctly implemented.The code properly handles:
- Type assertion matching the pool's return type
- Returning the original pooled buffer before allocating a larger one (lines 52-55)
- Tracking pooled vs newly-allocated buffers via
pooledBufThe use of
utils.UnsafeStringon line 65 is acceptable since the value is used immediately within the same call stack. Based on learnings, this aligns with fasthttp's zero-allocation design philosophy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/static/static.go (1)
111-114: Defensive copy is likely unnecessary but acceptable.Line 112 creates a safe copy via
string([]byte(s))before returning the buffer to the pool. However, by this pointshas already been transformed by:
url.PathUnescape(line 79) - creates new stringpathpkg.Clean("/" + s)(line 94) - concatenation creates new string, Clean returns new string- Potential additional transformations (lines 97-109)
After line 94's concatenation,
sshould already be detached from the pooled buffer. The defensive copy trades a small allocation for guaranteed safety, which is reasonable for this code path.If you want to make this more idiomatic and potentially more efficient, consider using
strings.Clone(s)(available in Go 1.18+) instead ofstring([]byte(s)).💡 Optional: Use strings.Clone for clearer intent
If the project uses Go 1.18+, you could replace line 112 with:
- s = string([]byte(s)) // detach from pooled storage before reuse + s = strings.Clone(s) // detach from pooled storage before reuseThis more clearly expresses the intent and may be optimized by the compiler.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/static/static.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/static/static.go
🧠 Learnings (2)
📚 Learning: 2024-10-02T23:02:12.306Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-02T23:02:12.306Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/static/static.go
📚 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/static/static.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). (4)
- GitHub Check: Compare
- GitHub Check: lint
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (4)
middleware/static/static.go (4)
22-30: LGTM! Pool configuration is type-consistent.The
sync.Poolcorrectly returns[]bytevalues, which matches the type assertion at line 53. The initial 64-byte capacity is reasonable for typical path lengths.
34-38: LGTM! Proper buffer size limiting.The 8KB threshold prevents large buffers from accumulating in the pool, which is good practice to avoid memory bloat. Resetting the buffer length to zero before reuse is correct.
52-64: LGTM! Proper buffer acquisition and capacity handling.The logic correctly:
- Retrieves a buffer from the pool with proper type assertion
- Tracks the pooled buffer separately in
pooledBuf- Returns the original pooled buffer before allocating a new one when capacity is insufficient (lines 60-61)
This properly addresses the resource leak concern from previous reviews.
65-75: LGTM! Efficient buffer usage with proper safety handling.The
utils.UnsafeString(buf)at line 72 efficiently avoids an allocation. The stringsinitially shares the buffer's underlying bytes, but:
- The buffer is not modified after line 71
- A safe copy is created at line 112 before the buffer is returned to the pool
- String operations at lines 79, 94, etc. create new strings, detaching from the buffer
The safety strategy is sound.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a sync.Pool to reduce memory allocations during static path sanitization, which is a good performance improvement. The changes in middleware/static/static.go correctly implement the pooling pattern, including detaching the string from the pooled buffer to prevent data races. I've made a few suggestions to simplify the buffer handling logic and remove a redundant check.
The new tests and benchmarks in middleware/static/static_test.go are valuable for verifying the changes. However, I found a critical issue in the new benchmark's loop syntax that will prevent it from compiling. Please see my detailed comments.
| if bytes.IndexByte(p, '\\') >= 0 { | ||
| b := make([]byte, len(p)) | ||
| copy(b, p) | ||
| for i := range b { | ||
| if b[i] == '\\' { | ||
| b[i] = '/' | ||
| buf, ok = sanitizeBufPool.Get().([]byte) | ||
| if !ok { | ||
| buf = nil | ||
| } else { | ||
| pooledBuf = buf | ||
| } | ||
| if cap(buf) < len(p) { | ||
| if pooledBuf != nil { | ||
| putSanitizeBuf(pooledBuf) | ||
| } | ||
| buf = make([]byte, len(p)) | ||
| } | ||
| buf = buf[:len(p)] | ||
| copy(buf, p) | ||
| for i := range buf { | ||
| if buf[i] == '\\' { | ||
| buf[i] = '/' | ||
| } | ||
| } | ||
| s = utils.UnsafeString(b) | ||
| s = utils.UnsafeString(buf) |
There was a problem hiding this comment.
Critical design flaw: The pooled buffer is acquired at lines 53-64 but only returned to the pool at line 113. There are multiple error return paths between these lines (lines 81, 92, 99, 102 in the full function) that would leak the buffer. Additionally, using UnsafeString creates a string backed by the buffer's memory while the buffer could potentially be returned to the pool and reused, risking data corruption. The correct approach is to either: 1) use string(buf) instead of UnsafeString(buf) for a safe copy and immediately return the buffer to the pool, or 2) use defer to ensure the buffer is always returned to the pool on all code paths.
| s = string([]byte(s)) // detach from pooled storage before reuse | ||
| putSanitizeBuf(buf) |
There was a problem hiding this comment.
The detachment logic here is ineffective because by this point in the code, the string 's' has already been reallocated by earlier operations, specifically the string concatenation at line 94 (pathpkg.Clean "/" + s in the full function). This means 's' is no longer backed by the pooled buffer, making this detachment unnecessary. The detachment should either happen immediately after line 72 where UnsafeString is used, or the code should use string(buf) instead of UnsafeString(buf) for a safe copy from the start.
middleware/static/static.go
Outdated
| buf, ok = sanitizeBufPool.Get().([]byte) | ||
| if !ok { | ||
| buf = nil | ||
| } else { | ||
| pooledBuf = buf | ||
| } | ||
| if cap(buf) < len(p) { | ||
| if pooledBuf != nil { | ||
| putSanitizeBuf(pooledBuf) | ||
| } | ||
| buf = make([]byte, len(p)) | ||
| } |
There was a problem hiding this comment.
The type assertion handling is unnecessarily complex. If the type assertion fails, setting buf to nil and then immediately checking if cap(buf) < len(p) will panic because cap(nil) is valid but the code doesn't need this branch. Since the pool's New function always returns []byte, the type assertion should never fail in practice. Consider simplifying this by removing the type assertion check, or at minimum handle the failure case by allocating a new buffer directly instead of setting buf to nil.
| buf, ok = sanitizeBufPool.Get().([]byte) | |
| if !ok { | |
| buf = nil | |
| } else { | |
| pooledBuf = buf | |
| } | |
| if cap(buf) < len(p) { | |
| if pooledBuf != nil { | |
| putSanitizeBuf(pooledBuf) | |
| } | |
| buf = make([]byte, len(p)) | |
| } | |
| bufAny := sanitizeBufPool.Get() | |
| buf, ok = bufAny.([]byte) | |
| if !ok { | |
| buf = make([]byte, len(p)) | |
| } else if cap(buf) < len(p) { | |
| putSanitizeBuf(buf) | |
| buf = make([]byte, len(p)) | |
| } else { | |
| pooledBuf = buf | |
| } |
| }, | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
The constant value 8KB seems arbitrary and lacks justification. Consider adding a comment explaining why this specific size limit was chosen for pooled buffers, especially in the context of typical path lengths in web applications.
| // sanitizeBufPoolMaxSize caps the size of buffers we keep in the pool. | |
| // 8KB is a conservative upper bound for typical URL/path lengths in web applications: | |
| // it comfortably covers normal request paths while avoiding retention of unusually large | |
| // buffers, which could otherwise increase memory usage if pooled. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/static/static.go (1)
77-109: Critical: Pooled buffer leaked on early returns.Lines 81, 92, 99, and 102 return early without releasing the pooled buffer acquired at line 53. High-traffic scenarios with invalid paths will leak buffers from the pool, increasing allocation pressure and defeating the pool's purpose.
🔎 Recommended fix using defer
Insert a deferred cleanup immediately after buffer acquisition:
if bytes.IndexByte(p, '\\') >= 0 { buf, ok = sanitizeBufPool.Get().([]byte) if !ok { buf = nil } else { pooledBuf = buf } if cap(buf) < len(p) { if pooledBuf != nil { putSanitizeBuf(pooledBuf) } buf = make([]byte, len(p)) } + defer func() { + if buf != nil { + putSanitizeBuf(buf) + } + }() buf = buf[:len(p)] copy(buf, p) for i := range buf { if buf[i] == '\\' { buf[i] = '/' } } s = utils.UnsafeString(buf) } else { s = utils.UnsafeString(p) }Then remove the manual cleanup at lines 111-114:
- if buf != nil { - s = string([]byte(s)) // detach from pooled storage before reuse - putSanitizeBuf(buf) - } - return utils.UnsafeBytes(s), nil
♻️ Duplicate comments (1)
middleware/static/static.go (1)
111-114: Unnecessary allocation: detachment is redundant.Line 112's detachment
string([]byte(s))is unnecessary. By this point,shas been reallocated byPathUnescape(line 79) andpathpkg.Clean(line 94), so it no longer references the pooled buffer. This creates an extra copy on every backslash-containing request.The detachment can be removed entirely:
if buf != nil { - s = string([]byte(s)) // detach from pooled storage before reuse putSanitizeBuf(buf) }Note: This comment assumes the critical buffer leak issue is addressed. If using the defer-based fix suggested above, lines 111-114 would be removed entirely.
🧹 Nitpick comments (1)
middleware/static/static.go (1)
32-32: Consider documenting the buffer size limit.The 8KB limit lacks justification. Adding a comment would help future maintainers understand the rationale.
📝 Suggested documentation
+// sanitizeBufPoolMaxSize caps pooled buffer size to prevent memory bloat. +// 8KB covers typical URL/path lengths while avoiding retention of unusually large buffers. const sanitizeBufPoolMaxSize = 8 * 1024
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/static/static.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/static/static.go
🧠 Learnings (2)
📚 Learning: 2024-10-02T23:02:12.306Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-02T23:02:12.306Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/static/static.go
📚 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/static/static.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). (5)
- GitHub Check: lint
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
middleware/static/static.go (1)
52-71: Critical: Resource leak on error paths.The pooled buffer acquired at line 53 is only returned to the pool at line 109, but there are multiple early return statements that bypass this cleanup (lines 77, 87, 95, 98). Each error path leaks a buffer from the pool, eventually degrading performance.
🔎 Recommended fix using defer
if bytes.IndexByte(p, '\\') >= 0 { buf, ok := sanitizeBufPool.Get().([]byte) if !ok { buf = make([]byte, 0, 64) } pooledBuf = buf + defer func() { + if buf != nil { + // Ensure string is detached from pooled storage + putSanitizeBuf(buf) + } + }() if cap(buf) < len(p) { if pooledBuf != nil { putSanitizeBuf(pooledBuf) } buf = make([]byte, len(p)) } buf = buf[:len(p)] copy(buf, p) for i := range buf { if buf[i] == '\\' { buf[i] = '/' } } s = utils.UnsafeString(buf) } else { s = utils.UnsafeString(p) }Then remove the manual Put at lines 107-110 since defer handles it:
- if buf != nil { - s = string([]byte(s)) // detach from pooled storage before reuse - putSanitizeBuf(buf) - } - return utils.UnsafeBytes(s), nilNote: You'll also need to ensure
sis detached from the buffer before the defer runs. Since the string operations (PathUnescape, Clean, concatenation) between lines 74-104 create new string allocations,sshould already be detached by the time defer executes. However, to be absolutely safe, you could dos = strings.Clone(s)before the return if using Go 1.18+, or keep a simpler detachment in the defer itself.
🧹 Nitpick comments (2)
middleware/static/static.go (2)
32-32: Add documentation for the buffer size limit.The 8KB limit is reasonable for typical web application paths, but consider adding a comment explaining this choice to improve code maintainability.
🔎 Suggested documentation
+// sanitizeBufPoolMaxSize caps the size of buffers retained in the pool. +// 8KB accommodates typical URL/path lengths while preventing memory bloat +// from unusually large paths. const sanitizeBufPoolMaxSize = 8 * 1024
107-110: Consider simplifying the string detachment.The expression
string([]byte(s))at line 108 performs two conversions to detach the string from pooled storage. While it works, it's somewhat convoluted.Note: This cleanup block is only reached on the success path. The resource leak on error paths (flagged separately) must be addressed first, likely using
deferwhich would move this logic.Alternative approaches
If using Go 1.18+:
s = strings.Clone(s)Or more explicitly:
s = string(append([]byte(nil), s...))However, in practice, the intermediate string operations (PathUnescape, Clean, concatenation) at lines 74-104 likely create new allocations anyway, making this detachment defensive rather than strictly necessary.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/static/static.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/static/static.go
🧠 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/static/static.go
📚 Learning: 2024-10-02T23:02:12.306Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-02T23:02:12.306Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/static/static.go
🪛 GitHub Actions: golangci-lint
middleware/static/static.go
[error] 46-46: golangci-lint: declared and not used: ok (typecheck)
🪛 GitHub Actions: Modernize Lint
middleware/static/static.go
[error] 46-46: Command 'go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test=false ./...' failed: declared and not used: ok
🪛 GitHub Actions: Run govulncheck
middleware/static/static.go
[error] 46-46: declared and not used: ok
🪛 GitHub Check: govulncheck-check
middleware/static/static.go
[failure] 46-46:
declared and not used: ok
🪛 GitHub Check: lint
middleware/static/static.go
[failure] 46-46:
declared and not used: ok (typecheck)
🪛 GitHub Check: modernize
middleware/static/static.go
[failure] 46-46:
declared and not used: ok
🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
middleware/static/static.go
[failure] 46-46:
declared and not used: ok
⏰ 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: Compare
- GitHub Check: repeated
🔇 Additional comments (2)
middleware/static/static.go (2)
22-30: LGTM! The buffer pool initialization is correct.The
sync.Poolcorrectly returns[]bytevalue type with an initial capacity of 64 bytes, which matches the type assertion used later at line 53.
34-38: LGTM! The buffer return logic is correct.The function properly checks buffer capacity before pooling and resets the length to 0, preventing both memory bloat and buffer content leakage.
| var ( | ||
| buf []byte | ||
| pooledBuf []byte | ||
| ok bool | ||
| s string | ||
| ) |
There was a problem hiding this comment.
Fix the unused variable causing build failures.
The variable ok is declared but never used in the type assertion at line 53, causing all pipeline checks to fail. The type assertion should check the boolean return value.
🔎 Proposed fix
var (
buf []byte
pooledBuf []byte
- ok bool
s string
)And then at line 53, handle the assertion properly:
if bytes.IndexByte(p, '\\') >= 0 {
- buf = sanitizeBufPool.Get().([]byte)
+ buf, ok := sanitizeBufPool.Get().([]byte)
+ if !ok {
+ buf = make([]byte, 0, 64)
+ }
pooledBuf = bufCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: golangci-lint
[error] 46-46: golangci-lint: declared and not used: ok (typecheck)
🪛 GitHub Actions: Modernize Lint
[error] 46-46: Command 'go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test=false ./...' failed: declared and not used: ok
🪛 GitHub Actions: Run govulncheck
[error] 46-46: declared and not used: ok
🪛 GitHub Check: govulncheck-check
[failure] 46-46:
declared and not used: ok
🪛 GitHub Check: lint
[failure] 46-46:
declared and not used: ok (typecheck)
🪛 GitHub Check: modernize
[failure] 46-46:
declared and not used: ok
🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
[failure] 46-46:
declared and not used: ok
🤖 Prompt for AI Agents
In middleware/static/static.go around lines 43 to 48, the variable `ok` is
declared but never used which causes build failures; update the type assertion
at line 53 to capture the boolean return value (e.g., assign to a variable and
check it) and then branch on that boolean so `ok` is used (or remove the unused
`ok` declaration and replace the assertion with the proper two-value form and a
check), ensuring the code handles the failed assertion path appropriately.
Summary
Related #3767