Skip to content

Commit 42d3d75

Browse files
[CP-stable]Check for overflow when computing the pixel buffer size for an animated PNG frame (flutter#185621)
### 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
1 parent 02085fe commit 42d3d75

5 files changed

Lines changed: 133 additions & 3 deletions

File tree

engine/src/flutter/fml/BUILD.gn

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ source_set("fml") {
6767
"process.h",
6868
"raster_thread_merger.cc",
6969
"raster_thread_merger.h",
70+
"safe_math.cc",
71+
"safe_math.h",
7072
"shared_thread_merger.cc",
7173
"shared_thread_merger.h",
7274
"status.h",
@@ -111,7 +113,7 @@ source_set("fml") {
111113
":string_conversion",
112114
]
113115

114-
deps = []
116+
deps = [ "//third_party/abseil-cpp/absl/numeric:int128" ]
115117
if (target_os != "wasm") {
116118
deps += [ "//flutter/third_party/icu" ]
117119

@@ -350,6 +352,7 @@ if (enable_unittests) {
350352
"message_loop_unittests.cc",
351353
"paths_unittests.cc",
352354
"raster_thread_merger_unittests.cc",
355+
"safe_math_unittests.cc",
353356
"string_conversion_unittests.cc",
354357
"synchronization/count_down_latch_unittests.cc",
355358
"synchronization/semaphore_unittest.cc",
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include <limits>
6+
7+
#include "flutter/fml/safe_math.h"
8+
#include "third_party/abseil-cpp/absl/numeric/int128.h"
9+
10+
namespace fml {
11+
12+
size_t SafeMath::mul(size_t x, size_t y) {
13+
return sizeof(size_t) == sizeof(uint64_t) ? mul64(x, y) : mul32(x, y);
14+
}
15+
16+
uint32_t SafeMath::mul32(uint32_t x, uint32_t y) {
17+
uint64_t big_x = x;
18+
uint64_t big_y = y;
19+
uint64_t result = big_x * big_y;
20+
if (result >> 32) {
21+
overflow_detected_ = true;
22+
}
23+
return static_cast<uint32_t>(result);
24+
}
25+
26+
uint64_t SafeMath::mul64(uint64_t x, uint64_t y) {
27+
if (x <= std::numeric_limits<uint32_t>::max() &&
28+
y <= std::numeric_limits<uint32_t>::max()) {
29+
return x * y;
30+
}
31+
32+
absl::uint128 big_x = x;
33+
absl::uint128 big_y = y;
34+
absl::uint128 result = big_x * big_y;
35+
if (absl::Uint128High64(result)) {
36+
overflow_detected_ = true;
37+
}
38+
return absl::Uint128Low64(result);
39+
}
40+
41+
} // namespace fml

engine/src/flutter/fml/safe_math.h

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#ifndef FLUTTER_FML_SAFE_MATH_H_
6+
#define FLUTTER_FML_SAFE_MATH_H_
7+
8+
#include <cstddef>
9+
#include <cstdint>
10+
11+
namespace fml {
12+
13+
// Math operations that check for overflow.
14+
// Based on Skia's SkSafeMath.
15+
class SafeMath {
16+
public:
17+
bool overflow_detected() const { return overflow_detected_; }
18+
19+
size_t mul(size_t x, size_t y);
20+
21+
private:
22+
uint32_t mul32(uint32_t x, uint32_t y);
23+
uint64_t mul64(uint64_t x, uint64_t y);
24+
25+
bool overflow_detected_ = false;
26+
};
27+
28+
} // namespace fml
29+
30+
#endif // FLUTTER_FML_SAFE_MATH_H_
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include <limits>
6+
7+
#include "flutter/fml/safe_math.h"
8+
#include "gtest/gtest.h"
9+
10+
namespace flutter {
11+
namespace testing {
12+
13+
TEST(SafeMathTest, MultiplySizeT) {
14+
// Multiplication with no overflow.
15+
fml::SafeMath safe1;
16+
EXPECT_EQ(safe1.mul(1000, 2000), static_cast<size_t>(2000000));
17+
EXPECT_FALSE(safe1.overflow_detected());
18+
19+
// Overflow detection when multiplying size_t values at or near the maximum.
20+
fml::SafeMath safe2;
21+
safe2.mul(std::numeric_limits<size_t>::max(),
22+
std::numeric_limits<size_t>::max());
23+
EXPECT_TRUE(safe2.overflow_detected());
24+
25+
fml::SafeMath safe3;
26+
safe3.mul(std::numeric_limits<size_t>::max() >> 2, 5);
27+
EXPECT_TRUE(safe3.overflow_detected());
28+
29+
// Overflow detection for a result that slightly exceeds the range of a
30+
// uint64_t.
31+
if (sizeof(size_t) == sizeof(uint64_t)) {
32+
fml::SafeMath safe4;
33+
safe4.mul(static_cast<size_t>(1ULL << 32), static_cast<size_t>(1ULL << 32));
34+
EXPECT_TRUE(safe4.overflow_detected());
35+
}
36+
}
37+
38+
} // namespace testing
39+
} // namespace flutter

engine/src/flutter/lib/ui/painting/image_generator_apng.cc

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <cstring>
88

99
#include "flutter/fml/logging.h"
10+
#include "flutter/fml/safe_math.h"
1011
#include "third_party/skia/include/codec/SkCodec.h"
1112
#include "third_party/skia/include/codec/SkCodecAnimation.h"
1213
#include "third_party/skia/include/core/SkAlphaType.h"
@@ -91,10 +92,26 @@ bool APNGImageGenerator::GetPixels(const SkImageInfo& info,
9192

9293
APNGImage& frame = images_[image_index];
9394
SkImageInfo frame_info = frame.codec->getInfo();
94-
auto frame_row_bytes = frame_info.bytesPerPixel() * frame_info.width();
95+
96+
fml::SafeMath safe;
97+
size_t frame_row_bytes =
98+
safe.mul(frame_info.bytesPerPixel(), frame_info.width());
99+
if (safe.overflow_detected()) {
100+
FML_DLOG(ERROR) << "Failed to decode image at index " << image_index
101+
<< " (frame index: " << frame_index
102+
<< ") of APNG due to frame row bytes overflow.";
103+
return false;
104+
}
95105

96106
if (frame.pixels.empty()) {
97-
frame.pixels.resize(frame_row_bytes * frame_info.height());
107+
size_t pixels_bytes = safe.mul(frame_row_bytes, frame_info.height());
108+
if (safe.overflow_detected()) {
109+
FML_DLOG(ERROR) << "Failed to decode image at index " << image_index
110+
<< " (frame index: " << frame_index
111+
<< ") of APNG due to pixel buffer size overflow.";
112+
return false;
113+
}
114+
frame.pixels.resize(pixels_bytes);
98115
SkCodec::Result result = frame.codec->getPixels(
99116
frame.codec->getInfo(), frame.pixels.data(), frame_row_bytes);
100117
if (result != SkCodec::kSuccess) {

0 commit comments

Comments
 (0)