Extra safety for SkRRect.

This moves closer to ensuring that all SkRRects are valid. It also checks for validity of deserialized SkRRects and sets the SkRRect to empty if the serialized data is invalid rather than asserting.

It is still possible to use mutators to create invalid SkRRects (e.g. outset() by large number, translate() so that type changes due to fp precision).


Bug: skia:
Change-Id: Ice5f73a020e99739ef4b3ce362181d3dbb35701c
Reviewed-on: https://skia-review.googlesource.com/49220
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

3 files changed, 88 insertions, 8 deletions

diff --git a/include/core/SkRRect.h b/include/core/SkRRect.h
index f359c9c167..eb983b6c0e 100644
--- a/include/core/SkRRect.h
+++ b/include/core/SkRRect.h
@@ -46,7 +46,7 @@ class SkMatrix;
 */
 class SK_API SkRRect {
 public:
-    SkRRect() { /* unititialized */ }
+    SkRRect() { this->setEmpty(); }
     SkRRect(const SkRRect&) = default;
     SkRRect& operator=(const SkRRect&) = default;
 
@@ -135,7 +135,7 @@ public:
         fRect = rect;
         fRect.sort();
 
-        if (fRect.isEmpty()) {
+        if (fRect.isEmpty() || !fRect.isFinite()) {
             this->setEmpty();
             return;
         }
@@ -178,7 +178,7 @@ public:
         fRect = oval;
         fRect.sort();
 
-        if (fRect.isEmpty()) {
+        if (fRect.isEmpty() || !fRect.isFinite()) {
             this->setEmpty();
             return;
         }
@@ -290,6 +290,7 @@ public:
     bool contains(const SkRect& rect) const;
 
     bool isValid() const;
+    static bool AreRectAndRadiiValid(const SkRect&, const SkVector[4]);
 
     enum {
         kSizeInMemory = 12 * sizeof(SkScalar)
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index 730868a440..f1b85fbc28 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -465,7 +465,12 @@ size_t SkRRect::readFromMemory(const void* buffer, size_t length) {
     if (length < kSizeInMemory) {
         return 0;
     }
-
+    // Note that the buffer may be smaller than SkRRect. It is important not to access
+    // bufferAsRRect->fType.
+    const SkRRect* bufferAsRRect = reinterpret_cast<const SkRRect*>(buffer);
+    if (!AreRectAndRadiiValid(bufferAsRRect->fRect, bufferAsRRect->fRadii)) {
+        return 0;
+    }
     // Deserialize rect and corners, then rederive the type tag.
     memcpy(this, buffer, kSizeInMemory);
     this->computeType();
@@ -505,6 +510,10 @@ static bool are_radius_check_predicates_valid(SkScalar rad, SkScalar min, SkScal
 }
 
 bool SkRRect::isValid() const {
+    if (!AreRectAndRadiiValid(fRect, fRadii)) {
+        return false;
+    }
+
     bool allRadiiZero = (0 == fRadii[0].fX && 0 == fRadii[0].fY);
     bool allCornersSquare = (0 == fRadii[0].fX || 0 == fRadii[0].fY);
     bool allRadiiSame = true;
@@ -570,14 +579,19 @@ bool SkRRect::isValid() const {
             break;
     }
 
+    return true;
+}
+
+bool SkRRect::AreRectAndRadiiValid(const SkRect& rect, const SkVector radii[4]) {
+    if (!rect.isFinite()) {
+        return false;
+    }
     for (int i = 0; i < 4; ++i) {
-        if (!are_radius_check_predicates_valid(fRadii[i].fX, fRect.fLeft, fRect.fRight) ||
-            !are_radius_check_predicates_valid(fRadii[i].fY, fRect.fTop, fRect.fBottom)) {
+        if (!are_radius_check_predicates_valid(radii[i].fX, rect.fLeft, rect.fRight) ||
+            !are_radius_check_predicates_valid(radii[i].fY, rect.fTop, rect.fBottom)) {
             return false;
         }
     }
-
     return true;
 }
-
 ///////////////////////////////////////////////////////////////////////////////
diff --git a/tests/RoundRectTest.cpp b/tests/RoundRectTest.cpp
index ce9744571b..d9b7a9aa08 100644
--- a/tests/RoundRectTest.cpp
+++ b/tests/RoundRectTest.cpp
@@ -722,6 +722,70 @@ static void test_issue_2696(skiatest::Reporter* reporter) {
     }
 }
 
+void test_read_rrect(skiatest::Reporter* reporter, const SkRRect& rrect, bool shouldSucceed) {
+    // It would be cleaner to call rrect.writeToMemory into a buffer. However, writeToMemory asserts
+    // that the rrect is valid and our caller may have fiddled with the internals of rrect to make
+    // it invalid.
+    const void* buffer = reinterpret_cast<const void*>(&rrect);
+    SkRRect deserialized;
+    size_t size = deserialized.readFromMemory(buffer, sizeof(SkRRect));
+    if (shouldSucceed) {
+        REPORTER_ASSERT(reporter, size == SkRRect::kSizeInMemory);
+        if (size) {
+           REPORTER_ASSERT(reporter, rrect == deserialized);
+           REPORTER_ASSERT(reporter, rrect.getType() == deserialized.getType());
+        }
+    } else {
+        REPORTER_ASSERT(reporter, !size);
+    }
+}
+
+static void test_read(skiatest::Reporter* reporter) {
+    static const SkRect kRect = {10.f, 10.f, 20.f, 20.f};
+    static const SkRect kNaNRect = {10.f, 10.f, 20.f, SK_ScalarNaN};
+    static const SkRect kInfRect = {10.f, 10.f, SK_ScalarInfinity, 20.f};
+    SkRRect rrect;
+
+    test_read_rrect(reporter, SkRRect::MakeEmpty(), true);
+    test_read_rrect(reporter, SkRRect::MakeRect(kRect), true);
+    // These get coerced to empty.
+    test_read_rrect(reporter, SkRRect::MakeRect(kInfRect), true);
+    test_read_rrect(reporter, SkRRect::MakeRect(kNaNRect), true);
+
+    rrect.setRect(kRect);
+    SkRect* innerRect = reinterpret_cast<SkRect*>(&rrect);
+    SkASSERT(*innerRect == kRect);
+    *innerRect = kInfRect;
+    test_read_rrect(reporter, rrect, false);
+    *innerRect = kNaNRect;
+    test_read_rrect(reporter, rrect, false);
+
+    test_read_rrect(reporter, SkRRect::MakeOval(kRect), true);
+    test_read_rrect(reporter, SkRRect::MakeOval(kInfRect), true);
+    test_read_rrect(reporter, SkRRect::MakeOval(kNaNRect), true);
+    rrect.setOval(kRect);
+    *innerRect = kInfRect;
+    test_read_rrect(reporter, rrect, false);
+    *innerRect = kNaNRect;
+    test_read_rrect(reporter, rrect, false);
+
+    test_read_rrect(reporter, SkRRect::MakeRectXY(kRect, 5.f, 5.f), true);
+    // rrect should scale down the radii to make this legal
+    test_read_rrect(reporter, SkRRect::MakeRectXY(kRect, 5.f, 400.f), true);
+
+    static const SkVector kRadii[4] = {{0.5f, 1.f}, {1.5f, 2.f}, {2.5f, 3.f}, {3.5f, 4.f}};
+    rrect.setRectRadii(kRect, kRadii);
+    test_read_rrect(reporter, rrect, true);
+    SkScalar* innerRadius = reinterpret_cast<SkScalar*>(&rrect) + 6;
+    SkASSERT(*innerRadius == 1.5f);
+    *innerRadius = 400.f;
+    test_read_rrect(reporter, rrect, false);
+    *innerRadius = SK_ScalarInfinity;
+    test_read_rrect(reporter, rrect, false);
+    *innerRadius = SK_ScalarNaN;
+    test_read_rrect(reporter, rrect, false);
+}
+
 DEF_TEST(RoundRect, reporter) {
     test_round_rect_basic(reporter);
     test_round_rect_rects(reporter);
@@ -735,4 +799,5 @@ DEF_TEST(RoundRect, reporter) {
     test_tricky_radii(reporter);
     test_empty_crbug_458524(reporter);
     test_empty(reporter);
+    test_read(reporter);
 }