Repository navigation
fix(middleware): restore previous response writer in BodyDump - #3174
Open
rajpreetcodes wants to merge 1 commit into
Open
rajpreetcodes wants to merge 1 commit into
rajpreetcodes wants to merge 1 commit into
Conversation
BodyDump installs a wrapped bodyDumpResponseWriter on the Echo Context via c.SetResponse, but previously never restored the original writer when the middleware returned. Because Echo invokes HTTPErrorHandler after the middleware chain has unwound, subsequent error handler writes went through the wrapped writer into a buffer that had already been returned to bodyDumpBufferPool, creating a data race and potential body corruption under concurrent load. Defer restoring the original response writer so that c.Response is reset before the pooled buffer is returned and before downstream error handling runs. Add regression tests verifying response writer restoration and concurrent error handling. Fixes labstack#3173
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Fixes #3173:
middleware.BodyDumpinstalls a wrapperbodyDumpResponseWriteronContextviac.SetResponse, but never restores the previous response writer before returning.Because
Echo.serveHTTPexecutesHTTPErrorHandlerafter the middleware chain unwinds, any error formatted by the error handler writes into the still-installed wrapper response writer. At that point, the underlying dump buffer (resBuf) has already been handed back tobodyDumpBufferPoolviadefer bodyDumpBufferPool.Put(resBuf). This creates a data race onbytes.Bufferunder concurrent traffic and risks corrupting pooled request/response buffers.Solution
origResponse := c.Response()before setting the wrapper.c.SetResponse(origResponse)so the previous response writer is restored whenever the middleware returns (normally, on error, or on panic).c.SetResponse(origResponse)runs beforebodyDumpBufferPool.Put(resBuf), guaranteeing thatc.Response()is restored before the buffer is returned to the pool and beforeHTTPErrorHandlerwrites.Changes
middleware/body_dump.go: save and defer restoring previous response writer.middleware/body_dump_test.go:TestBodyDump_RestoreResponse: asserts thatc.Response()is restored to its original value afterBodyDumpcompletes.TestBodyDump_ErrorHandlerRace: exercises concurrent error handling underBodyDumpwith multiple goroutines.Verification
go test ./...: all packages pass (github.com/labstack/echo/v5,echotest,middleware).go test -v -run TestBodyDump_RestoreResponse ./middleware: confirmed failure on pre-fix code (expected: *echo.Response, actual: *middleware.bodyDumpResponseWriter), passes cleanly with fix.gofmt -s.