🐛 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 |
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. |
|
@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 |
|
@copilot You didnt update the docs for Request() and Response() for nil |
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Updated API documentation in
Both now clearly document the nil return behavior when context is released after handler completion. Markdown linting passes. Commit: b64b1c4 |
|
@copilot Document on the code... Look at this thread #4062 (review) the nil return has to be documented at the code and docs level |
Both levels are already documented: Code level (ctx.go godoc comments):
Docs level (docs/api/ctx.md):
These were added in commits 032385e (godoc) and b64b1c4 (docs). Both levels are complete. |
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 released/docs/api/ctx.mdfor:Request()- Added info box documenting nil return when context is releasedResponse()- Added info box documenting nil return when context is releasedSendStream()- Documented asynchronous behavior, automatic stream closing, context lifecycle warnings, and added practical file streaming exampleTest_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 documentation forRequest(),Response(), andSendStream()methods with nil behavior, 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. Enhanced API documentation in/docs/api/ctx.mdforRequest(),Response(), andSendStream()methods to document nil return behavior, async behavior, automatic stream closing, and proper context usage patterns with goroutines. 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 forRequest(),Response(), andSendStream()to clarify nil return behavior, async streaming 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 nil return behavior for Request/Response methods, 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, async operations, and nil return scenarios.
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.