Repository navigation
Fix discarded errors in marshalSlice and marshalUint - #187
Merged
Kubo, Ryosuke (sunfish-shogi) merged 1 commit intoSep 16, 2026
Merged
Kubo, Ryosuke (sunfish-shogi) merged 1 commit into
Kubo, Ryosuke (sunfish-shogi) merged 1 commit into
Conversation
marshalSlice ignored the error from the recursive marshal call for each element, and marshalUint ignored the error from writeUvarint. Both then returned nil, so a failing io.Writer or an inconsistent nested slice produced a truncated box, a byte count larger than the writer accepted and no error at all. Every other recursion site in the file already checks the returned error.
Kubo, Ryosuke (sunfish-shogi)
approved these changes
Sep 16, 2026
Contributor
|
Thanks! |
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.
Kubo, Ryosuke (@sunfish-shogi)
mp4.Marshalcan report success after writing a truncated box.marshalSlicecallsm.marshal(v.Index(i), fi)for each element atmarshaller.go:154and throws the returned error away, then returnsnil.marshalUintdoes the same withm.writeUvarint(val)atmarshaller.go:199. Every other recursion site in the file propagates the error:marshalStructatmarshaller.go:115andmarshalArrayatmarshaller.go:127both assign it and return early, so the identical failure one level higher is reported correctly.The README's "Writing" example calls
mp4.Marshaland returns its error atREADME.md:101, the caller pattern this defeats. On masterd3b5338, marshalling a four-entrySttsinto anio.Writerthat accepts 10 bytes and then fails returnsn=34, err=<nil>: the writer took 10 bytes,Marshalclaims 34 and reports nothing wrong. AnEsdsholding one descriptor, written to a writer that accepts 5 bytes, returnsn=8, err=<nil>, because the write fails inside the descriptor's varintSizefield and the error is dropped twice, once inmarshalUintand again inmarshalSlice. The same silence hides an in-memory inconsistency: aKeysentry declaringKeySize: 12with an emptyKeyValueneedsKeySize-8 = 4bytes there and has 0, soKey.KeyValueproducesthe slice has too few elements: required=4 actual=0andMarshalstill returnsn=16, err=<nil>. Then=34on the first case is 10 real bytes plus 24 counted after the failed write, becausebitio.writer.WriteBitkeeps counting after a failed flush; that counter is a separate issue I have not touched here.The change is four added and three removed lines in
marshaller.go:marshalSlicechecks the error from each element, andmarshalUintreturnsm.writeUvarint(val)directly. With it applied the three cases returnn=0withno space left,no space leftandthe slice has too few elements: required=4 actual=0. Any marshal that finishes without an internal error writes the same bytes and returns the same count as before. The only change is that an error that was computed and dropped is now returned. These two are the only discardedm.marshal*andm.write*calls inmarshaller.go, so this covers all of them.marshaller_test.gogainsTestMarshalErrorand a smallfailAfterWriterthat accepts a fixed number of bytes and then fails. Its three cases are theSttsslice failure, theEsdsvarint failure and the nestedKeyslength mismatch, each asserting its exact message, so both edits stay pinned. All three fail on master withAn error is expected but got nilandnof0x22,0x8and0x10.Verified on
go1.26.4 darwin/arm64:go test -run TestMarshalError -v .fails with the fix reverted and passes with it,go test ./...is ok for every package,gofmt -l .is empty,go vet ./...is clean andgo build ./...succeeds.