[CP-beta]Check for overflow when computing the pixel buffer size for an animated PNG frame#185620
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 class in the fml library to handle overflow-safe multiplication for size_t, uint32_t, and uint64_t types, and integrates it into APNGImageGenerator for safer buffer size calculations. Review feedback suggests using if constexpr in the mul method to avoid narrowing conversion warnings and notes that public members of the new class require documentation to comply with the style guide.
| 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.
The ternary operator here can lead to narrowing conversion warnings or errors on 64-bit systems because mul32 expects uint32_t arguments, and x and y are size_t (64-bit). Even though the branch is not taken at runtime, the compiler still checks the types. Using if constexpr ensures that only the relevant branch is compiled, avoiding these issues.
| return sizeof(size_t) == sizeof(uint64_t) ? mul64(x, y) : mul32(x, y); | |
| if constexpr (sizeof(size_t) == sizeof(uint64_t)) { | |
| return mul64(x, y); | |
| } else { | |
| return mul32(x, y); | |
| } |
|
|
||
| // Math operations that check for overflow. | ||
| // Based on Skia's SkSafeMath. | ||
| class SafeMath { |
There was a problem hiding this comment.
According to the Flutter style guide, all public members should have documentation. Please add a documentation comment for the SafeMath class.
References
- All public members should have documentation. (link)
| // Based on Skia's SkSafeMath. | ||
| class SafeMath { | ||
| public: | ||
| bool overflow_detected() const { return overflow_detected_; } |
There was a problem hiding this comment.
According to the Flutter style guide, all public members should have documentation. Please add a documentation comment for the overflow_detected method.
References
- All public members should have documentation. (link)
| public: | ||
| bool overflow_detected() const { return overflow_detected_; } | ||
|
|
||
| size_t mul(size_t x, size_t y); |
There was a problem hiding this comment.
According to the Flutter style guide, all public members should have documentation. Please add a documentation comment for the mul method.
References
- All public members should have documentation. (link)
c8cc698
into
flutter:flutter-3.44-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