Skip to content

Assert the avifROStream offset invariant - #3349

Closed
WAM41 wants to merge 1 commit into
AOMediaCodec:mainfrom
WAM41:assert-rostream-offset-invariant
Closed

Assert the avifROStream offset invariant#3349
WAM41 wants to merge 1 commit into
AOMediaCodec:mainfrom
WAM41:assert-rostream-offset-invariant

Conversation

@WAM41

@WAM41 WAM41 commented Sep 4, 2026

Copy link
Copy Markdown

include/avif/internal.h documents that functions operating on an avifROStream must maintain stream->offset <= stream->raw->size.

Two functions rely on that invariant to compute a size_t difference:

avifROStreamHasBytesLeft():    byteCount <= (stream->raw->size - stream->offset)
avifROStreamRemainingBytes():  stream->raw->size - stream->offset

If the invariant were broken, both subtractions would wrap around, so the comparison in avifROStreamHasBytesLeft() would succeed for byte counts that must be rejected, and the memcpy() in avifROStreamRead() would read past the end of the buffer.

The invariant does hold today, and every path that moves an avifROStream offset preserves it: avifROStreamSetOffset() clamps to raw->size; avifROStreamSkip() and avifROStreamRead() advance only after avifROStreamHasBytesLeft() has succeeded; avifROStreamReadString() advances by at most the number of remaining bytes, because it first proves a null terminator exists within them; and the bit readers go through avifROStreamSkip() rather than touching offset. Nothing outside src/stream.c writes an avifROStream offset.

But nothing checks the invariant, so a future change that broke any of those paths would surface as an out-of-bounds read rather than as a diagnosable failure. This adds an assert() at the two places that depend on it, consistent with the existing use of assert() elsewhere in src/stream.c. Release builds are unaffected, since assert() is compiled out under NDEBUG.

Tested with dav1d 1.4.1 and libaom 3.8.2 on Ubuntu 24.04, GCC 13.3, CMAKE_BUILD_TYPE=Debug: all 51 ctest targets pass. Inverting the condition in avifROStreamHasBytesLeft() instead aborts 70 test subprocesses, and inverting it in avifROStreamRemainingBytes() aborts 62, so both functions are reached by the suite and the assertions are compiled in.

🤖 Generated with Claude Code

include/avif/internal.h documents that functions operating on an
avifROStream must maintain stream->offset <= stream->raw->size.
avifROStreamHasBytesLeft() and avifROStreamRemainingBytes() both rely on
that invariant to compute a size_t difference that would otherwise wrap.
Assert it at those two places so a violation aborts under assertions
instead of becoming an out-of-bounds read. Release builds are unaffected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@WAM41 WAM41 closed this Sep 4, 2026
@WAM41
WAM41 deleted the assert-rostream-offset-invariant branch September 4, 2026 12:34
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.

1 participant