Improved SkPathRef interface security

https://codereview.chromium.org/115323004/

git-svn-id: http://skia.googlecode.com/svn/trunk@12676 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/include/core/SkPath.h b/include/core/SkPath.h
index 1526461..9b3f281 100644 (file)
--- a/include/core/SkPath.h
+++ b/include/core/SkPath.h
@@ -1005,7 +1005,9 @@ private:
 
     // 'rect' needs to be sorted
     void setBounds(const SkRect& rect) {
-        fPathRef->setBounds(rect);
+        SkPathRef::Editor ed(&fPathRef);
+
+        ed.setBounds(rect);
     }
 
 #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V16_AND_ALL_OTHER_INSTANCES_TOO

diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h
index 99fd90a..c55f549 100644 (file)
--- a/include/core/SkPathRef.h
+++ b/include/core/SkPathRef.h
@@ -49,7 +49,8 @@ public:
         /**
          * Returns the array of points.
          */
-        SkPoint* points() { return fPathRef->fPoints; }
+        SkPoint* points() { return fPathRef->getPoints(); }
+        const SkPoint* points() const { return fPathRef->points(); }
 
         /**
          * Gets the ith point. Shortcut for this->points() + i
@@ -58,6 +59,10 @@ public:
             SkASSERT((unsigned) i < (unsigned) fPathRef->fPointCnt);
             return this->points() + i;
         };
+        const SkPoint* atPoint(int i) const {
+            SkASSERT((unsigned) i < (unsigned) fPathRef->fPointCnt);
+            return this->points() + i;
+        };
 
         /**
          * Adds the verb and allocates space for the number of points indicated by the verb. The
@@ -89,6 +94,7 @@ public:
         void resetToSize(int newVerbCnt, int newPointCnt, int newConicCount) {
             fPathRef->resetToSize(newVerbCnt, newPointCnt, newConicCount);
         }
+
         /**
          * Gets the path ref that is wrapped in the Editor.
          */
@@ -96,6 +102,8 @@ public:
 
         void setIsOval(bool isOval) { fPathRef->setIsOval(isOval); }
 
+        void setBounds(const SkRect& rect) { fPathRef->setBounds(rect); }
+
     private:
         SkPathRef* fPathRef;
     };
@@ -158,13 +166,6 @@ public:
         return fBounds;
     }
 
-    void setBounds(const SkRect& rect) {
-        SkASSERT(rect.fLeft <= rect.fRight && rect.fTop <= rect.fBottom);
-        fBounds = rect;
-        fBoundsIsDirty = false;
-        fIsFinite = fBounds.isFinite();
-    }
-
     /**
      * Transforms a path ref by a matrix, allocating a new one only if necessary.
      */
@@ -299,6 +300,13 @@ private:
         fBoundsIsDirty = false;
     }
 
+    void setBounds(const SkRect& rect) {
+        SkASSERT(rect.fLeft <= rect.fRight && rect.fTop <= rect.fBottom);
+        fBounds = rect;
+        fBoundsIsDirty = false;
+        fIsFinite = fBounds.isFinite();
+    }
+
     /** Makes additional room but does not change the counts or change the genID */
     void incReserve(int additionalVerbs, int additionalPoints) {
         SkDEBUGCODE(this->validate();)
@@ -418,6 +426,12 @@ private:
 
     void setIsOval(bool isOval) { fIsOval = isOval; }
 
+    SkPoint* getPoints() { 
+        SkDEBUGCODE(this->validate();) 
+        fIsOval = false;
+        return fPoints; 
+    }
+
     enum {
         kMinSize = 256,
     };
@@ -441,6 +455,7 @@ private:
     mutable uint32_t    fGenerationID;
     SkDEBUGCODE(int32_t fEditorsAttached;) // assert that only one editor in use at any time.
 
+    friend class PathRefTest_Private;
     typedef SkRefCnt INHERITED;
 };
 

diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index a57e2f4..0711e3f 100644 (file)
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -24,7 +24,6 @@ SkPathRef::Editor::Editor(SkAutoTUnref<SkPathRef>* pathRef,
     }
     fPathRef = *pathRef;
     fPathRef->fGenerationID = 0;
-    fPathRef->fIsOval = false;
     SkDEBUGCODE(sk_atomic_inc(&fPathRef->fEditorsAttached);)
 }
 

diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 991b4fd..b33ef97 100644 (file)
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -2679,8 +2679,8 @@ static void test_circle_with_add_paths(skiatest::Reporter* reporter) {
     SkMatrix translate;
     translate.setTranslate(SkIntToScalar(12), SkIntToScalar(12));
 
-    // For simplicity, all the path concatenation related operations
-    // would mark it non-circle, though in theory it's still a circle.
+    // Although all the path concatenation related operations leave
+    // the path a circle, most mark it as a non-circle for simplicity
 
     // empty + circle (translate)
     path = empty;
@@ -2690,7 +2690,7 @@ static void test_circle_with_add_paths(skiatest::Reporter* reporter) {
     // circle + empty (translate)
     path = circle;
     path.addPath(empty, translate);
-    check_for_circle(reporter, path, false, kCircleDir);
+    check_for_circle(reporter, path, true, kCircleDir);
 
     // test reverseAddPath
     path = circle;
@@ -3138,77 +3138,74 @@ static void test_contains(skiatest::Reporter* reporter) {
     }
 }
 
-static void test_pathref(skiatest::Reporter* reporter) {
-    static const int kRepeatCnt = 10;
-
-    SkPathRef* pathRef = SkPathRef::CreateEmpty();
-    SkAutoTUnref<SkPathRef> pathRef2(SkPathRef::CreateEmpty());
-    SkMatrix mat;
-
-    mat.setTranslate(10, 10);
+class PathRefTest_Private {
+public:
+    static void TestPathRef(skiatest::Reporter* reporter) {
+        static const int kRepeatCnt = 10;
 
-    SkPathRef::CreateTransformedCopy(&pathRef2, *pathRef, mat);
+        SkAutoTUnref<SkPathRef> pathRef(SkNEW(SkPathRef));
 
-    SkPathRef::Editor ed(&pathRef2);
+        SkPathRef::Editor ed(&pathRef);
 
-    {
-        ed.growForRepeatedVerb(SkPath::kMove_Verb, kRepeatCnt);
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countVerbs());
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countPoints());
-        REPORTER_ASSERT(reporter, 0 == pathRef2->getSegmentMasks());
-        for (int i = 0; i < kRepeatCnt; ++i) {
-            REPORTER_ASSERT(reporter, SkPath::kMove_Verb == pathRef2->atVerb(i));
+        {
+            ed.growForRepeatedVerb(SkPath::kMove_Verb, kRepeatCnt);
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countVerbs());
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countPoints());
+            REPORTER_ASSERT(reporter, 0 == pathRef->getSegmentMasks());
+            for (int i = 0; i < kRepeatCnt; ++i) {
+                REPORTER_ASSERT(reporter, SkPath::kMove_Verb == pathRef->atVerb(i));
+            }
+            ed.resetToSize(0, 0, 0);
         }
-        ed.resetToSize(0, 0, 0);
-    }
 
-    {
-        ed.growForRepeatedVerb(SkPath::kLine_Verb, kRepeatCnt);
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countVerbs());
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countPoints());
-        REPORTER_ASSERT(reporter, SkPath::kLine_SegmentMask == pathRef2->getSegmentMasks());
-        for (int i = 0; i < kRepeatCnt; ++i) {
-            REPORTER_ASSERT(reporter, SkPath::kLine_Verb == pathRef2->atVerb(i));
+        {
+            ed.growForRepeatedVerb(SkPath::kLine_Verb, kRepeatCnt);
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countVerbs());
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countPoints());
+            REPORTER_ASSERT(reporter, SkPath::kLine_SegmentMask == pathRef->getSegmentMasks());
+            for (int i = 0; i < kRepeatCnt; ++i) {
+                REPORTER_ASSERT(reporter, SkPath::kLine_Verb == pathRef->atVerb(i));
+            }
+            ed.resetToSize(0, 0, 0);
         }
-        ed.resetToSize(0, 0, 0);
-    }
 
-    {
-        ed.growForRepeatedVerb(SkPath::kQuad_Verb, kRepeatCnt);
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countVerbs());
-        REPORTER_ASSERT(reporter, 2*kRepeatCnt == pathRef2->countPoints());
-        REPORTER_ASSERT(reporter, SkPath::kQuad_SegmentMask == pathRef2->getSegmentMasks());
-        for (int i = 0; i < kRepeatCnt; ++i) {
-            REPORTER_ASSERT(reporter, SkPath::kQuad_Verb == pathRef2->atVerb(i));
+        {
+            ed.growForRepeatedVerb(SkPath::kQuad_Verb, kRepeatCnt);
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countVerbs());
+            REPORTER_ASSERT(reporter, 2*kRepeatCnt == pathRef->countPoints());
+            REPORTER_ASSERT(reporter, SkPath::kQuad_SegmentMask == pathRef->getSegmentMasks());
+            for (int i = 0; i < kRepeatCnt; ++i) {
+                REPORTER_ASSERT(reporter, SkPath::kQuad_Verb == pathRef->atVerb(i));
+            }
+            ed.resetToSize(0, 0, 0);
         }
-        ed.resetToSize(0, 0, 0);
-    }
 
-    {
-        SkScalar* weights = NULL;
-        ed.growForRepeatedVerb(SkPath::kConic_Verb, kRepeatCnt, &weights);
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countVerbs());
-        REPORTER_ASSERT(reporter, 2*kRepeatCnt == pathRef2->countPoints());
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countWeights());
-        REPORTER_ASSERT(reporter, SkPath::kConic_SegmentMask == pathRef2->getSegmentMasks());
-        REPORTER_ASSERT(reporter, NULL != weights);
-        for (int i = 0; i < kRepeatCnt; ++i) {
-            REPORTER_ASSERT(reporter, SkPath::kConic_Verb == pathRef2->atVerb(i));
+        {
+            SkScalar* weights = NULL;
+            ed.growForRepeatedVerb(SkPath::kConic_Verb, kRepeatCnt, &weights);
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countVerbs());
+            REPORTER_ASSERT(reporter, 2*kRepeatCnt == pathRef->countPoints());
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countWeights());
+            REPORTER_ASSERT(reporter, SkPath::kConic_SegmentMask == pathRef->getSegmentMasks());
+            REPORTER_ASSERT(reporter, NULL != weights);
+            for (int i = 0; i < kRepeatCnt; ++i) {
+                REPORTER_ASSERT(reporter, SkPath::kConic_Verb == pathRef->atVerb(i));
+            }
+            ed.resetToSize(0, 0, 0);
         }
-        ed.resetToSize(0, 0, 0);
-    }
 
-    {
-        ed.growForRepeatedVerb(SkPath::kCubic_Verb, kRepeatCnt);
-        REPORTER_ASSERT(reporter, kRepeatCnt == pathRef2->countVerbs());
-        REPORTER_ASSERT(reporter, 3*kRepeatCnt == pathRef2->countPoints());
-        REPORTER_ASSERT(reporter, SkPath::kCubic_SegmentMask == pathRef2->getSegmentMasks());
-        for (int i = 0; i < kRepeatCnt; ++i) {
-            REPORTER_ASSERT(reporter, SkPath::kCubic_Verb == pathRef2->atVerb(i));
+        {
+            ed.growForRepeatedVerb(SkPath::kCubic_Verb, kRepeatCnt);
+            REPORTER_ASSERT(reporter, kRepeatCnt == pathRef->countVerbs());
+            REPORTER_ASSERT(reporter, 3*kRepeatCnt == pathRef->countPoints());
+            REPORTER_ASSERT(reporter, SkPath::kCubic_SegmentMask == pathRef->getSegmentMasks());
+            for (int i = 0; i < kRepeatCnt; ++i) {
+                REPORTER_ASSERT(reporter, SkPath::kCubic_Verb == pathRef->atVerb(i));
+            }
+            ed.resetToSize(0, 0, 0);
         }
-        ed.resetToSize(0, 0, 0);
     }
-}
+};
 
 static void test_operatorEqual(skiatest::Reporter* reporter) {
     SkPath a;
@@ -3369,6 +3366,6 @@ DEF_TEST(Path, reporter) {
     test_conicTo_special_case(reporter);
     test_get_point(reporter);
     test_contains(reporter);
-    test_pathref(reporter);
     PathTest_Private::TestPathTo(reporter);
+    PathRefTest_Private::TestPathRef(reporter);
 }