Skip to content

Fix two issues in the new APNG handling (read_chunks bounds + progressive reader diagnostic) - #911

Open
17krishna8 wants to merge 2 commits into
pnggroup:libpng18from
17krishna8:fix/fctl-table-bounds
Open

Fix two issues in the new APNG handling (read_chunks bounds + progressive reader diagnostic)#911
17krishna8 wants to merge 2 commits into
pnggroup:libpng18from
17krishna8:fix/fctl-table-bounds

Conversation

@17krishna8

Copy link
Copy Markdown

Fix two issues in the new table-driven / progressive-reader APNG handling

While auditing the recently introduced APNG support (introduced in a92c7d7, "Introduce APNG support -- patch applied, animations unlocked!") I found two small problems. Each fix is an independent commit; both are validated below.

Commit 1: Correct contradictory fcTL length bounds in read_chunks table

CDfcTL declared max_length=25 with min_length=26. The checks in png_handle_chunk() (length < min_length -> "too short", otherwise length > max_length -> "too long") can never both pass: no chunk length satisfies 26 <= length <= 25.

An fcTL chunk is exactly 26 bytes (4-byte big-endian sequence number + 22 bytes of frame data, as consumed by png_handle_fcTL: png_ensure_sequence_number reads 4 bytes and the handler then reads data[22]). Both bounds must be 26, mirroring the CDacTL entry where acTL is exactly 8 bytes and uses equal bounds.

The entry is currently unreachable because the APNG handlers are #defined to NULL above the table and such chunks are routed through unknown-chunk handling, but the table must stay correct so that enabling native table-driven APNG dispatch later does not silently reject every valid fcTL.

Commit 2: Report a rejected fcTL accurately in the progressive reader

When png_handle_fcTL declined an fcTL (e.g. an oversized frame description for the leading frame, which produces "Ignoring leading fcTL ..."), the progressive reader aborted with "Missing required fcTL chunk in APNG stream" even though the fcTL was demonstrably present. The misleading message sends users hunting for a missing-chunk problem when the real issue is that the present chunk was not accepted. The follow-up message now says so; the accurate handler-level diagnostics ("Oversized frame in fcTL", etc.) are still emitted first.

Validation

  • Full CTest suite under -fsanitize=address,undefined: 37/37 pass at each commit
  • pngtest passes
  • Additional APNG stress corpus (10 files x sequential + progressive APIs = 20 runs) built specifically to exercise these paths: valid animation, hidden first frame, oversized fcTL, bad sequence numbers, orphan fdAT, truncated fdAT, unexpected intra-sequence chunks, interlaced animation, short acTL, short fcTL - all handled correctly, no sanitizer findings

Happy to adjust anything - thanks for reviewing!

The CDfcTL entry declared max_length=25 with min_length=26, which no
chunk length can satisfy: png_handle_chunk rejects length < 26 as "too
short" and everything else as "too long".

An fcTL chunk is exactly 26 bytes (4-byte sequence number + 22 bytes of
frame data), so both bounds must be 26, matching the CDacTL pattern
where an acTL is exactly 8 bytes.  The entry is currently unreachable
because the APNG handlers are #defined to NULL and such chunks are
routed to unknown-chunk handling, but the table must stay correct so
that enabling native table-driven APNG dispatch does not silently
reject every valid fcTL.
When png_handle_fcTL declined an fcTL chunk (for example an oversized
frame description for the leading frame), the progressive reader
reported "Missing required fcTL chunk in APNG stream" even though the
fcTL was present in the stream; the misleading message sent users
hunting for a nonexistent missing-chunk problem.

The sequential reader already aborts such streams with the accurate
"Out-of-order sequence number"/"Oversized frame in fcTL" diagnostics
produced by the handler itself, so only the follow-up message here
needs to describe the actual situation: the chunk was seen but not
accepted.
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