Conversation
- Validate that the EditListBox ('elst') version is 0 or 1 regardless
of whether flags & 1 is set.
- Do not skip entry parsing when flags & 1 is 0; ISO/IEC 14496-12 defines
the entry structure unconditionally.
- Fully read media_time, media_rate_integer, and media_rate_fraction
fields according to the box version.
- Enforce that no unexpected trailing bytes remain in the box.
- Add regression tests covering valid files, size delta errors, invalid
version, zero segment duration, and invalid entry count in
avifchildboxsizetest.cc.
| if (version != 0 && version != 1) { | ||
| // Unsupported version | ||
| avifDiagnosticsPrintf(diag, "Box[elst] has an unsupported version [%u]", version); | ||
| return AVIF_FALSE; |
There was a problem hiding this comment.
Not such a big deal, but AVIF_FALSE leads to AVIF_RESULT_BMFF_PARSE_FAILED in avifParseTrackBox(). Maybe avifParseEditListBox() should return an avifResult to distinguish with NOT_IMPLEMENTED. Could be another PR to split the refactoring out of this change.
There was a problem hiding this comment.
Also I would guess it is inconsistent across libavif but spec says:
FullBoxeswith an unrecognized version shall be ignored and skipped.
Here it returns an error instead.
|
|
||
| if ((flags & 1) == 0) { | ||
| track->isRepeating = AVIF_FALSE; | ||
| return AVIF_TRUE; |
There was a problem hiding this comment.
I am wondering why there was an early return here. Spec says:
When an
EditListBoxindicates the playback of zero samples or one sample,RepeatEditsshall be equal to 0.
And below we return false if entryCount != 1.
| uint64_t ignored64; | ||
| uint32_t ignored32; | ||
| if (version == 1) { | ||
| AVIF_CHECK(avifROStreamReadU64(&s, &track->segmentDuration)); // unsigned int(64) segment_duration; |
There was a problem hiding this comment.
9th ed seems to call that field "edit_duration", maybe it changed recently
| AVIF_CHECK(avifROStreamReadU16(&s, &ignored16)); // int(16) media_rate_integer; | ||
| AVIF_CHECK(avifROStreamReadU16(&s, &ignored16)); // int(16) media_rate_fraction = 0; |
There was a problem hiding this comment.
Format check will likely complain here
| if (avifROStreamRemainingBytes(&s) != 0) { | ||
| avifDiagnosticsPrintf(diag, "Box[elst] has %zu unexpected trailing bytes", avifROStreamRemainingBytes(&s)); | ||
| return AVIF_FALSE; | ||
| } |
There was a problem hiding this comment.
This is invalid. Spec says:
When a parser has not reached the end of a content box as defined by the values of the size or largesize field (as appropriate) but does not recognize the remaining syntax elements, it shall ignore and skip the remaining of the content box.
2 participants
Summary
This PR hardens EditListBox (elst) parsing in src/read.c to fully conform to ISO/IEC 14496-12 and ISO/IEC 23008-12 (MIAF/HEIF):
ead.c terminated parsing early after reading only the 4-byte FullBox header, leaving all entry bytes unparsed and unverified.