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>

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)

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;
     }
 
-    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)) {
-            return false;
-        }
-    }
-
     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(radii[i].fX, rect.fLeft, rect.fRight) ||
+            !are_radius_check_predicates_valid(radii[i].fY, rect.fTop, rect.fBottom)) {
+            return false;
+        }
+    }
+    return true;
+}
 ///////////////////////////////////////////////////////////////////////////////

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);
 }