🐛 bug: Fix nil pointer dereference in context methods when accessed after release#4062
🐛 bug: Fix nil pointer dereference in context methods when accessed after release#4062
Conversation
Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
|
@ReneWerner87 the check is nice to merge, but the problem in 4061 is related to context usage i think. |
Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
|
ok, lets wait for the comment of the bug author |
|
@copilot When user passes c to GetObject the issue happens. But if they pass c.RequestCtx() it doesnt. |
You're correct - The proper recommendation should be for users to call However, the nil check in
Would you prefer we also add documentation recommending |
|
@copilot That doesnt answer my question. The fasthttp requestctx also implemente Value() here https://github.com/valyala/fasthttp/blob/master/server.go#L2868
|
... You're absolutely right, and I apologize for the confusion. Here's what's happening: When you pass When you pass The nil check prevents the panic, but you're correct that the root cause is the lifecycle difference:
For minio specifically, |
|
@copilot Make sure we check for nil in any place calling c.fasthttp to avoid this issue further on. |
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Added nil checks to all context-related methods that access
These are the methods most likely to be called from goroutines or after handler completion. Updated tests to verify defensive behavior instead of expecting panics. Commit: 1fcab44 |
|
@copilot You need to run tests with "-race" |
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Fixed the race condition in All tests now pass with
Commit: 9f670d6 Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4062 +/- ##
==========================================
- Coverage 91.11% 91.10% -0.01%
==========================================
Files 119 119
Lines 11207 11217 +10
==========================================
+ Hits 10211 10219 +8
- Misses 630 631 +1
- Partials 366 367 +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:
|
|
@copilot Is it possible to wait for the stream to finish after calling SendStream before ReleaseCtx gets called |
Technically possible but challenging. Here's why: Problem: Challenges:
Better approaches:
The nil checks are the pragmatic solution - they prevent the crash while users should follow the documented pattern of using |
There was a problem hiding this comment.
Pull request overview
This PR hardens fiber.Ctx’s context.Context-related methods against use-after-release by adding defensive nil checks when the underlying *fasthttp.RequestCtx has been cleared during ReleaseCtx, preventing nil-pointer panics in goroutines (e.g., streaming/object downloads).
Changes:
- Add defensive
nilchecks inDefaultCtxmethods that accessc.fasthttp(Context,SetContext,Value,Request,Response). - Add/adjust unit tests to verify post-release behavior does not panic, including a goroutine scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ctx.go | Adds c.fasthttp == nil guards to prevent panics after ReleaseCtx. |
| ctx_test.go | Adds tests validating Value(), Context(), Request()/Response() behavior after release and from a goroutine. |
| // Request return the *fasthttp.Request object | ||
| // This allows you to use all fasthttp request methods | ||
| // https://godoc.org/github.com/valyala/fasthttp#Request | ||
| func (c *DefaultCtx) Request() *fasthttp.Request { | ||
| if c.fasthttp == nil { |
There was a problem hiding this comment.
Request() can now return nil when c.fasthttp is nil (after ReleaseCtx). The doc comment currently implies it always returns a *fasthttp.Request. Please update the comment to mention the nil return after release so callers understand the behavior change and avoid nil dereferences.
| // Response return the *fasthttp.Response object | ||
| // This allows you to use all fasthttp response methods | ||
| // https://godoc.org/github.com/valyala/fasthttp#Response | ||
| func (c *DefaultCtx) Response() *fasthttp.Response { | ||
| if c.fasthttp == nil { |
There was a problem hiding this comment.
Response() can now return nil when c.fasthttp is nil (after ReleaseCtx). The doc comment currently implies it always returns a *fasthttp.Response. Please update the comment to mention the nil return after release so callers understand the behavior change and avoid nil dereferences.
ctx.go
Outdated
| @@ -592,6 +604,9 @@ | |||
| // Value makes it possible to retrieve values (Locals) under keys scoped to the request | |||
| // and therefore available to all following routes that match the request. | |||
There was a problem hiding this comment.
Value() now returns nil when c.fasthttp is nil (after ReleaseCtx). The comment describes retrieving Locals for the request, but doesn’t mention the post-release fallback behavior. Consider updating the comment to note that it returns nil once the context has been released.
| // and therefore available to all following routes that match the request. | |
| // and therefore available to all following routes that match the request. If the context | |
| // has been released and c.fasthttp is nil (for example, after ReleaseCtx), Value returns nil. |
ctx_test.go
Outdated
| // Give time for context to be released | ||
| time.Sleep(50 * time.Millisecond) | ||
|
|
There was a problem hiding this comment.
Test_Ctx_Value_InGoroutine uses a fixed time.Sleep(50 * time.Millisecond) to “give time for context to be released”. This makes the test slower and can be flaky on slow/fast environments. Since app.Test(...) returns only after the handler (and its deferred ReleaseCtx) completes, the goroutine can be unblocked immediately after app.Test returns (or use a deterministic sync point tied to the handler completion) instead of sleeping.
| // Give time for context to be released | |
| time.Sleep(50 * time.Millisecond) |
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Applied all changes from the code review:
All tests pass including with Commit: 032385e |
|
@copilot we do not make it clear in the documentation that c.SendStream() closes the provided stream (if possible). We should also document the async behavior of that function from fasthttp. Let's document this behavior under docs/ctx.md |
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Added comprehensive documentation to
Markdown linting passes. Commit: a86af2f |
Description
When
fiber.Ctxis passed ascontext.Contextto libraries that spawn goroutines (e.g.,minio.GetObject), context-related methods panic with nil pointer dereference. This occurs becausec.fasthttpis set tonilwhen the context is released back to the pool after the request handler completes (viadefer app.ReleaseCtx(ctx)inrouter.go), but goroutines may still call these methods.The root cause is a lifecycle mismatch:
SendStreamsets up streaming but doesn't block, so the handler returns immediately, triggering the deferred context release. Fasthttp then reads from the stream after the context has been released andc.fasthttpis nil.Note: While this fix prevents panics through defensive nil checks, users should call
c.Context()instead of passingcdirectly to avoid lifecycle issues with goroutines. The existing documentation already recommends this pattern.Alternative: Users can also pass
c.RequestCtx()which returns the underlying*fasthttp.RequestCtxthat persists during streaming, thoughc.Context()is the recommended approach for general use.Changes introduced
c.fasthttp:Value()- Returnsnilwhenc.fasthttpisnil(prevents panic, maintainscontext.Contextcontract)Context()- Returnscontext.Background()whenc.fasthttpisnilSetContext()- Becomes no-op whenc.fasthttpisnilRequest()- Returnsnilwhenc.fasthttpisnilResponse()- Returnsnilwhenc.fasthttpisnilRequest(),Response(), andValue()methods to explicitly document nil return behavior when context has been releasedSendStreamdocumentation in/docs/api/ctx.mdto clarify:io.Closerare automatically closed by fasthttpTest_Ctx_Value_AfterRelease- VerifiesValue()doesn't panic after releaseTest_Ctx_Value_InGoroutine- Tests goroutine usage with deterministic synchronization (no sleeps) to avoid race conditionsTest_Ctx_Request_Response_AfterRelease- VerifiesRequest()/Response()return nil safelyTest_Ctx_Context_AfterHandlerPanicsto expect defensive behavior instead of panicsModified Files:
ctx.go: Added nil checks to 5 methods accessingc.fasthttpand updated godoc comments to clarify nil return behaviorctx_test.go: Added 3 new tests and updated 1 existing test to verify defensive behavior with race-safe, deterministic synchronizationdocs/api/ctx.md: Enhanced SendStream documentation with async behavior, stream closing, and lifecycle warningsBenchmarks: No performance impact - nil checks are single comparisons in non-hot paths
Documentation Update: Updated godoc comments for
Request(),Response(), andValue()methods. EnhancedSendStreamdocumentation in/docs/api/ctx.mdto document async behavior, automatic stream closing, and proper context usage patterns with goroutines. Updated context guide already documentedc.Context()recommendation for long-running operations.Changelog/What's New: Fixed nil pointer dereference in context methods (
Value(),Context(),SetContext(),Request(),Response()) when accessed after handler completion. Improved documentation forSendStreamto clarify async behavior and stream lifecycle. Users should usec.Context()for operations in goroutines to avoid lifecycle issues.Migration Guide: No migration needed - transparent fix that provides defensive behavior instead of panics. However, users passing
cdirectly to libraries spawning goroutines should migrate toc.Context()for proper lifecycle management. Documentation now clearly explains streaming behavior and context lifecycle.API Alignment with Express: N/A - Go-specific context.Context interface compliance
API Longevity: Maintains backward compatibility, provides safer behavior for goroutine usage. The nil checks are defensive measures; proper usage is documented. Documentation clarifies expected behavior for edge cases and async operations.
Examples: See issue reproduction code. Recommended pattern:
obj, _ := minioClient.GetObject(c.Context(), "bucket", "file", opts). Added file streaming example in SendStream documentation demonstrating proper usage.Type of change
Checklist
/docs/directory for Fiber's documentation.-raceflag.Original prompt
This section details on the original issue you should resolve
<issue_title>🐛 [Bug]: nil pointer dereference for (fiber.Ctx).Value</issue_title>
<issue_description>### Bug Description
When sending streaming response with object that was created from minio.GetObject with fiber.Ctx context it tries to access ctx.Value but is hit with nil pointer dereference.
How to Reproduce
Steps to reproduce the behavior:
Expected Behavior
Body can be streamed in response like it is in fiber v2
Fiber Version
v3.0.0
Code Snippet (optional)
Checklist:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.