Fix a potential buffer overflow in the animated PNG decoder when parsing malformed fdAT chunks#186700
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements bounds checking for APNG chunk data lengths within the APNGImageGenerator to mitigate potential integer underflows and out-of-bounds memory access. It also refactors CRC32 computation into a static helper and introduces unit tests to verify the decoder's handling of malformed fdAT chunks. Feedback focuses on maintaining const-correctness for read-only buffer pointers, adhering to the Flutter style guide by documenting public members, and addressing a minor typo in the test suite.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces bounds checks in the APNG image generator to prevent integer underflows and out-of-bounds memory access when processing acTL, fcTL, and fdAT chunks. It also refactors CRC32 computation into a reusable static method and replaces magic numbers with the kFrameDataSequenceNumberSize constant. A new unit test file is added to verify the decoder's resilience against malicious APNG data. Feedback focuses on improving const-correctness for data pointers and adding documentation for new members as required by the style guide.
gaaclarke
reviewed
May 18, 2026
gaaclarke
left a comment
gaaclarke
left a comment
Member
There was a problem hiding this comment.
lgtm for the most part. i agree with the constness that gemini pointed out and some other minor notes
|
|
||
| // Writes a big-endian uint32_t to a buffer. | ||
| void WriteBE32(std::vector<uint8_t>& buf, uint32_t val) { | ||
| buf.push_back((val >> 24) & 0xFF); |
Member
There was a problem hiding this comment.
add a static assert for little endian please
static_assert(std::endian::native == std::endian::little,
"This code requires a Little Endian architecture.");
Member
Author
There was a problem hiding this comment.
This does not depend on the endianness of the host. The bytes will be written in order from most significant to least significant on any architecture.
Member
There was a problem hiding this comment.
This function has the assumption that val is in little endian. That will only be the case when the host is little endian. If we are on a big endian machine we don't have to do this swizzling.
Member
Author
There was a problem hiding this comment.
val is a uint32_t, and the shifts of val done by this function will behave consistently regardless of the host architecture's endianness.
This function writes the value of val into buf in the big-endian format required by the PNG spec. It will work as intended on both big- and little-endian architectures.
Member
There was a problem hiding this comment.
Jason's right, this will operate the same across the different machines, lgtm
bdero
reviewed
May 19, 2026
| auto make_generator = [](uint32_t fdat_length) -> auto { | ||
| auto apng_bytes = BuildMaliciousApng(fdat_length); | ||
| auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); | ||
| return APNGImageGenerator::MakeFromData(data); |
Member
There was a problem hiding this comment.
Just noting for awareness:
Up until now, tests for the APNG demuxer have lived a level above this at the codec level in engine/src/flutter/testing/dart/codec_test.dart with pre-generated APNG fixtures. This is mainly due to the image generator state machine being so intertwined with the classes driving it.
I kinda like the idea of generating bad APNGs inline, since it makes the structure of the bad file easier to decipher, and IMO since this is a "crash on initial parse" bug rather than a "crash after advancing N frames" bug... seems fine to place it this deep.
bdero
previously approved these changes
May 19, 2026
guard against fdAT data_length < 4 underflow in APNGImageGenerator::DemuxNextImage and add a ui_unittests regression test + minimal fixtures.
When converting fdAT chunks to IDAT chunks during APNG demuxing, DemuxNextImage() subtracts 4 from data_length to remove the fdAT sequence number. If an fdAT chunk has data_length < 4, this uint32_t subtraction underflows to ~4GB (0xFFFFFFFC for data_length=0). The underflowed value is then used as the length argument to memcpy, causing a heap buffer overflow. A crafted APNG image served from any HTTP server can trigger this when loaded by a Flutter application via Image.network() or similar APIs. The APNG parser is registered as a default image decoder and runs on all image data with a valid PNG signature + acTL chunk. This commit adds bounds checks in both the chunk_space calculation loop and the fdAT-to-IDAT conversion loop to reject fdAT chunks with insufficient data before the subtraction occurs. Related prior fixes in this file: - PR flutter#56928: frame offset bounds check - PR flutter#57025: integer wrapping fix for the bounds check Neither addressed the fdAT data_length underflow.
Tests that malformed APNG images with fdAT chunks whose data_length is less than the required 4 bytes (for the sequence number) do not cause crashes. Without the bounds check, the uint32_t subtraction in DemuxNextImage() underflows and causes a heap buffer overflow. Three test cases: - fdAT with data_length=0 (underflows to 0xFFFFFFFC) - fdAT with data_length=2 (underflows to 0xFFFFFFFE) - fdAT with data_length=8 (valid, should be accepted)
Extend the data_length validation to cover two additional chunk types that use CastChunkData<T>() without size checks: - acTL (line 272): reject if data_length < sizeof(AnimationControlChunkData) Prevents heap OOB read of num_frames and num_plays fields. - fcTL (line 419): reject if data_length < sizeof(FrameControlChunkData) Prevents heap OOB read of width, height, offsets, delay, and blend/dispose operation fields. Same vulnerability class as the fdAT fix in the previous commit.
* Define a constant for the size of the sequence number in the fdAT chunk * Use an assert in the chunk copying loop to confirm that the data was validated by the parser. * Add an ImageGeneratorRegistry to the test which will register the required PNG codec in Skia. * Simplify the test to create an APNG containing a single frame with the potentially malformed fdAT chunk. The original version of the test did not reach the fdAT parsing code because its first frame got an error from SkCodec::MakeFromStream. Skia could not recognize the frame because the codecs had not been registered. * Use the CRC implementation from APNGImageGenerator in the test.
Member
Author
|
Added a change to fix a clang-tidy warning (8f60267) - PTAL |
gaaclarke
approved these changes
May 21, 2026
jason-simmons
added
the
autosubmit
bdero
approved these changes
May 21, 2026
flutter-dashboard
Bot
removed
the
autosubmit
This was referenced May 22, 2026
auto-submit Bot
pushed a commit
to flutter/packages
that referenced
this pull request
May 26, 2026
Roll Flutter from e03b91f1fe34 to f3a4b9897834 (63 revisions) flutter/flutter@e03b91f...f3a4b98 2026-05-26 [email protected] Update batch release doc to reflect latest workflow (flutter/flutter#186979) 2026-05-26 [email protected] Roll Skia from 0442274cc696 to 27a819894f7c (5 revisions) (flutter/flutter#187094) 2026-05-26 [email protected] [Tool Robustness] Gracefully handle asynchronous subprocess crashes and connection timeouts (flutter/flutter#186964) 2026-05-26 [email protected] [pubspec] Bump Dart SDK constraint to ^3.13.0 (flutter/flutter#186957) 2026-05-26 [email protected] Roll Dart SDK from 7eb54169841d to 00e625453c43 (1 revision) (flutter/flutter#187086) 2026-05-26 [email protected] [Impeller] Retire Y-coord-scale plumbing by flipping GLES at the vertex stage (flutter/flutter#186556) 2026-05-26 [email protected] Roll Skia from f4f294bdf98d to 0442274cc696 (2 revisions) (flutter/flutter#187079) 2026-05-26 [email protected] [flutter_tools] Fix version cache poisoning from git environment variables (flutter/flutter#186595) 2026-05-26 [email protected] [Tool] Handle DTD connection failures gracefully in widget-preview (flutter/flutter#186952) 2026-05-25 [email protected] Roll Skia from 9d1adb5f2427 to f4f294bdf98d (1 revision) (flutter/flutter#187056) 2026-05-25 [email protected] Roll Skia from 4dd78179e6ec to 9d1adb5f2427 (1 revision) (flutter/flutter#187048) 2026-05-25 [email protected] Roll Skia from 1f26101197bf to 4dd78179e6ec (4 revisions) (flutter/flutter#187044) 2026-05-24 [email protected] Roll Skia from bbe9ccc2bdbf to 1f26101197bf (1 revision) (flutter/flutter#187016) 2026-05-24 [email protected] Roll Fuchsia Linux SDK from nsgcNDlZOuweOvy3Q... to Itd2Jq_ZIABH2rW7B... (flutter/flutter#187032) 2026-05-23 [email protected] Roll Dart SDK from 7e0f28eb5315 to 7eb54169841d (1 revision) (flutter/flutter#187005) 2026-05-23 [email protected] Roll Dart SDK from 90e55fa88456 to 7e0f28eb5315 (1 revision) (flutter/flutter#186990) 2026-05-23 [email protected] Roll Fuchsia Linux SDK from 6T6BY9PTftoG3vP_1... to nsgcNDlZOuweOvy3Q... (flutter/flutter#186984) 2026-05-23 [email protected] iOS] Migrate VSyncClient to a pure Obj-C implementation (#186166) (flutter/flutter#186935) 2026-05-23 [email protected] Disables embedder_tests.cm for fuchsia (flutter/flutter#186969) 2026-05-23 [email protected] Roll Dart SDK from b8414c46f6c7 to 90e55fa88456 (2 revisions) (flutter/flutter#186977) 2026-05-22 [email protected] Roll Skia from 6fdb013d1953 to bbe9ccc2bdbf (1 revision) (flutter/flutter#186980) 2026-05-22 [email protected] [web] Fix cutoff text in WebParagraph (flutter/flutter#186819) 2026-05-22 [email protected] fix(web): Removes the iterative downscaling hack (flutter/flutter#186914) 2026-05-22 [email protected] opts the linux embedder into sdf rendering (flutter/flutter#186909) 2026-05-22 [email protected] Roll Skia from dae8778ca40d to 6fdb013d1953 (5 revisions) (flutter/flutter#186970) 2026-05-22 [email protected] Fix hooks inputs outputs rebuilt (flutter/flutter#186701) 2026-05-22 [email protected] adds linux impeller integration test for external textures (flutter/flutter#186759) 2026-05-22 [email protected] fix(flutter_tools): defensively catch DWDS unregistered service extension errors (flutter/flutter#186896) 2026-05-22 [email protected] [Impeller] Add golden harness support to the renderer test layer (flutter/flutter#186735) 2026-05-22 [email protected] [web] Remove image codecs from canvaskit_chromium (flutter/flutter#178133) 2026-05-22 [email protected] opts all macos into wide gamut (flutter/flutter#186277) 2026-05-22 [email protected] Roll Skia from 356185490a75 to dae8778ca40d (9 revisions) (flutter/flutter#186949) 2026-05-22 [email protected] Filter out SwiftPM schemes when fetching schemes (flutter/flutter#186006) 2026-05-22 [email protected] Roll Packages from 3754d04 to 69cf959 (1 revision) (flutter/flutter#186950) 2026-05-22 [email protected] Roll Dart SDK from eca46bec956d to b8414c46f6c7 (2 revisions) (flutter/flutter#186944) 2026-05-22 [email protected] Saves a DeviceHolderVK with the CommandPoolVK (flutter/flutter#186749) 2026-05-22 [email protected] Roll Dart SDK from e0d509fd676e to eca46bec956d (1 revision) (flutter/flutter#186922) 2026-05-22 [email protected] [ Tool ] Stop generating widget preview scaffold under $TMP (flutter/flutter#186476) 2026-05-21 [email protected] Fix typo in StretchingOverscrollIndicator docs (flutter/flutter#186897) 2026-05-21 [email protected] Roll Dart SDK from 28c7cb5a8e8d to e0d509fd676e (1 revision) (flutter/flutter#186903) 2026-05-21 [email protected] Fix some issues in the integration between EmbedderExternalViewEmbedder and Impeller (flutter/flutter#184905) 2026-05-21 [email protected] Fix a potential buffer overflow in the animated PNG decoder when parsing malformed fdAT chunks (flutter/flutter#186700) 2026-05-21 [email protected] Roll Skia from 2ff20950975d to 356185490a75 (5 revisions) (flutter/flutter#186892) 2026-05-21 [email protected] Add primitive shadows to rendering benchmark (flutter/flutter#186779) 2026-05-21 [email protected] Move prefetchSwiftPackages to be per platform (flutter/flutter#186468) 2026-05-21 [email protected] Upgrade iOS version (flutter/flutter#186889) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An fdAT (frame data) chunk in an APNG should contain a sequence number
followed by image data. The APNG decoder needs to reject invalid fdAT
chunks that do not have the expected contents.
Based on #184301 and #183180
Fixes #183179