Skip to content

Reject an item that is the alpha auxiliary of several tiles - #3341

Closed
Nexory wants to merge 1 commit into
AOMediaCodec:mainfrom
Nexory:alpha-item-shared-between-tiles
Closed

Reject an item that is the alpha auxiliary of several tiles#3341
Nexory wants to merge 1 commit into
AOMediaCodec:mainfrom
Nexory:alpha-item-shared-between-tiles

Conversation

@Nexory

@Nexory Nexory commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

@wantehchang asked me to upload this with the test file for @vigneshvg and
@y-guyon to review. It replaces the temporary workaround in eb67309 with a fix
for the cause. #3339 solves the same assertion the other way round, by allowing
the gap instead of refusing the file.

I want to be straight about what this does and does not settle.

When the color item is a grid and each tile carries its own alpha auxiliary item,
avifMetaFindAlphaItem() builds a synthetic alpha grid and gives each of those
items its own dimg index. The loop rejects two alpha items for one tile, an alpha
item that is already a dimg of something else, and a repeated dimg index, but not
one item serving several tiles. The second assignment overwrites the dimg index
written by the first, so the alpha grid ends up with fewer tiles than it declares
and avifFillDimgIdxToItemIdxArray() fails its assertion.

Before 2f8eb91 this was unreachable: avifDecoderItemIsAlphaAux() read a single
valued auxForID that the parsing loop overwrote per entry, so an item could be the
alpha auxiliary of exactly one item. Keeping every reference made the relation one
to many and exposed the missing guard. The dimg parsing itself is unchanged; these
associations are written here, not parsed, and which items receive them follows
from the auxl relation.

2f8eb91 is not contained in any release tag, so no released version is affected.

What this patch does not settle is that such a file is arguably well formed. The
commit message of 2f8eb91 argued from section 8.11.12.1 that one item can be the
source of several references of a type, which is exactly what the test file does,
and refusing it narrows what that commit set out to keep. Supporting it would mean
putting the same alpha item in both cells, and the obstacle is not the guard: the
cell to item mapping is written back through avifDecoderItem::dimgForID and
::dimgIdx, which hold one value each and are read in more than a dozen other
places, including gain maps and sample transforms. The correct mapping already
exists in avifMetaFindAlphaItem() as dimgIdxToAlphaItemIdx and is discarded. I did
not take that on: it is a change to the item model, and #3310 is already open on
the reset behaviour of this same function.

So this is the narrow fix. It refuses one shape instead of decoding it, and I
would rather say that plainly than present it as the only reading.

With the guard in place the loop can call avifDecoderItemIsAlphaAux() again, so
this also removes avifReferencesOld() and avifDecoderItemIsAlphaAuxOld(). Against
the tree before eb67309 the net change is the guard alone.

Three things this gains over the workaround. avifReferencesOld() walks the
references backwards and returns on the first entry of the type, so only the last
auxl reference counts, which is the behaviour 2f8eb91 set out to remove; the guard
leaves avifDecoderItemIsAlphaAux() intact and that reduction does not come back.
The caller is told instead of being handed a different picture: on the current
main the test file decodes with exit 0 and no alpha at all, where this returns an
error. And there is one code path again rather than a second one kept alongside
for a single loop.

The test file differs from color_grid_alpha_nogrid.avif in a single byte, 0x06 to
0x05 at offset 379. On the current main the workaround keeps the assertion from
firing, but the file is still accepted and decodes with its alpha channel silently
dropped: avifdec reports Alpha Absent for it and Not premultiplied for the
unmodified file.

On #3339 I measured that skipping an unset cell also moves the tiles after it,
because the copy places a tile by its loop counter rather than by its cell index,
so the surviving tile lands in the wrong cell and the empty one is never written.

Measured on this branch: without the patch the new test fails, expecting 18 and
getting 0, and with it the four DimgTest cases pass. The suite is 63 of 63 both
with the patch and on an untouched checkout of main, and clang-format 22.1.8 is
clean on both changed files. Not measured: fault injection, a positive control,
and a run in the CI image.

The error code follows the block it joins and returns AVIF_RESULT_INVALID_IMAGE_GRID.
AVIF_RESULT_NOT_IMPLEMENTED would match the tile that is a dimg of two grids; say
which you prefer and I will change it.

Bug: https://g-issues.oss-fuzz.com/issues/553221196

When the color item is a grid and each tile carries its own alpha auxiliary
item, avifMetaFindAlphaItem() builds a synthetic alpha grid and gives each of
those items its own dimg index. The loop rejects two alpha items for one tile,
an alpha item that is already a dimg for something else, and a repeated dimg
index, but not one item serving several tiles. The second assignment then
overwrites the dimg index written by the first, so the alpha grid ends up with
fewer tiles than it declares and avifFillDimgIdxToItemIdxArray() fails its
assertion on numTiles == numExpectedTiles.

Before 2f8eb91 this was unreachable. avifDecoderItemIsAlphaAux() read a single
valued auxForID that the parsing loop overwrote per entry, so an item could be
the alpha auxiliary of exactly one item, whichever was parsed last. Keeping
every reference made the relation one to many and exposed the missing guard.
The 'dimg' parsing itself is unchanged: these dimg associations are not parsed
from the file, they are written here, and which items receive them follows from
the 'auxl' relation.

This also removes avifReferencesOld() and avifDecoderItemIsAlphaAuxOld(), added
in eb67309 to suppress the assertion while the cause was being investigated.
With the guard in place the loop can call avifDecoderItemIsAlphaAux() again.

The new test file differs from color_grid_alpha_nogrid.avif in one field: the
from_item_ID of the second auxl box, 6 changed to 5, so item 5 is the alpha
auxiliary of both tiles. Without this patch and without the temporary
workaround it aborts in avifFillDimgIdxToItemIdxArray().

Bug: https://g-issues.oss-fuzz.com/issues/553221196

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This review was prepared with OpenAI Codex assistance under the account owner's authorization.

I reviewed exact head e8eb73c9b5775cc637e2522d348f4626b8d391a5 against base eb673097950e0b0ec77f696958091ef0244a45a9 and the current GitHub merge ref 3bcb05ed9da7566c34b1baa04aa2abde259195f8.

The new guard correctly rejects reuse of one alpha auxiliary item by multiple color-grid tiles before the synthetic alpha grid can be left with fewer unique tiles than declared. An exact-revision decode oracle showed the malformed fixture changing from OK on base to Invalid image grid on both head and current merge, while the valid alpha-grid case and the existing repeated/reversed/shared-item controls retained their prior results.

Local verification with libaom decoding enabled:

  • full avifdimgtest: base 3/3, head 4/4, current merge 4/4
  • git diff --check: clean
  • live CI: 25/25 checks successful, with no pending or action-required checks

No blocking findings.

@Nexory

Nexory commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Obsolete now that #3342 has landed, so closing this.

The workaround it removed is already gone: avifReferencesOld and
avifDecoderItemIsAlphaAuxOld no longer appear in main. And the guard it added
is no longer the right answer, because an item being an input of several derived
items is now expressible rather than something to reject.

@Nexory Nexory closed this Sep 1, 2026
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.

2 participants