Repository navigation
fix(render): don't modify caller's headers map in Reader.Render - #4900
dhruv-techdev wants to merge 2 commits into
Conversation
Reader.Render wrote Content-Length into the caller-supplied Headers map, so DataFromReader leaked a stale Content-Length into later responses that reused the map and raced when the map was shared across requests. Set Content-Length on the response headers instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4900 +/- ##
==========================================
- Coverage 98.65% 98.65% -0.01%
==========================================
Files 48 48
Lines 3188 3186 -2
==========================================
- Hits 3145 3143 -2
Misses 43 43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @appleboy, could you please take a look at #4900 when you get a chance? This PR fixes an unintended mutation of the caller-provided headers map in I'd appreciate your feedback. Thanks for maintaining Gin! |
Tales-of-zuna
left a comment
There was a problem hiding this comment.
Tested locally against upstream master (Go 1.27.3, -race):
- Reproduced #4899 on master: the caller's map gained
Content-Length, a stale length leaked into a later-1response, and concurrent use raced. - With this PR, all three are fixed, and
go test -race ./...passes. - The new tests fail with the
reader.gochange reverted, so they guard the regression. - Precedence is unchanged: a
Content-Lengthalready set on the writer (e.g. by middleware) still wins, matching howwriteHeadersbehaves on master.
It also avoids the per-response map copy used in #4905. The failing Trivy check fails on master too, so it's unrelated.
LGTM 👍 (first review here, happy to be corrected)
|
Thank you, @Tales-of-zuna, for testing this against upstream master and for the detailed review! I really appreciate you taking the time to verify the fix and regression tests. |
|
Just a quick note for the maintainers: The CI failure here is due to the global repository build issue and is unrelated to my changes. Ready for core review once the pipeline is fixed upstream! 👍 |
Fixes #4899
Reader.RenderwroteContent-Lengthinto the caller-suppliedHeadersmap, soDataFromReaderleaked a staleContent-Lengthinto later responses that reused the map and raced when the map was shared across requests. SetContent-Lengthon the response headers instead.Problem
Context.DataFromReaderpassesextraHeadersstraight torender.Reader, andReader.Renderchanged it:contentLength = -1still sends the oldContent-Length. Go's server then rejects the body because it is longer than the declared length, and the client gets an empty 200 response.go test -racereports a race atrender/reader.go:28, and the runtime can abort withfatal error: concurrent map writes.Fix
Content-Lengthis now set directly onw.Header(), so the caller's map is only read. Which value wins is unchanged:Content-Lengthalready set on the response still takes priority;contentLengthargument still overrides aContent-Lengthpassed inextraHeaders.Tests
Added
TestReaderRenderDoesNotModifyHeadersinrender/reader_test.go. It checks that the caller's map is unchanged and that reusing the map for an unknown-length response does not carry overContent-Length. It fails on the old code and passes with the fix.This replaces #3568, which was closed without being merged.
Pull Request Checklist
masterbranch.docs/doc.md.Thank you for contributing!