Parse 'auxl' and 'prem' within their own box size

[wantehchang]

A follow-up to PR #3321.

Delete the comments that describe the bugs that were fixed.

Also move avifROStreamSkip(&s, irefHeader.size) and avifROStreamSkip(&s, groupHeader.size) calls to the end of the loops, which is the common convention in this file.

[wantehchang]

cc: @Nexory

I audited the entire src/read.c file and inspected all the avifROStreamReadBoxHeader() and avifROStreamReadBoxHeaderPartial() calls. Only avifTrackReferenceBox() needs work, see the changes below.

I deleted the comments that describe the bugs that were fixed. They can be confusing unless one looks at the old code.

[Nexory]

Thanks for the follow-up and for auditing the whole file, @wantehchang.

Both changes look correct to me. Moving the avifROStreamSkip(&s, ...size)
to the end of the loop body is behaviour-preserving: the field reads go
through the bounded sub-stream, so s is not advanced between BEGIN_STREAM
and the skip, and every error path returns via AVIF_CHECK/AVIF_CHECKERR
rather than continuing, so there is no path that skips the skip. Matching the
file convention is nicer.

The auxl/prem guards are the same class of fix: without
header.size >= sizeof(uint32_t), the header.size - sizeof(uint32_t) skip
would underflow. Good catch on the track reference box; I had only looked at
the item and groups boxes.

And no objection to dropping the comments now that the bug is gone.

[Nexory]

LGTM. I verified the skip move is behaviour-preserving and the auxl/prem size checks close the same underflow in the track reference box.

[wantehchang]

Nexory: Thank you for reviewing this PR. While auditing all the avifBoxHeader variables in src/read.c, I found the avifParse() function to be quite complicated. If you could analyze avifParse(), that would be much appreciated.

[Nexory]

@wantehchang Thanks for the pointer, and for extending the iref fix to auxl/prem in this PR. I have finished the pass over avifParse() you asked for. Summary up front: I found no memory safety issue in avifParse() or in the helpers it calls directly. Most of what looked suspicious on a first read turned out to be deliberate, and I say so explicitly below so the list stays useful.

Scope: read.c, stream.c and internal.h at 6868dd1. I checked the tmap items against a build; everything else below is from reading the code, and I flag which is which.

How the loop is structured, as I understand it

Three properties determine most of the behaviour, and three of my initial candidates died on them:

1. header.size from avifROStreamReadBoxHeaderPartial() is the content size (stream.c:293, size - bytesRead), so parseOffset is advanced twice per box: once by the header length at read.c:4855, once by the content length at read.c:4920. There is no double count, and the overflow guard at 4917 covers the second advance.
2. A size zero box has no declared length at all, so header.size = boxContents.size at read.c:4907 is the contract from internal.h:662-665, not a missing truncation check. The counterpart check at 4909 exists only because a sized box has a declared length to compare against.
3. The early out at read.c:4999-5003 returns as soon as the brands' requirements are satisfied, so boxes after that point are neither parsed nor validated. The comment at read.c:6358-6361 already states this trade-off for tmap, so I treated it as accepted design.

Observations I think are worth a follow up

1. largesize == 0 is treated as "payload extends to end of file". In avifROStreamReadBoxHeaderPartial(), size is overwritten by the 64 bit largesize at stream.c:258-260, and the size zero test at stream.c:269 then reads the overwritten value. So a box written as size=1, largesize=0 sets isSizeZeroBox and never reaches the size < bytesRead rejection at stream.c:288, even though the header alone is 16 bytes. Section 4.2.2 attaches the to EOF meaning to the 32 bit size element only, which is what the comment at stream.c:270-273 quotes. Practical effect in avifParse(): such a meta swallows the following top level boxes, the trailing mdat is skipped as an unrecognised child at read.c:3520, and the file parses to AVIF_RESULT_OK. No memory safety consequence, item extents are still bounded by sizeHint. The fix is one token, testing smallSize == 0 instead, so largesize == 0 falls into the existing rejection.

2. AUTO source selection can depend on top level box order. needsMeta and needsMoov at read.c:4934-4935 consider the major brand and the compatible brands, but the AUTO chain at read.c:6121-6131 looks only at data->majorBrand and then falls back to data->tracks.count > 0. For a file with major brand mif1 and avif among the compatible brands, needsMoov is false, so whether a top level moov was parsed at all depends on whether it precedes the meta box. Both orderings return AVIF_RESULT_OK, but one decodes as a still image and the other as a sequence, with different dimensions and imageCount, and no diagnostic on either branch. Since avifFileTypeIsCompatible() already requires avif or avis somewhere in the ftyp, the tracks.count fallback is only reachable for exactly these files today. Making the chain consult avifBrandArrayHasBrand(&data->compatibleBrands, ...) before the fallback would give the same treatment tmap already gets at read.c:6362. One behaviour change worth calling out: a file branded avif whose moov precedes a meta with no usable primary item would then return AVIF_RESULT_NO_CONTENT instead of silently playing an undeclared track.

