Bound iloc extent_count to prevent memory amplification - #3375
littlepig12345 wants to merge 4 commits into
Conversation
|
Thank you for your interest in libavif. Rather than adding arbitrary limits, what about checking extents? Could zero-length extents be dropped, avoiding the issue you described? Do you have a reproducer test that crashes because of memory allocation failures, for such a small input file? Is the |
|
Thank you for the suggestion. I agree that dropping zero-length extents is more precise than an arbitrary cap. I've updated the patch to skip extent allocation when all size fields are zero. This avoids the amplification entirely and does not affect legitimate files, since zero-size extents carry no data reference. Here is the updated check: |
|
The issue comes from empty extents, right? What is the link with index and offset being 0? Do you have a reproducer test please? To make sure a change is necessary. |
bbd9ba1 to
64bae73
Compare
|
The issue comes from empty extents. An extent is empty when it carries no offset, no length, and no index information. That happens when offset_size, length_size, and index_size are all zero (index_size is only present for version 1 and 2, and is treated as zero for version 0). In that case, avifROStreamReadUX8() consumes zero bytes per extent, but the loop still calls avifArrayPush(&item->extents) once per declared extent. A small input can therefore declare a very large number of extents, each triggering an allocation. This is a memory amplification issue. The input file itself is tiny, but the parser allocates one avifExtent per declared extent, so a crafted file can drive the process to OOM. I confirmed this with a local test. The configuration is: With this configuration the generated file is only ~59 KB, yet it declares 10000 items × 65535 extents = ~655 million extents. Each extent allocates an avifExtent without consuming any input bytes. Tested on Windows 11 natively (not WSL). Before the patch: After the patch: The fix is to skip extents that carry no information instead of pushing them into item->extents. This is safe because such extents are empty by definition: they have no offset, no length, and no index. They cannot contribute any usable data to the item, so skipping them does not change the parsing result for valid files. It only prevents the parser from allocating an avifExtent for every declared but information-free extent. For malformed or empty inputs, the decoder may now report "Missing or empty image item" instead of attempting to allocate extents. This is expected and preferable, because the input contains no usable extent data, and the previous behavior only led to unbounded allocation. This prevents the unbounded allocation in this path. |
|
Thank you for the explanation. I understand for The current patch also does not skip extents where This also makes me think about overlapping extents causing similar effects. If extents can be signaled as overlapping in |
|
Good point about the case where length_size != 0 but extent_length == 0. I can also skip extents with extent_length == 0, but I want to make sure that's safe: is a zero-length extent with a valid length field meaningful in any valid AVIF file, or can we skip it as well? To explain the current logic: the condition offset_size == 0 && length_size == 0 && index_size == 0 is the only case where the extent carries nothing at all. It has no offset field, no length field, and no index field. If offset_size or index_size is non-zero, the extent still carries a field, so skipping it could change the extents list for a valid file. That's why I kept the condition strict. I'd rather not drop extents that might carry semantic meaning in a valid file. |
|
Rereading ISOBMFF 14496-12 Section 8.11.3.2.4.3 "Extent length":
So extents with |
|
Thank you for the clarification. You're right that extent_length == 0 has a defined meaning in ISO BMFF, and libavif does not implement it. I've updated the patch to handle both cases:
I confirmed with a local test: a 59KB input causes ~9541.95 MB peak memory and OOM before the patch, and returns immediately after the patch. |
|
I've updated the patch (see PR changes). The check is now purely about stream bytes, no semantic assumptions: const uint32_t bytesPerExtent = indexSize + offsetSize + lengthSize;
if (bytesPerExtent == 0 && extentCount > 1) {
avifDiagnosticsPrintf(diag, "Item ID [%u] declares %u extents that consume no input bytes", itemID, extentCount);
return AVIF_RESULT_BMFF_PARSE_FAILED;
}
const size_t remainingBytes = avifROStreamRemainingBytes(&s);
if ((uint64_t)extentCount * bytesPerExtent > remainingBytes) {
avifDiagnosticsPrintf(diag,
"Item ID [%u] extent_count [%u] exceeds the remaining %zu bytes of the iloc box",
itemID,
extentCount,
remainingBytes);
return AVIF_RESULT_BMFF_PARSE_FAILED;
}Inside the loop, after reading // ISO/IEC 14496-12, Section 8.11.3.2.4.3: extent_length == 0 is not supported by libavif.
if (lengthSize > 0 && extentLength == 0) {
avifDiagnosticsPrintf(diag, "Item ID [%u] uses extent_length = 0, which is not supported", itemID);
return AVIF_RESULT_NOT_IMPLEMENTED;
}Differences from the earlier version:
Test results with
Diagnostics: Valid AVIF files with genuine multi-byte extents still parse unchanged. |
| extentCount, | ||
| remainingBytes); | ||
| return AVIF_RESULT_BMFF_PARSE_FAILED; | ||
| } |
There was a problem hiding this comment.
cc: @matejsmycka
Yannis: #3372 is about this very subject. This doesn't seem like a coincidence.
Something strange is going on behind the recent flurry of activities on libavif.
|
No worries. Closing it. Thanks for the review. |
avifParseItemLocationBox() trusts extent_count without an independent upper bound. When offset_size, length_size, base_offset_size, and index_size are all zero, each extent consumes no input bytes but still allocates an avifExtent. A 64KB input can declare up to 65535 extents per item, amplifying to multi-GB allocations.
Add a hard cap of 256 on extent_count. Legitimate AVIF files use only a handful of extents per item; 256 is far above any realistic requirement.