Fix avifParseItemLocationBox() itemReferenceIndex #2080
Conversation
Revert bug introduced with StreamReadBits(). Read item_reference_index even if construction_method!=2 to be accurate with the ISOBMFF specification.
|
There is the same issue with v1.0.x: Lines 1661 to 1665 in 8892ff4 I do not think it is necessary to cherry pick the fix. It should just wrongly reject/accept some rare files. |
Verbose description: * Fix 'iloc' box parsing when offset_size, length_size or base_offset_size was equal to 1 or 2. Such rare files were wrongly accepted. * Fix 'iloc'v1/2 box parsing when base_offset_size!=0. Such rare files were wrongly rejected. * Fix 'iloc'v1/2 box parsing when index_size!=0. Such rare files were parsed with corrupted extent offsets and sizes instead of ignoring index_size.
| // :: unsigned int(index_size*8) extent_index; | ||
| // :: } | ||
| uint64_t itemReferenceIndex; // unsigned int(index_size*8) item_reference_index; (ignored unless construction_method=2) | ||
| AVIF_CHECKERR(avifROStreamReadUX8(&s, &itemReferenceIndex, indexSize), AVIF_RESULT_BMFF_PARSE_FAILED); |
There was a problem hiding this comment.
Nit/optional: It would be clearer to put this call inside if (indexSize > 0). The corresponding spec is:
if (((version == 1) || (version == 2)) && (index_size > 0)) {
unsigned int(index_size*8) item_reference_index;
}
But perhaps it is that spec that should omit the && (index_size > 0) check.
There was a problem hiding this comment.
offset_size can be 0 so it makes as much sense as unsigned int(0) extent_offset; to me.
There was a problem hiding this comment.
Thanks. While reviewing #2091, I discovered a subtle point about item_reference_index -- its value is 1 rather than 0 when item_size is 0. Here is the excerpt from ISO/IEC 14496-12:2022 Section 8.11.3.1, page 82:
The
item_reference_indexis only used for the methoditem_offset; it indicates the 1-based index of the item reference withreferenceType 'iloc'linked from this item. Ifindex_sizeis 0, then the value 1 is implied; the value 0 is reserved.
Calling avifROStreamReadUX8(&s, &itemReferenceIndex, indexSize) with indexSize=0 will set itemReferenceIndex to 0. So item_reference_index needs different handling anyway.
Revert bug introduced with StreamReadBits(): https://github.com/AOMediaCodec/libavif/pull/1506/files#diff-d4b71621e3fb7422ad72c18339089f9626a0c48a95d4b71483bb18351bcd58c4
Read item_reference_index even if construction_method!=2 to be accurate with the ISOBMFF specification.