Uh oh!
There was an error while loading. Please reload this page.
fix: bound the envelope item payload length before allocating a read buffer - #5541
Open
thaildhe172591 wants to merge 1 commit into
Open
fix: bound the envelope item payload length before allocating a read buffer#5541thaildhe172591 wants to merge 1 commit into
thaildhe172591 wants to merge 1 commit into
Conversation
…buffer EnvelopeItem.DeserializePayloadAsync took the payload length straight from the item header and rented a buffer of exactly that size, so a corrupt but parseable header could make a small cache file allocate an arbitrary amount. "length": 2000000000 rents 2 GB; 3000000000 overflows the unchecked (int) cast to -1294967296 and throws out of ArrayPool.Rent; and with no length key at all, (int)stream.Length overflows the same way on a file larger than 2 GB. The four buffering branches now take their length from GetPayloadBufferLength, which rejects a declared length that is negative or longer than what remains in the stream, and a length that cannot fit in an int. Both throw InvalidDataException, which the caching transport already discards on. The attachment branch is left alone, so a file truncated mid-write still deserializes into a short PartialStream as it does today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 freeto 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.
Addresses #5536, following on from #5507.
EnvelopeItem.DeserializePayloadAsynctakes the payload length from the item header and rents a buffer of exactly that size, so a corrupt but parseable header can make a few-KB cache file allocate an arbitrary amount. The three shapes from the issue:"length": 2000000000rents 2 GB,"length": 3000000000overflows the unchecked(int)cast to-1294967296and throwsArgumentOutOfRangeExceptionout ofArrayPool.Rent, and an item with nolengthkey at all takes(int)stream.Length, which overflows the same way once the file is larger than 2 GB.The four buffering branches now derive their length from
GetPayloadBufferLength, which rejects a declared length that is negative or longer than what remains in the stream, and a length that cannot fit in anint. Both throwInvalidDataException, which #5507 already routes into the caching transport's discard, so the user-visible outcome for a corrupt file is unchanged: it is discarded rather than retried on every launch. What changes is that the oversized allocation never happens. Neither bound encodes any Relay configuration, as you noted on the issue:remainingis measured from the file in front of us andint.MaxValueis the CLR array limit.I kept this to the allocation bound. The attachment branch is untouched, so a file truncated mid-write still builds a short
PartialStreamand is sent as it is today. Discarding those is the behaviour change you flagged, and since it wants its own changelog line I left it out — happy to fold it in here instead if you would rather have both at once. I have left the issue open rather than closing it from this PR for the same reason.Five tests in
EnvelopeTests, covering each of the three failure modes above, the no-lengthpath through a buffering branch, and the over-2 GB case. That last one uses a smallOversizedStream : MemoryStreamthat reports aLengthof 3 GB over a real buffer, since theint.MaxValuebound is otherwise only reachable with a 2 GB fixture.Verified locally, as CI will not run on a first-time fork PR until it is approved. Ubuntu under WSL2, SDK 10.0.400 as pinned in
global.json, with the .NET 8, 9 and 10 runtimes installed.dotnet build Sentry-CI-Build-Linux-NoMobile.slnf -c Releasesucceeds with no errors, anddotnet teston the same filter gives 10339 passed, 0 failed, 94 skipped across 53 project and framework combinations.Sentry.Testson its own is 2548 passed and 0 failed on each of net8.0, net9.0 and net10.0, which includesApiApprovalTests, so the public API snapshots are unchanged.dotnet format --verify-no-changesis clean on both files.For the before and after: reverting only
EnvelopeItem.csand rerunning the same tests unchanged fails four of the five, each with the symptom the issue predicts —Rent(-1)for the negative length,Rent(-1294967296)for both overflow cases, and no exception at all for the length that simply exceeds the stream. The fifth passes in both states; it is there to catch a regression in the switch fromstream.Lengthto the remaining bytes.I could not run net48 or the macOS, iOS and Android targets here.
🤖 Generated with Claude Code