Skip to content

Reject unsupported external iloc data references #3206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jmestwa-coder wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −0
Open

Reject unsupported external iloc data references #3206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2054,6 +2054,10 @@ static avifResult avifParseItemLocationBox(avifMeta * meta, const uint8_t * raw,

uint16_t dataReferenceIndex;
AVIF_CHECKERR(avifROStreamReadU16(&s, &dataReferenceIndex), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(16) data_reference_index;
if (dataReferenceIndex != 0) {
avifDiagnosticsPrintf(diag, "Item ID [%u] contains an unsupported data reference index [%u]", itemID, dataReferenceIndex);
return AVIF_RESULT_BMFF_PARSE_FAILED;
}
Copy link
Copy Markdown
Member

@wantehchang wantehchang May 18, 2026

Choose a reason for hiding this comment

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

Thank you for the pull request. ISO/IEC 14496-12:2022 Clause 8.11.3 (Item location box) does not require or recommend that data_reference_index be set to 0 when this field is not used. Therefore I think it is better to simply ignore this field.

Note that the value 0 is a valid value for data_reference_index, so one can argue that the value 0 can't really be used to indicate that the field is not used.

Copy link
Copy Markdown
Member

@wantehchang wantehchang May 18, 2026

Choose a reason for hiding this comment

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

I took a closer look. I think the condition at line 2057 should also check that construction_method is equal to 0:

        if (constructionMethod == 0 && dataReferenceIndex != 0) {

But it is not clear what value constructionMethod should be set to for version 0. Do you know?

Copy link
Copy Markdown
Author

@jmestwa-coder jmestwa-coder May 23, 2026

Choose a reason for hiding this comment

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

Thanks for taking a closer look.

From what I understand, construction_method is only explicitly present starting from iloc version 1/2, so for version 0 I’m not fully sure what the intended interpretation should be either.

I’ve updated the test to make the fixture assumptions explicit before mutating data_reference_index.

uint64_t baseOffset;
AVIF_CHECKERR(avifROStreamReadUX8(&s, &baseOffset, baseOffsetSize), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(base_offset_size*8) base_offset;
uint16_t extentCount;
Expand Down
32 changes: 32 additions & 0 deletions tests/gtest/avifilocextenttest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright 2024 Google LLC
// SPDX-License-Identifier: BSD-2-Clause

#include <algorithm>
#include <string>

#include "avif/avif.h"
#include "aviftest_helpers.h"
#include "gtest/gtest.h"
Expand Down Expand Up @@ -30,6 +33,35 @@ TEST(IlocTest, TwoExtents) {
EXPECT_LT(psnr, 45.0);
}

TEST(IlocTest, NonZeroDataReferenceIndex) {
testutil::AvifRwData avif =
testutil::ReadFile(std::string(data_path) + "white_1x1.avif");
ASSERT_NE(avif.data, nullptr);

const uint8_t kIloc[] = {'i', 'l', 'o', 'c'};
uint8_t* iloc_position =
std::search(avif.data, avif.data + avif.size, kIloc, kIloc + 4);
ASSERT_NE(iloc_position, avif.data + avif.size);
ASSERT_GE(static_cast<size_t>(avif.data + avif.size - iloc_position),
size_t{16});

// white_1x1.avif uses iloc version 0 with a single item. The
// data_reference_index field follows the item_ID field.
ASSERT_EQ(iloc_position[4], 0);
ASSERT_EQ(iloc_position[10], 0);
ASSERT_EQ(iloc_position[11], 1);
ASSERT_EQ(iloc_position[14], 0);
ASSERT_EQ(iloc_position[15], 0);
iloc_position[14] = 0;
iloc_position[15] = 1;
Copy link
Copy Markdown
Member

@wantehchang wantehchang May 18, 2026

Choose a reason for hiding this comment

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

I suggest also verifying data_reference_index is equal to 0 before changing it to 1:

   ASSERT_EQ(iloc_position[4], 0);
+  ASSERT_EQ(iloc_position[14], 0);
+  ASSERT_EQ(iloc_position[15], 0);
   iloc_position[14] = 0;
   iloc_position[15] = 1;

Copy link
Copy Markdown
Author

@jmestwa-coder jmestwa-coder May 23, 2026

Choose a reason for hiding this comment

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

I verified the fixture layout and added assertions checking the original data_reference_index value before mutating it.


DecoderPtr decoder(avifDecoderCreate());
ASSERT_NE(decoder, nullptr);
ASSERT_EQ(avifDecoderSetIOMemory(decoder.get(), avif.data, avif.size),
AVIF_RESULT_OK);
EXPECT_EQ(avifDecoderParse(decoder.get()), AVIF_RESULT_BMFF_PARSE_FAILED);
}

//------------------------------------------------------------------------------

} // namespace
Expand Down