Guard against width/height overflow in SkIRect::subtract
SkRectPriv::Subtract had assumed that Sk[I]Rect::Intersects returns
false if either argument is empty. However, when an SkIRect with
overflowing dimensions is intersected with a non-overflowing SkIRect,
then the intersection can be valid and the remainder of the impl's
expectations are violated because dimensions are "negative".
Since these overflowing rects are considered empty anyways, this just
explicitly checks for that.
Bug: chromium:1243206
Change-Id: I8b69731e8d8ae467cf98c906da3aaa657dfe7994
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442277
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
diff --git a/src/core/SkRect.cpp b/src/core/SkRect.cpp
index 2ff75ad..9e40691 100644
--- a/src/core/SkRect.cpp
+++ b/src/core/SkRect.cpp
@@ -174,7 +174,7 @@
static bool subtract(const R& a, const R& b, R* out) {
static constexpr C kZero = C(0);
- if (!R::Intersects(a, b)) {
+ if (a.isEmpty() || b.isEmpty() || !R::Intersects(a, b)) {
// Either already empty, or subtracting the empty rect, or there's no intersection, so
// in all cases the answer is A.
*out = a;
diff --git a/tests/RectTest.cpp b/tests/RectTest.cpp
index 8be796c..02fb161 100644
--- a/tests/RectTest.cpp
+++ b/tests/RectTest.cpp
@@ -12,6 +12,8 @@
#include "src/core/SkRectPriv.h"
#include "tests/Test.h"
+#include <limits.h>
+
static bool has_green_pixels(const SkBitmap& bm) {
for (int j = 0; j < bm.height(); ++j) {
for (int i = 0; i < bm.width(); ++i) {
@@ -240,6 +242,32 @@
}
}
+DEF_TEST(Rect_subtract_overflow, reporter) {
+ // This rectangle is sorted but whose int32 width overflows and appears negative (so
+ // isEmpty() returns true).
+ SkIRect reallyBig = SkIRect::MakeLTRB(-INT_MAX + 1000, 0, INT_MAX - 1000, 100);
+ // However, because it's sorted, an intersection with a reasonably sized rectangle is still
+ // valid so the assumption that SkIRect::Intersects() returns false when either input is
+ // empty is invalid, leading to incorrect use of negative width (see crbug.com/1243206)
+ SkIRect reasonable = SkIRect::MakeLTRB(-50, -5, 50, 125);
+
+ // Ignoring overflow, "reallyBig - reasonable" should report exact = false and select either the
+ // left or right portion of 'reallyBig' that excludes 'reasonable', e.g.
+ // {-INT_MAX+1000, 0, -50, 100} or {150, 0, INT_MAX-1000, 100}.
+ // This used to assert, but now it should be detected that 'reallyBig' overflows and is
+ // technically empty, so the result should be itself and exact.
+ SkIRect difference;
+ bool exact = SkRectPriv::Subtract(reallyBig, reasonable, &difference);
+ REPORTER_ASSERT(reporter, exact);
+ REPORTER_ASSERT(reporter, difference == reallyBig);
+
+ // Similarly, if we subtract 'reallyBig', since it's technically empty then we expect the
+ // answer to remain 'reasonable'.
+ exact = SkRectPriv::Subtract(reasonable, reallyBig, &difference);
+ REPORTER_ASSERT(reporter, exact);
+ REPORTER_ASSERT(reporter, difference == reasonable);
+}
+
#include "include/core/SkSurface.h"
// Before the fix, this sequence would trigger a release_assert in the Tiler