Skip to content

Bound iloc extent_count to prevent memory amplification - #3375

Closed
littlepig12345 wants to merge 4 commits into
AOMediaCodec:mainfrom
littlepig12345:main
Closed

littlepig12345 wants to merge 4 commits into
AOMediaCodec:mainfrom
littlepig12345:main

Conversation

@littlepig12345

Copy link
Copy Markdown

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.

@y-guyon

y-guyon commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

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 avifExtent struct size itself generating multi-GB allocations? I would have thought that zero-size extents would not allocate anything on the heap.

@littlepig12345

Copy link
Copy Markdown
Author

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:

if ((version == 0 || indexSize == 0) && offsetSize == 0 && lengthSize == 0) {
    extentCount = 0;
}

@y-guyon

y-guyon commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

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.

@littlepig12345

Copy link
Copy Markdown
Author

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:
item_count = 10000
extent_count = 65535
offset_size = 0, length_size = 0, base_offset_size = 0, index_size = 0

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" used avifdec.exe from the official libavif release page (latest release).
"After the patch" used avifdec.exe built from my fork's CI artifacts, with the patch applied.

Before the patch:
Peak WorkingSet: ~9541.95 MB (system-dependent; the allocation continues until OOM)
Elapsed: ~5.963 s
Result: "Out of memory"

After the patch:
Peak WorkingSet: ~15.2 MB
Elapsed: ~0.06 s
Result: "Missing or empty image item" (the zero-size extents are skipped, so the item has no extents)

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.

@y-guyon

y-guyon commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Thank you for the explanation. I understand for length_size and/or length being zero but I still fail to see the link to offset and index.

The current patch also does not skip extents where length_size!=0 and extent_length=0. If I understand correctly, it is less of an issue because it means the iloc box itself will be much larger.

This also makes me think about overlapping extents causing similar effects. If extents can be signaled as overlapping in iloc, we should check what happens and if we should accept extents pointing to the contents of an mdat box smaller than the sum of their lengths. That is out of scope of this PR.

@littlepig12345

Copy link
Copy Markdown
Author

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.

@y-guyon

y-guyon commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Rereading ISOBMFF 14496-12 Section 8.11.3.2.4.3 "Extent length":

A value of 0 for extent_length is interpreted as follows: [...] the length of the extent is assumed to be the length of the data between the offset (if specified) or the origin (if not specified), and the end of the file [...]

So extents with length_size or extent_length=0 actually contain bytes. But this is not supported by libavif. So rather than skipping, libavif should return NOT_IMPLEMENTED (or implement it).

@littlepig12345

Copy link
Copy Markdown
Author

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:

  1. Zero-byte amplification: when all size fields are zero and extentCount > 0, return AVIF_RESULT_BMFF_PARSE_FAILED. This is the path my local test triggers.

  2. Unsupported semantics: when length_size > 0 and extent_length == 0, return AVIF_RESULT_NOT_IMPLEMENTED, since libavif does not support the "to end of file" meaning.

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.

@littlepig12345

Copy link
Copy Markdown
Author

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 extent_length:

// 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:

  • No arbitrary cap.
  • indexSize used directly (initialized to 0 for version 0).
  • uint32_t for bytesPerExtent (max 24).
  • extentCount == 1 allowed.
  • extent_length == 0 returns AVIF_RESULT_NOT_IMPLEMENTED per ISO/IEC 14496-12 Section 8.11.3.2.4.3.

Test results with item_count = 10000, extent_count = 65535, all size fields 0 (~59 KB input):

Before After
Peak WorkingSet ~9542 MB ~2.92 MB
Elapsed ~5.96 s ~0.044 s
Result Out of memory BMFF parsing failed

Diagnostics:

ERROR: Failed to parse image: BMFF parsing failed
Diagnostics:
 * Item ID [1] declares 65535 extents that consume no input bytes

Valid AVIF files with genuine multi-byte extents still parse unchanged.

Comment thread src/read.c
extentCount,
remainingBytes);
return AVIF_RESULT_BMFF_PARSE_FAILED;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well its the usage of ai....

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3372 is about this very subject

Right, I did not look into #3372, sorry for the double work. Since this PR is about the same topic and was open after #3372, let's close this PR.

@littlepig12345

Copy link
Copy Markdown
Author

No worries. Closing it. Thanks for the review.

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.

4 participants