fix: reject record batches with a negative record count - #1445
Open
ankit-songara wants to merge 1 commit into
Open
fix: reject record batches with a negative record count#1445ankit-songara wants to merge 1 commit into
ankit-songara wants to merge 1 commit into
Conversation
readFromVersion2 read numRecords as a signed int32 and used it directly as the length for make([]optimizedRecord, numRecords) without checking its sign. A Fetch response with a crafted v2 record batch reporting numRecords=-1 passes the batch length and CRC checks but then panics with "makeslice: len out of range", crashing any client that doesn't recover around the fetch path. Reject the batch with an error as soon as a negative count is seen, before the allocation. Fixes segmentio#1438
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
2 participants
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.
Summary
readFromVersion2readsnumRecordsas a signedint32after the batch length and CRC checks, then uses it directly as the length formake([]optimizedRecord, numRecords). A Fetch response containing a structurally valid v2 record batch withnumRecords=-1passes those checks and then panics withmakeslice: len out of range, which crashes any consumer that doesn't wrap the fetch path in arecover.This adds a sign check right before the allocation so the batch is rejected with a normal error instead of panicking.
Fixes #1438
Test plan
TestRecordSetNegativeRecordCountinprotocol/record_batch_test.go: builds a real v2 record batch viaRecordSet.WriteTo, patches the record count field to-1, recomputes the CRC over the crc-protected region, then feeds it throughRecordSet.ReadFromand asserts an error is returned (and that decoding does not panic).mainwithout the fix and passes with it.go build ./protocol/...,go vet ./protocol/...,go test ./protocol/...all pass.gofmt -lclean on both touched files (ignoring the pre-existing CRLF-vs-LF noise fromgofmt -lon this Windows checkout, which flags these files identically before and after my change).