[CP-stable]Check for overflow when computing the pixel buffer size for an animated PNG frame#185621
Conversation
…ed PNG frame (flutter#185442) See internal issue b/489180577
|
@jason-simmons please fill out the PR description above, afterwards the release team will review this request. |
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
There was a problem hiding this comment.
Code Review
This pull request introduces a SafeMath utility class to the fml library, providing overflow-checked multiplication for size_t, uint32_t, and uint64_t types using Abseil's uint128 for 64-bit checks. The utility is applied within the APNG image generator to prevent overflows when calculating row bytes and pixel buffer sizes. Review feedback suggests improving documentation by using /// for all members as per the style guide, adding [[nodiscard]] to methods, and refactoring the mul implementation to use if constexpr for better type safety and branch optimization.
| // Math operations that check for overflow. | ||
| // Based on Skia's SkSafeMath. | ||
| class SafeMath { | ||
| public: | ||
| bool overflow_detected() const { return overflow_detected_; } | ||
|
|
||
| size_t mul(size_t x, size_t y); |
There was a problem hiding this comment.
The Flutter style guide requires all public members to have documentation using ///. Additionally, it is good practice to mark methods that return a value that should not be ignored with [[nodiscard]].
| // Math operations that check for overflow. | |
| // Based on Skia's SkSafeMath. | |
| class SafeMath { | |
| public: | |
| bool overflow_detected() const { return overflow_detected_; } | |
| size_t mul(size_t x, size_t y); | |
| /// Math operations that check for overflow. | |
| /// Based on Skia's SkSafeMath. | |
| class SafeMath { | |
| public: | |
| /// Returns true if any operation performed by this instance has overflowed. | |
| [[nodiscard]] bool overflow_detected() const { return overflow_detected_; } | |
| /// Multiplies two size_t values and returns the result. | |
| /// | |
| /// If the multiplication overflows, the internal overflow state is set to true. | |
| [[nodiscard]] size_t mul(size_t x, size_t y); |
References
- All public members should have documentation. Use /// for public-quality documentation. (link)
| uint32_t mul32(uint32_t x, uint32_t y); | ||
| uint64_t mul64(uint64_t x, uint64_t y); |
There was a problem hiding this comment.
The style guide recommends using /// for documentation even on private members.
| uint32_t mul32(uint32_t x, uint32_t y); | |
| uint64_t mul64(uint64_t x, uint64_t y); | |
| /// Multiplies two uint32_t values and checks for overflow. | |
| uint32_t mul32(uint32_t x, uint32_t y); | |
| /// Multiplies two uint64_t values and checks for overflow. | |
| uint64_t mul64(uint64_t x, uint64_t y); |
References
- Use /// for public-quality documentation, even on private members. (link)
| namespace fml { | ||
|
|
||
| size_t SafeMath::mul(size_t x, size_t y) { | ||
| return sizeof(size_t) == sizeof(uint64_t) ? mul64(x, y) : mul32(x, y); |
There was a problem hiding this comment.
Using if constexpr with explicit casts is preferred to avoid potential narrowing conversion warnings and to ensure that only the relevant branch is instantiated for the target architecture's size_t size.
| return sizeof(size_t) == sizeof(uint64_t) ? mul64(x, y) : mul32(x, y); | |
| if constexpr (sizeof(size_t) == sizeof(uint64_t)) { | |
| return static_cast<size_t>(mul64(x, y)); | |
| } else { | |
| return static_cast<size_t>(mul32(x, y)); | |
| } |
42d3d75
into
flutter:flutter-3.41-candidate.0
Issue Link:
What is the link to the issue this cherry-pick is addressing?
http://b/489180577
Impact Description:
Reliability improvement in the animated PNG decoder
Changelog Description:
Fixes a potential integer overflow that could happen when handling some animated PNG files.
Workaround:
Is there a workaround for this issue?
No
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
What are the steps to validate that this fix works?
See http://b/489180577