3. decoder->imageSequenceTrackPresent is never reset. read.c:4984 is the only assignment in the decoder besides the memset in avifDecoderCreate(). Neither avifDecoderCleanup() nor avifDecoderReset() clears it, so on a reused decoder a still image is reported as having an image sequence track after a sequence was parsed earlier. decoder->repetitionCount has the same shape, assigned only at read.c:6297. Every other output field is set on both branches of avifDecoderReset(), including the explicit "safe but uninteresting values" block at read.c:6481, so these two look like gaps in an otherwise complete convention rather than intent. Clearing the flag at the top of avifParse() next to moovSeen avoids the case where a moov in a parse that later fails leaves it set. Note it should not go into avifDecoderReset(), because avifDecoderSetSource() calls that without re-parsing.

4. The tmap brand is evaluated by two different predicates, and the strict one rejects decodable files. avifParse() asks via avifFileTypeHasBrand() (read.c:4951), which matches the major brand first, while avifDecoderReset() asks via avifBrandArrayHasBrand(&data->compatibleBrands, ...) (read.c:6362), which has no major brand fallback, and read.c:4927-4933 stores the major brand outside that array. Independently of that split, a file that declares the tmap brand but carries no tmap item is rejected outright at read.c:5013 with AVIF_RESULT_BMFF_PARSE_FAILED and an empty diagnostic. I measured this one: starting from tests/data/paris_icc_exif_xmp.avif, changing 4 bytes so the major brand reads tmap makes avifDecoderParse() fail, and changing 4 bytes so a compatible brand reads tmap (the placement 10.2.6 mandates) fails identically. Both fail with AVIF_STRICT_DISABLED as well, since avifParse() does not consult strictFlags there. That is the opposite of how GainMapTest.DecodeInvalidFtyp handles the inverse mismatch, where the brand/content disagreement is tolerated and the gain map is ignored. If you agree the two directions should be symmetric, the smaller change is to keep only the metaIsSizeZero case fatal at read.c:5013 and emit a diagnostic otherwise.

5. Experimental MINI only: mif3 plus the tmap brand cannot parse. needsTmap forces needsMeta at read.c:4952-4954, which runs after the AVIF_CHECKERR(!needsMeta, ...) guard at read.c:4939, and tmapSeen is only ever set in the meta branch at read.c:4965 while meta and mini are mutually exclusive. The early out can then never fire and the file falls through to AVIF_RESULT_TRUNCATED_DATA at read.c:5011, with an empty diagnostic, for a complete file. This does not affect default builds, and libavif's own encoder writes empty compatible brands for mif3, so it only bites a third party writer following 10.2.6. Mentioning it because the MINI path is under active development. avifParseMinimizedImageBox() already creates the tmap item in the same data->meta at read.c:4452-4455, so hoisting the item scan out of the meta branch would cover it.

6. Minor, low priority: read.c:4805 is (int)compatibleBrandsBytes / 4, so the cast narrows the byte count before the divide. It only matters for an ftyp payload of 2 GiB or more, which no real file has, and I confirmed it cannot become an out of bounds access: the % 4 check at read.c:4799 plus the > 0 guard at read.c:4928 keep 4 * count within the box in every case. Dividing in size_t and range checking before the cast would match the style already used at read.c:4015 and read.c:6283. I would not open a PR for this on its own, and a regression test for it would need about 2 GiB, so it would have to be disabled by default.

Things I checked and consider intentional or benign, so they do not need action

• The AVIF_RESULT_BMFF_PARSE_FAILED versus AVIF_RESULT_TRUNCATED_DATA split. I initially read read.c:4838 and read.c:4854 as inconsistent with read.c:4911, but the codebase is consistent on a different axis: declared structure exceeding sizeHint gives BMFF_PARSE_FAILED (read.c:588, 620, 1517, 5266), a read that came back short gives TRUNCATED_DATA (read.c:1531, 4911, 5274). The retry signal for incremental callers is AVIF_RESULT_WAITING_ON_IO in any case, and nothing in read.c branches on TRUNCATED_DATA.
• Unknown top level boxes before ftyp being skipped. The enforced set at read.c:4883-4884 mirrors the example list in the clause quoted right above it, and read.c:4913 shows unknown top level boxes are an anticipated case. avifPeekCompatibleFileType() is a prefix sniffer that already disagrees with avifParse() in both directions on ordinary files, so I do not think the two are meant to agree.
• headerContents not being initialised at read.c:4837. It is only consumed after the read returns AVIF_RESULT_OK, both shipped readers assign *out on every OK path, and the same pattern is used at read.c:1499, 5265 and 5686. The initialised sibling at read.c:4860 is initialised because its read is conditional.
• The metaIsSizeZero conditional at read.c:5014. For a size zero box there is no declared length to cross check, so truncation genuinely cannot be excluded, and the softer code looks like the right call.
• AVIF_ASSERT_OR_RETURN at read.c:4856. It can only fire if an avifIO reports a sizeHint smaller than what its read delivers, which the documented contract forbids.
• The early out making duplicate meta/moov detection depend on how far the parser got. The comment at read.c:6358-6361 already records that trade-off. Item 2 above is only about an output decision hanging off it, not about the early out itself.

Happy to send a patch plus a regression test for any of the six items you think is worth fixing, one at a time and in whatever order suits you. Items 1 and 3 are the smallest and I would start there if you have no preference. If you would rather close any of them as working as intended, that is a fine answer too and I will not re-raise it.