Skip to content

Fix discarded errors in marshalSlice and marshalUint - #187

Merged
Kubo, Ryosuke (sunfish-shogi) merged 1 commit into
abema:masterfrom
MaxFreedomPollard:fix-discarded-marshal-errors
Sep 16, 2026
Merged

Kubo, Ryosuke (sunfish-shogi) merged 1 commit into
abema:masterfrom
MaxFreedomPollard:fix-discarded-marshal-errors

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown
Contributor

Kubo, Ryosuke (@sunfish-shogi)

mp4.Marshal can report success after writing a truncated box. marshalSlice calls m.marshal(v.Index(i), fi) for each element at marshaller.go:154 and throws the returned error away, then returns nil. marshalUint does the same with m.writeUvarint(val) at marshaller.go:199. Every other recursion site in the file propagates the error: marshalStruct at marshaller.go:115 and marshalArray at marshaller.go:127 both assign it and return early, so the identical failure one level higher is reported correctly.

The README's "Writing" example calls mp4.Marshal and returns its error at README.md:101, the caller pattern this defeats. On master d3b5338, marshalling a four-entry Stts into an io.Writer that accepts 10 bytes and then fails returns n=34, err=<nil>: the writer took 10 bytes, Marshal claims 34 and reports nothing wrong. An Esds holding one descriptor, written to a writer that accepts 5 bytes, returns n=8, err=<nil>, because the write fails inside the descriptor's varint Size field and the error is dropped twice, once in marshalUint and again in marshalSlice. The same silence hides an in-memory inconsistency: a Keys entry declaring KeySize: 12 with an empty KeyValue needs KeySize-8 = 4 bytes there and has 0, so Key.KeyValue produces the slice has too few elements: required=4 actual=0 and Marshal still returns n=16, err=<nil>. The n=34 on the first case is 10 real bytes plus 24 counted after the failed write, because bitio.writer.WriteBit keeps 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: marshalSlice checks the error from each element, and marshalUint returns m.writeUvarint(val) directly. With it applied the three cases return n=0 with no space left, no space left and the 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 discarded m.marshal* and m.write* calls in marshaller.go, so this covers all of them.

marshaller_test.go gains TestMarshalError and a small failAfterWriter that accepts a fixed number of bytes and then fails. Its three cases are the Stts slice failure, the Esds varint failure and the nested Keys length mismatch, each asserting its exact message, so both edits stay pinned. All three fail on master with An error is expected but got nil and n of 0x22, 0x8 and 0x10.

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 and go build ./... succeeds.

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.
@sunfish-shogi

Copy link
Copy Markdown
Contributor

Thanks!
LGTM.

@sunfish-shogi
Kubo, Ryosuke (sunfish-shogi) merged commit cf19f66 into abema:master Sep 16, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants