commit c3d8a48f1b27370049aa512019cd726c59354743
author Mike Reed <reed@google.com> Wed Sep 12 10:08:40 2018 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Wed Sep 12 14:29:18 2018 +0000
tree 26b30c9b34bce88c2afb185daf43cb2ebbc82827
parent b2acf0a93927a57cc1fa9323839e788379ae3366

allow path.add(path) safely

Bug: 882423
Change-Id: Ied2ad2d5dfdf00af8f3ba722b522e9602fea5557
Reviewed-on: https://skia-review.googlesource.com/153260
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>

src/core/SkPath.cpp

diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index b51bb2d..09d0a95 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -19,6 +19,7 @@
 #include "SkPointPriv.h"
 #include "SkRRect.h"
 #include "SkSafeMath.h"
+#include "SkTLazy.h"
 #include "SkTo.h"
 
 #include <cmath>
@@ -1577,10 +1578,17 @@
     return this->addPath(path, matrix, mode);
 }
 
-SkPath& SkPath::addPath(const SkPath& path, const SkMatrix& matrix, AddPathMode mode) {
-    SkPathRef::Editor(&fPathRef, path.countVerbs(), path.countPoints());
+SkPath& SkPath::addPath(const SkPath& srcPath, const SkMatrix& matrix, AddPathMode mode) {
+    // Detect if we're trying to add ourself
+    const SkPath* src = &srcPath;
+    SkTLazy<SkPath> tmp;
+    if (this == src) {
+        src = tmp.set(srcPath);
+    }
 
-    RawIter iter(path);
+    SkPathRef::Editor(&fPathRef, src->countVerbs(), src->countPoints());
+
+    RawIter iter(*src);
     SkPoint pts[4];
     Verb    verb;
 

tests/PathTest.cpp

diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 349a309f..cbb2b48 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -5069,6 +5069,29 @@
     REPORTER_ASSERT(reporter, rect == compare);
 }
 
+// Be sure we can safely add ourselves
+DEF_TEST(Path_self_add, reporter) {
+    // The possible problem is that during path.add() we may have to grow the dst buffers as
+    // we append the src pts/verbs, but all the while we are iterating over the src. If src == dst
+    // we could realloc the buffer's (on behalf of dst) leaving the src iterator pointing at
+    // garbage.
+    //
+    // The test runs though verious sized src paths, since its not defined publicly what the
+    // reserve allocation strategy is for SkPath, therefore we can't know when an append operation
+    // will trigger a realloc. At the time of this writing, these loops were sufficient to trigger
+    // an ASAN error w/o the fix to SkPath::addPath().
+    //
+    for (int count = 0; count < 10; ++count) {
+        SkPath path;
+        for (int add = 0; add < count; ++add) {
+            // just add some stuff, so we have something to copy/append in addPath()
+            path.moveTo(1, 2).lineTo(3, 4).cubicTo(1,2,3,4,5,6).conicTo(1,2,3,4,5);
+        }
+        path.addPath(path, 1, 2);
+        path.addPath(path, 3, 4);
+    }
+}
+
 #include "SkVertices.h"
 static void draw_triangle(SkCanvas* canvas, const SkPoint pts[]) {
     // draw in different ways, looking for an assert