Bail out of avifDecoderParse() if an essential-required item property is not flagged as essential

[joedrago]

This should ensure that all av1C property associations are flagged as essential.

@wantehchang @cconcolato are there other properties we should enforce this way, while we're at it?

[baumanj]

Suggested change

	"av1C" // AVIF: Section 2.2.1: "This property shall be marked as essential."
	"av1C" // AVIF: Section 2.2.1: "This property shall be marked as essential."
	"a1op" // AVIF: Section 2.3.2.1: "If associated, it shall be marked as essential."

Not sure if a1op needs to be handled differently here.

Also, should there be an opposite check for a1lx since the spec says "If associated, it shall not be marked as essential"

[joedrago]

Hmm. I'm thinking about removing lsel from the list for now and just adding boxes to this list as I add support for new boxes. lsel isn't written or read at all right now, so it seems odd to preemptively block a read on it.

[joedrago]

One issue with this is any file created with a sufficiently old avifenc (prior to v0.7.2) will break with this change, as av1C weren't being properly flagged as essential until this commit:

0da2997

This means I have to fix the IO data checked into tests and oss-fuzz's seed corpus.

[wantehchang]

I see. Let's also check the AVIF encoder in libheif.

[wantehchang]

The libheif function to inspect is Box_ipma::add_property_for_item_ID(), declared in libheif/box.h:

  class Box_ipma : public Box
  {
  public:
    ...

    struct PropertyAssociation
    {
      bool essential;
      uint16_t property_index;
    };

    ...

    void add_property_for_item_ID(heif_item_id itemID,
                                  PropertyAssociation assoc);

The relevant libheif code is in libheif/heif_file.cc:

void HeifFile::add_av1C_property(heif_item_id id)
{
  auto av1C = std::make_shared<Box_av1C>();
  int index = m_ipco_box->append_child_box(av1C);

  m_ipma_box->add_property_for_item_ID(id, Box_ipma::PropertyAssociation{true, uint16_t(index + 1)});
}

The code was originally added in this commit, with essential set to true:
strukturag/libheif@1059280

So the AVIF encoder in libheif marks av1C as essential.

[joedrago]

I've submitted a PR to oss-fuzz to correct the seed corpus. It is the same content, but with updated shasums due to the updated essential bit being set for av1C properties.

[joedrago]

I think this is a good change in the longrun, but I admit to being a bit concerned that there will be some "old" AVIFs (generated with avifenc prior to v0.7.2) that will "break".

Perhaps before rolling this out (or alongside it), we should offer some kind of cheap fixup tool that toggles the correct essential bit in "old" files?

[joedrago]

I've created a standalone script to fix old AVIFs with this issue. Should I check this into libavif somewhere?

fix_essential.zip