Fix max_body_size for Content-Encoding: br - #230
Merged
Conversation
decompress() is a one-argument method, so passing it the limit as a second argument was an XS usage error rather than a size cap. Any brotli response therefore failed to decode whenever a limit was set, no matter how small the body was -- an 11-byte body died under a 1GB limit. unbro() is the only brotli interface that accepts a size, so use it when a limit is in effect. It decodes into a buffer of that many octets, so the limit bounds the allocation directly. It does not commit the buffer up front: RSS stays flat decoding a small payload under a large limit. unbro() reports "output buffer too small" and "this is not valid brotli" identically, so the error cannot say which occurred and has to name both possibilities. t/message-decode-brotlibomb.t asserted only that decoding died when the body exceeded the limit. It did die -- on the arity error -- so the test passed for the wrong reason. It now asserts on the error message, and checks that a body under the limit still decodes. Broken since 7fc855e, the commit that added max_body_size; released in 6.42 through 7.02. GH#229 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #230 +/- ##
==========================================
- Coverage 94.57% 94.50% -0.08%
==========================================
Files 10 10
Lines 1254 1256 +2
Branches 350 351 +1
==========================================
+ Hits 1186 1187 +1
Misses 14 14
- Partials 54 55 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Fixes #229.
IO::Uncompress::Brotli::decompressis a one-argument method, so passing the limit as a second argument was an XS usage error rather than a size cap. Every brotli response therefore failed to decode whenever a limit was set, regardless of size -- an 11-byte body died under a 1 GB limit.unbro($in, $max)is the only brotli interface that accepts a size. It decodes into a buffer of that many octets, so the limit bounds the allocation directly rather than being checked after the fact. It does not commit the buffer up front: RSS stays flat decoding a small payload under a large limit.One consequence worth knowing:
unbroreports "output buffer too small" and "this is not valid brotli" identically, so the error cannot say which occurred and names both possibilities. That is unlike the gzip and bzip2 branches, which can tell the two apart.Test
t/message-decode-brotlibomb.tasserted only that decoding died when the body exceeded the limit. It did die -- on the arity error -- so it passed for the wrong reason since it was written. It now also asserts on the error message, and checks that a body under the limit still decodes, which is the case that was actually broken.Verified the new assertions fail against the current
masterand pass here:Note that
prove -Iis not enough to check this if you have--libin a.proverc, since./libthen wins over-I. Run the file withperl -I<path>directly.The plan is now
done_testing()rather than a hardcoded count.Scope
Code, test and a
Changesline only.max_body_sizehas no Pod onmaster, so nothing to update here.