Fix SeekEnd direction and ReadInt24 sign extension - #131
Open
youdie006 wants to merge 1 commit into
Open
Conversation
Seek with io.SeekEnd computed Len()-off, so a negative offset moved past the end instead of before it and later reads returned zeros. ReadInt24 converted a 24-bit value straight to int32, which cannot carry the sign the way the int8/16/32/64 siblings do.
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.
Two bugs in
BinaryReader, both in the part ofbinary.gothat no test or in-tree caller reaches.1.
Seekwithio.SeekEndmoves the wrong waybinary.go:316says// Seek complies with io.Seeker.Forwhence == 2the guard is alreadyright —
offmust be in[-Len(), 0]— but the arithmetic subtracts it (binary.go:332):With a negative offset that lands past the end. Against
bytes.Reader, the standard library'sio.Seeker, on an 8-byte buffer:and no error is returned, so reading the last four bytes silently yields zeros:
2.
ReadInt24does not sign-extendbinary.go:441:ReadInt8/16/32/64get the sign for free because the conversion keeps the width. There is noint24, so the 24-bit value is zero-extended into anint32and every negative number comes backpositive. The package's own writer is the oracle:
Change
binary.go:332:-becomes+.binary.go:441: sign-extend from bit 23,int32(r.ReadUint24()<<8) >> 8.Why these survived
grep -c "Seek\|ReadInt24" *_test.gois 0 in every test file, andbinary.gohas no in-treecaller outside its own definitions — so neither line has ever been executed by the suite.
Tests added to
binary_test.goin the existing style: the fourSeekEndoffsets checked againsttheir expected positions plus the read that follows, and a
WriteInt24/ReadInt24round trip over-1,-2,-8388608,0,1,8388607. Red with onlybinary.goreverted, green with thechange;
go test -race -count=1 ./...passes across all packages andgofmt -lis clean on bothfiles.
Happy to split these into two PRs if you would rather review them separately — they are independent
apart from living in the same untested file.
Disclosure: prepared with AI assistance; I verified both reproductions, the stdlib differential and
the red/green runs myself.