fix(delta): compute the distance in int, not byte - #488
Open
youdie006 wants to merge 1 commit into
Open
Conversation
The property byte holds distance-1, so 0xff means a distance of 256, but int(p[0] + 1) wraps in byte arithmetic and yields 0. Read then never advances its index and spins forever on an archive that parses cleanly.
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.
internal/delta/reader.go:87computes the delta distance in byte arithmetic:7-Zip stores the distance as
distance - 1, so a property byte of0xffmeans a distance of 256.p[0] + 1is evaluated asbyte, so0xff + 1wraps to0, anddeltabecomes 0. The decode loopin
Read(internal/delta/reader.go:57) then never advances:An archive using a Delta filter with distance 256 parses cleanly and then hangs the extractor:
I built an 85-byte archive by hand to check this - a
Copy -> Deltafolder with the delta propertyset to
0xff- rather than mutating an existing one, so it is a well-formed archive rather than acorrupt blob. Happy to attach it if that is useful for the repo's testdata, though the unit test
below covers the same ground without shipping a file.
The fix
256 is exactly
stateSize, so the widened value still fits the[256]bytestate and buffer - whichis what the surrounding code was already written for.
Test
internal/delta/reader_test.go, table-driven over both ends of the property byte:0x00-> distance1 and
0xff-> distance 256, asserting the distance and that aReadreturns.I mutation-checked it in both directions:
int(p[0] + 1)->panic: test timed out after 15s(the hang)int(p[0]) + 2-> fails on theminimumcase,expected: 1so the test pins the distance rather than just the absence of a hang. With
+2on the maximum casethe state slice also goes out of range (
[:257] with length 256), which is the other side of why 256is the ceiling.
go test ./...is green:ok github.com/bodgit/sevenzip 81.711splus all subpackages, including theexisting
delta.7zround trip.gofmt -landgo vet ./...clean.Observable change: only for delta distance 256, which previously never produced output. Nothing
that decoded before decodes differently.
One observation, not part of this change
(*readCloser).Readswallowsio.EOF- it returnsn, nilon every path - so callingio.Copydirectly on a delta reader does not terminate even with this fix. It does not matter inside
sevenzip, which bounds the read by the known uncompressed size, and it is a separate root cause, soI left it alone. Mentioning it because it is why the test above calls
Readdirectly rather thanusing
io.Copy.Disclosure: found and prepared with AI assistance (Claude). The root cause, the archive construction
and every figure above are mine and verified by hand on this branch.