🐛 fix: Respect Immutable config for Body() #3246
Conversation
nickajacks1
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
nickajacks1
force-pushed
the
issue-3236
branch
from
9d0ddae to
6e40bd8
Compare
WalkthroughThe changes primarily involve the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gaby
changed the title
|
@nickajacks1 This happens in v3 too right? If so can you send a PR for that target branch |
|
@gaby no, v3 properly checks for immutability in the getBody() function. I can update to add getBody. Actually, we should probably just backport that function. Let me make that update. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ctx.go (2)
Line range hint
348-365: Consider respecting Immutable config in tryDecodeBodyInOrder.The method modifies the request body during decoding attempts using
SetBodyRaw. When Immutable is true, consider creating copies of the body for each decoding attempt to prevent potential data races.func (c *Ctx) tryDecodeBodyInOrder( originalBody *[]byte, encodings []string, ) ([]byte, uint8, error) { var ( err error body []byte decodesRealized uint8 ) for index, encoding := range encodings { decodesRealized++ switch encoding { case StrGzip: body, err = c.fasthttp.Request.BodyGunzip() case StrBr, StrBrotli: body, err = c.fasthttp.Request.BodyUnbrotli() case StrDeflate: body, err = c.fasthttp.Request.BodyInflate() default: decodesRealized-- if len(encodings) == 1 { body = c.fasthttp.Request.Body() + if c.app.config.Immutable { + body = utils.CopyBytes(body) + } } return body, decodesRealized, nil } if err != nil { return nil, decodesRealized, err } // Only execute body raw update if it has a next iteration to try to decode if index < len(encodings)-1 && decodesRealized > 0 { if index == 0 { tempBody := c.fasthttp.Request.Body() + if c.app.config.Immutable { + tempBody = utils.CopyBytes(tempBody) + } *originalBody = make([]byte, len(tempBody)) copy(*originalBody, tempBody) } c.fasthttp.Request.SetBodyRaw(body) } } return body, decodesRealized, nil }
Potential data leak in context pool reuse
The verification confirms that several fields are not properly reset between reuses:
pathBufferanddetectionPathBufferbyte slices are not resetvaluesarray is not clearedpath,detectionPath, andtreePathstrings are not resetWhile some fields are initialized in
AcquireCtx, these specific fields could retain data from previous requests, potentially leading to data leaks or incorrect behavior.Locations requiring attention:
ctx.go:190:ReleaseCtxshould be updated to reset:
pathBuffer = nildetectionPathBuffer = nilvalues = [maxParams]string{}path = ""detectionPath = ""treePath = ""🔗 Analysis chain
Line range hint
89-91: Verify context cleanup in the pool reuse.The context is reused from a pool, but not all fields are explicitly reset in
ReleaseCtx. Consider resetting additional fields likepathBuffer,detectionPathBuffer, andvaluesto prevent potential data leaks or race conditions.Also applies to: 149-163
The initial results confirm that not all fields are being reset in
ReleaseCtx. Let's verify if there's any initialization of these fields elsewhere, particularly in the context acquisition method.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for field declarations in Ctx struct and compare with resets in ReleaseCtx echo "Fields in Ctx struct:" ast-grep --pattern 'type Ctx struct { $$$ }' echo -e "\nField resets in ReleaseCtx:" ast-grep --pattern 'func (app *App) ReleaseCtx(c *Ctx) { $$$ }'Length of output: 1967
Script:
#!/bin/bash # Search for context acquisition and initialization echo "Context acquisition and initialization:" ast-grep --pattern 'func (app *App) AcquireCtx(fctx *fasthttp.RequestCtx) *Ctx { $$$ }' # Also check if there's a New or init method echo -e "\nNew/Init methods:" ast-grep --pattern 'func New$_($$$) *Ctx { $$$ }'Length of output: 1201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ctx.go(1 hunks)
🔇 Additional comments (1)
ctx.go (1)
343-347: LGTM! The changes properly respect the Immutable configuration.
The implementation correctly handles the Immutable configuration by creating a copy of the request body when needed, which prevents data races when processing the body in separate goroutines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ctx.go (1)
Line range hint
338-341: Consider enhancing the method documentation.While the implementation is solid, the method documentation could be more explicit about the behavior when Immutable is enabled, particularly regarding memory implications of body copying.
- // Body contains the raw body submitted in a POST request. - // This method will decompress the body if the 'Content-Encoding' header is provided. - // It returns the original (or decompressed) body data which is valid only within the handler. - // Don't store direct references to the returned data. + // Body contains the raw body submitted in a POST request. + // This method will decompress the body if the 'Content-Encoding' header is provided. + // When config.Immutable is true, it returns a copy of the body, which is safe to use + // across goroutines. Otherwise, it returns the original body which is only valid + // within the handler and should not be stored or used in other goroutines. + // + // Note: When Immutable is enabled, each call creates a new copy of the body, + // which has memory implications for large payloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ctx.go(1 hunks)
🔇 Additional comments (1)
ctx.go (1)
343-347: LGTM! The fix correctly implements body immutability.
The implementation now properly respects the Immutable configuration by returning a copy of the body when enabled. This prevents data races when accessing the request body across multiple goroutines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ctx.go (1)
343-347: Consider optimizing the copy operation.While the current implementation is correct, we could optimize it by only copying the body when it's actually going to be stored/used beyond the handler's lifecycle. This could be done by exposing a new method like
ImmutableBody()for cases requiring immutability, while keepingBody()fast for read-only operations.Example implementation:
func (c *Ctx) Body() []byte { - body = c.fasthttp.Request.Body() - if c.app.config.Immutable { - return utils.CopyBytes(body) - } - return body + return c.fasthttp.Request.Body() +} + +// ImmutableBody returns a copy of the body that is safe to use beyond the handler's lifecycle +func (c *Ctx) ImmutableBody() []byte { + return utils.CopyBytes(c.fasthttp.Request.Body()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ctx.go(1 hunks)
🔇 Additional comments (2)
ctx.go (2)
343-347: LGTM! The change correctly handles immutable body data.
The implementation properly respects the Immutable configuration by returning a copy of the body data when enabled, which prevents potential data races when accessing the request body across different goroutines.
343-347: Verify thread safety of the implementation.
Let's confirm that the implementation is truly thread-safe by checking the utils.CopyBytes() implementation and testing concurrent access patterns.
✅ Verification successful
Let me gather more information about the decompression methods and their thread safety.
Based on the gathered information, I can now provide the final response:
Implementation is thread-safe when Immutable is enabled
The implementation is verified to be thread-safe when Immutable config is enabled because:
utils.CopyBytes()creates a new byte slice and performs a deep copy of the data- The implementation properly handles concurrent access by:
- Creating a new slice with
make([]byte, len(b)) - Using
copy()to duplicate the data - Returning the new independent slice
- Creating a new slice with
However, when Immutable is false, direct access to the request body is returned which could lead to data races in concurrent scenarios. Consider making immutable behavior the default to prevent potential race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of utils.CopyBytes and its usage
# Check utils.CopyBytes implementation
echo "Checking utils.CopyBytes implementation..."
ast-grep --pattern 'func CopyBytes(b []byte) []byte {
$$$
}'
# Look for other direct body access patterns that might bypass the immutable check
echo "Checking for direct body access patterns..."
rg "Request.Body()" --type go
Length of output: 820
Script:
#!/bin/bash
# Check implementation of body decompression methods
echo "Checking body decompression implementations..."
rg -A 10 "func \(c \*Ctx\) Body\(\)" --type go
# Check if there are any other places where the body is stored or cached
echo "Checking body storage patterns..."
rg "var body" --type go -A 2
Length of output: 633
|
@nickajacks1 While you are at it, add go1.22 and 1.23 to the tests workflow |
|
Sure. This pr or separate? |
Same PR |
nickajacks1
force-pushed
the
issue-3236
branch
from
6e40bd8 to
06ad889
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
ctx.go (3)
271-271: Update documentation forBodyRaw()to reflectImmutableconfiguration handlingThe
BodyRaw()method now callsgetBody(), which respects theImmutablesetting by returning a copy of the body data when enabled. Consider updating the method's documentation to reflect this behavior change.
343-343: Update documentation forBody()to reflectImmutableconfiguration handlingSince
Body()now usesgetBody()when no content encoding is present, it's essential to update the documentation to indicate that the method respects theImmutablesetting by returning a copy of the body data when enabled.
2011-2017: Add documentation for the newgetBody()methodConsider adding a comment to the
getBody()method to explain its purpose and how it centralizes body data retrieval while respecting theImmutableconfiguration. This will enhance code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/test.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
ctx.go(4 hunks)
🔇 Additional comments (1)
ctx.go (1)
357-359: Properly handling Immutable configuration after body decoding
Good job ensuring that the Immutable setting is respected even after decoding the body by returning a copy of the body when Immutable is enabled. This prevents potential data races when the body is accessed across different goroutines.
Description
For request bodies without encoding, the Immutable configuration was not being respected, leading to potential data races.
go run -raceno longer produces warnings for the example in the original issue.Fixes #3236
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md