🐛 bug: copy FullURL string before returning pooled buffer#4275
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFullURL() now returns the buffer's string via buf.String(); a test was added to ensure FullURL does not alias pooled buffer memory. Generated Ctx/Req interfaces’ IsFromLocal() comments were clarified to specify loopback-IP semantics. ChangesRequest and Context Interface Updates
Sequence Diagram(s)sequenceDiagram
participant DefaultCtx
participant bytebufferpool
participant Test
DefaultCtx->>bytebufferpool: acquire buffer and build URL
DefaultCtx->>DefaultCtx: return buf.String()
Test->>bytebufferpool: mutate pooled buffers repeatedly
Test->>DefaultCtx: assert returned fullURL remains unchanged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![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 updates the documentation for the IsFromLocal method in the Ctx and Req interfaces to clarify that it checks for loopback IPs. Additionally, the FullURL method in req.go was refactored to use buf.String() directly, which is a more standard and efficient way to retrieve the string from a buffer. I have no feedback to provide as there were no review comments.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4275 +/- ##
==========================================
+ Coverage 91.21% 91.26% +0.04%
==========================================
Files 130 130
Lines 12760 12760
==========================================
+ Hits 11639 11645 +6
+ Misses 709 704 -5
+ Partials 412 411 -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:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a pooled-buffer aliasing risk in DefaultCtx.FullURL() by ensuring the returned string cannot reference memory that is returned to bytebufferpool before the caller uses it. It also updates regenerated interface files to reflect updated method documentation.
Changes:
- Make
DefaultCtx.FullURL()return a safe, detached string copy (buf.String()) instead of using the app’s unsafe byte-slice-to-string conversion. - Regenerate
ctx_interface_gen.goandreq_interface_gen.go, updating theIsFromLocaldoc comment to clarify it refers to loopback IPs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| req.go | Fix FullURL() to return a non-aliased string (safe vs pooled buffer reuse). |
| req_interface_gen.go | Regenerated interface comment update for IsFromLocal. |
| ctx_interface_gen.go | Regenerated interface comment update for IsFromLocal. |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/6b1b820e-2612-437e-be11-bb6dc177daa4 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/6b1b820e-2612-437e-be11-bb6dc177daa4 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Addressed in 5227db4. I added a focused regression test in |
![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: 456afdd | Previous: c48bbc2 | Ratio |
|---|---|---|---|
Benchmark_Ctx_FullURL (github.com/gofiber/fiber/v3) - B/op |
48 B/op |
0 B/op |
+∞ |
Benchmark_Ctx_FullURL (github.com/gofiber/fiber/v3) - allocs/op |
1 allocs/op |
0 allocs/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
|
Performance notice is expected since we are creating a string. Previous implementation was returning a buffer that was already deferred. |
Motivation
DefaultCtx.FullURL()where the method returned a string that could alias memory from a pooledbytebufferpoolbuffer that was returned to the pool before the caller could safely use the string.Description
DefaultCtx.FullURL()to return a safe copy usingbuf.String()instead of returningc.app.toString(buf.Bytes()), preventing returned strings from referencing pooled memory that is recycled immediately. (seereq.go).ctx_interface_gen.go,req_interface_gen.go).