fix security bug

This fix is a tradeoff. It changes intersection to
treat a case where one coincident run is intersected at one point
and the other edge is not as continuing to be a span.

The old code tried to treat this as a single point.
The old code is probably right, but this change alone
made the data structures inconsistent. Later, extending
the coincident runs would fail by incorrectly discarding
the single point intersection.

As a result, this fixes the security test and one other, but
makes a different test fail. Isolating the failure uncovered
a reduced case that fails with and without the change, so
there are more serious problems here. Those problems are
addressed in a separate CL.

Many of the test edits below remove ill-thought out debugging
messaging that fire off global state, which isn't usable
in a multi-threaded test environment.

In the end, with this fix, all existing tests (modulo one
new failure and one new non-failure) pass in debug and
in the extended release test suites.

TBR=reed@google.com
BUG=614248
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2018513003

Review-Url: https://codereview.chromium.org/2018513003

9 files changed, 65 insertions, 22 deletions

diff --git a/tests/PathOpsExtendedTest.cpp b/tests/PathOpsExtendedTest.cpp
index c96cbcdae5..b5bf5b8d9f 100644
--- a/tests/PathOpsExtendedTest.cpp
+++ b/tests/PathOpsExtendedTest.cpp
@@ -299,8 +299,6 @@ int comparePaths(skiatest::Reporter* reporter, const char* filename, const SkPat
     return errors2x2 > MAX_ERRORS ? errors2x2 : 0;
 }
 
-const int gTestFirst = 41;
-static int gTestNo = gTestFirst;
 static SkTDArray<SkPathOp> gTestOp;
 
 static void showPathOpPath(const char* testName, const SkPath& one, const SkPath& two,
@@ -310,10 +308,9 @@ static void showPathOpPath(const char* testName, const SkPath& one, const SkPath
     if (!testName) {
         testName = "xOp";
     }
-    SkDebugf("static void %s%d%s(skiatest::Reporter* reporter, const char* filename) {\n",
-        testName, gTestNo, opSuffixes[shapeOp]);
+    SkDebugf("static void %s_%s(skiatest::Reporter* reporter, const char* filename) {\n",
+        testName, opSuffixes[shapeOp]);
     *gTestOp.append() = shapeOp;
-    ++gTestNo;
     SkDebugf("    SkPath path, pathB;\n");
     SkPathOpsDebug::ShowOnePath(a, "path", false);
     SkPathOpsDebug::ShowOnePath(b, "pathB", false);
@@ -322,15 +319,6 @@ static void showPathOpPath(const char* testName, const SkPath& one, const SkPath
     drawAsciiPaths(scaledOne, scaledTwo, true);
 }
 
-void ShowTestArray(const char* testName) {
-    if (!testName) {
-        testName = "xOp";
-    }
-    for (int x = gTestFirst; x < gTestNo; ++x) {
-        SkDebugf("    TEST(%s%d%s),\n", testName, x, opSuffixes[gTestOp[x - gTestFirst]]);
-    }
-}
-
 SK_DECLARE_STATIC_MUTEX(compareDebugOut3);
 
 static int comparePaths(skiatest::Reporter* reporter, const char* testName, const SkPath& one,
diff --git a/tests/PathOpsExtendedTest.h b/tests/PathOpsExtendedTest.h
index f8d74098ce..17073833cb 100644
--- a/tests/PathOpsExtendedTest.h
+++ b/tests/PathOpsExtendedTest.h
@@ -58,7 +58,6 @@ void RunTestSet(skiatest::Reporter* reporter, TestDesc tests[], size_t count,
                 void (*firstTest)(skiatest::Reporter* , const char* filename),
                 void (*skipTest)(skiatest::Reporter* , const char* filename),
                 void (*stopTest)(skiatest::Reporter* , const char* filename), bool reverse);
-void ShowTestArray(const char* testName);
 void ShowTestName(PathOpsThreadState* data, int a, int b, int c, int d);
 void ShowFunctionHeader(const char* name);
 void ShowPath(const SkPath& path, const char* pathName);
diff --git a/tests/PathOpsFuzz763Test.cpp b/tests/PathOpsFuzz763Test.cpp
index 55525734fc..7bc88ab5a5 100755
--- a/tests/PathOpsFuzz763Test.cpp
+++ b/tests/PathOpsFuzz763Test.cpp
@@ -254,7 +254,7 @@ static void fuzz763_378c(skiatest::Reporter* reporter, const char* filename) {
     path.quadTo(-39.8065f, 18.9507f, -43.0072f, 19.8086f);
     path.close();
     SkPath path2(path);
-    testPathOpCheck(reporter, path1, path2, (SkPathOp) 2, filename, FLAGS_runFail);
+    testPathOp(reporter, path1, path2, (SkPathOp) 2, filename);
 }
 
 static void fuzz763_378d(skiatest::Reporter* reporter, const char* filename) {
@@ -932,7 +932,8 @@ path.quadTo(SkBits2Float(0x42240000), SkBits2Float(0x41ed7d86), SkBits2Float(0x4
 path.close();
 
     SkPath path2(path);
-    testPathOp(reporter, path1, path2, (SkPathOp) 2, filename);
+    // FIXME: This should not fail; trading adding this failure for fixing security bug
+    testPathOpCheck(reporter, path1, path2, (SkPathOp) 2, filename, FLAGS_runFail);
 }
 
 static void fuzz763_24588(skiatest::Reporter* reporter, const char* filename) {
@@ -2416,10 +2417,10 @@ static struct TestDesc tests[] = {
     TEST(fuzz763_35322),
     TEST(fuzz763_8712),
     TEST(fuzz763_8712a),
+    TEST(fuzz763_4713),
     TEST(fuzz763_4014),
     TEST(fuzz763_4014a),
     TEST(fuzz763_1404),
-    TEST(fuzz763_4713),
     TEST(fuzz763_378),
     TEST(fuzz763_378b),
     TEST(fuzz763_378d),
diff --git a/tests/PathOpsOpCircleThreadedTest.cpp b/tests/PathOpsOpCircleThreadedTest.cpp
index 30b1603589..fac767720b 100644
--- a/tests/PathOpsOpCircleThreadedTest.cpp
+++ b/tests/PathOpsOpCircleThreadedTest.cpp
@@ -79,5 +79,4 @@ DEF_TEST(PathOpsOpCircleThreaded, reporter) {
     }
 finish:
     testRunner.render();
-    ShowTestArray("circleOp");
 }
diff --git a/tests/PathOpsOpCubicThreadedTest.cpp b/tests/PathOpsOpCubicThreadedTest.cpp
index 991d4e764c..2c0c3d372d 100644
--- a/tests/PathOpsOpCubicThreadedTest.cpp
+++ b/tests/PathOpsOpCubicThreadedTest.cpp
@@ -96,5 +96,4 @@ DEF_TEST(PathOpsOpCubicsThreaded, reporter) {
     }
 finish:
     testRunner.render();
-    ShowTestArray("cubicOp");
 }
diff --git a/tests/PathOpsOpLoopThreadedTest.cpp b/tests/PathOpsOpLoopThreadedTest.cpp
index 7b4b7ce7cf..56c5068d7a 100644
--- a/tests/PathOpsOpLoopThreadedTest.cpp
+++ b/tests/PathOpsOpLoopThreadedTest.cpp
@@ -108,5 +108,4 @@ DEF_TEST(PathOpsOpLoopsThreaded, reporter) {
     }
 finish:
     testRunner.render();
-    ShowTestArray("loopOp");
 }
diff --git a/tests/PathOpsOpTest.cpp b/tests/PathOpsOpTest.cpp
index 2af941498d..00a8f52477 100644
--- a/tests/PathOpsOpTest.cpp
+++ b/tests/PathOpsOpTest.cpp
@@ -5191,6 +5191,26 @@ path.close();
     testPathOp(reporter, path1, path2, (SkPathOp) 2, filename);
 }
 
+static void fuzzX_392(skiatest::Reporter* reporter, const char* filename) {
+    SkPath path;
+    path.setFillType(SkPath::kEvenOdd_FillType);
+path.moveTo(SkBits2Float(0x41e80000), SkBits2Float(0x43bde212));  // 29, 379.766f
+path.lineTo(SkBits2Float(0x41e80000), SkBits2Float(0x43bdc7ef));  // 29, 379.562f
+path.conicTo(SkBits2Float(0x42a5861e), SkBits2Float(0x43c61f86), SkBits2Float(0x430b0610), SkBits2Float(0x43c61f86), SkBits2Float(0x3f7d23f3));  // 82.7619f, 396.246f, 139.024f, 396.246f, 0.98883f
+path.conicTo(SkBits2Float(0x42a58e20), SkBits2Float(0x43c61f86), SkBits2Float(0x41e80000), SkBits2Float(0x43bde212), SkBits2Float(0x3f7d2cf5));  // 82.7776f, 396.246f, 29, 379.766f, 0.988967f
+path.close();
+
+    SkPath path1(path);
+    path.setFillType(SkPath::kWinding_FillType);
+path.moveTo(SkBits2Float(0xc36c7bd8), SkBits2Float(0xc3a31d72));  // -236.484f, -326.23f
+path.lineTo(SkBits2Float(0xc367a4ae), SkBits2Float(0xc3a31d72));  // -231.643f, -326.23f
+path.lineTo(SkBits2Float(0x430b0610), SkBits2Float(0x43c61f86));  // 139.024f, 396.246f
+path.lineTo(SkBits2Float(0xc36c7bd8), SkBits2Float(0x43c61f86));  // -236.484f, 396.246f
+
+    SkPath path2(path);
+    testPathOp(reporter, path1, path2, kIntersect_SkPathOp, filename);
+}
+
 static void (*skipTest)(skiatest::Reporter* , const char* filename) = 0;
 static void (*firstTest)(skiatest::Reporter* , const char* filename) = 0;
 static void (*stopTest)(skiatest::Reporter* , const char* filename) = 0;
@@ -5198,6 +5218,7 @@ static void (*stopTest)(skiatest::Reporter* , const char* filename) = 0;
 #define TEST(name) { name, #name }
 
 static struct TestDesc tests[] = {
+    TEST(fuzzX_392),
     TEST(crbug_526025),
     TEST(fuzz38),
     TEST(cubics44d),
diff --git a/tests/PathOpsSimplifyQuadThreadedTest.cpp b/tests/PathOpsSimplifyQuadThreadedTest.cpp
index 8ab84d7d8f..9e9c0103d5 100644
--- a/tests/PathOpsSimplifyQuadThreadedTest.cpp
+++ b/tests/PathOpsSimplifyQuadThreadedTest.cpp
@@ -95,5 +95,4 @@ DEF_TEST(PathOpsSimplifyQuadsThreaded, reporter) {
     }
 finish:
     testRunner.render();
-    ShowTestArray("testQuads");
 }
diff --git a/tests/PathOpsSimplifyTest.cpp b/tests/PathOpsSimplifyTest.cpp
index 70835d7e8a..a3f415ff89 100644
--- a/tests/PathOpsSimplifyTest.cpp
+++ b/tests/PathOpsSimplifyTest.cpp
@@ -5067,11 +5067,49 @@ path.close();
     REPORTER_ASSERT(reporter, !Simplify(path, &path));
 }
 
+// FIXME: this should not fail -- it was isolated looking for the root cause to fuzz763_4713
+// it fails with and without that change
+static void fuzz763_4713_b(skiatest::Reporter* reporter, const char* filename) {
+    SkPath path;
+    path.setFillType((SkPath::FillType) 0);
+path.moveTo(SkBits2Float(0x42240000), SkBits2Float(0x42040000));
+path.quadTo(SkBits2Float(0x42240000), SkBits2Float(0x4211413d), SkBits2Float(0x421aa09e), SkBits2Float(0x421aa09e));
+path.quadTo(SkBits2Float(0x4211413d), SkBits2Float(0x42240000), SkBits2Float(0x42040000), SkBits2Float(0x42240000));
+path.quadTo(SkBits2Float(0x41ed7d86), SkBits2Float(0x42240000), SkBits2Float(0x41dabec3), SkBits2Float(0x421aa09e));
+path.quadTo(SkBits2Float(0x41c80000), SkBits2Float(0x4211413d), SkBits2Float(0x41c80000), SkBits2Float(0x42040000));
+path.quadTo(SkBits2Float(0x41c80000), SkBits2Float(0x41ed7d86), SkBits2Float(0x41dabec3), SkBits2Float(0x41dabec3));
+path.quadTo(SkBits2Float(0x41ed7d86), SkBits2Float(0x41c80000), SkBits2Float(0x42040000), SkBits2Float(0x41c80000));
+path.quadTo(SkBits2Float(0x4211413d), SkBits2Float(0x41c80000), SkBits2Float(0x421aa09e), SkBits2Float(0x41dabec3));
+path.quadTo(SkBits2Float(0x42240000), SkBits2Float(0x41ed7d86), SkBits2Float(0x42240000), SkBits2Float(0x42040000));
+path.close();
+
+path.moveTo(SkBits2Float(0x4204f72e), SkBits2Float(0x41c56cd2));
+path.quadTo(SkBits2Float(0x42123842), SkBits2Float(0x41c52adf), SkBits2Float(0x421baed7), SkBits2Float(0x41d7bac6));
+path.quadTo(SkBits2Float(0x4225256d), SkBits2Float(0x41ea4aad), SkBits2Float(0x42254667), SkBits2Float(0x4202666b));
+path.quadTo(SkBits2Float(0x42256760), SkBits2Float(0x420fa77f), SkBits2Float(0x421c1f6c), SkBits2Float(0x42191e14));
+path.quadTo(SkBits2Float(0x421bff97), SkBits2Float(0x42193e89), SkBits2Float(0x421bdf6b), SkBits2Float(0x42195eb8));
+path.quadTo(SkBits2Float(0x421bbff6), SkBits2Float(0x42197f32), SkBits2Float(0x421ba03b), SkBits2Float(0x42199f57));
+path.quadTo(SkBits2Float(0x421b605e), SkBits2Float(0x4219e00a), SkBits2Float(0x421b1fa8), SkBits2Float(0x421a1f22));
+path.quadTo(SkBits2Float(0x421ae0f1), SkBits2Float(0x421a604b), SkBits2Float(0x421aa09e), SkBits2Float(0x421aa09e));
+path.quadTo(SkBits2Float(0x4211413d), SkBits2Float(0x42240000), SkBits2Float(0x42040000), SkBits2Float(0x42240000));
+path.quadTo(SkBits2Float(0x41ed7d86), SkBits2Float(0x42240000), SkBits2Float(0x41dabec3), SkBits2Float(0x421aa09e));
+path.quadTo(SkBits2Float(0x41c80000), SkBits2Float(0x4211413d), SkBits2Float(0x41c80000), SkBits2Float(0x42040000));
+path.quadTo(SkBits2Float(0x41c80000), SkBits2Float(0x41ed7d86), SkBits2Float(0x41dabec3), SkBits2Float(0x41dabec3));
+path.quadTo(SkBits2Float(0x41db19b1), SkBits2Float(0x41da63d5), SkBits2Float(0x41db755b), SkBits2Float(0x41da0a9b));
+path.quadTo(SkBits2Float(0x41dbce01), SkBits2Float(0x41d9ae59), SkBits2Float(0x41dc285e), SkBits2Float(0x41d952ce));
+path.quadTo(SkBits2Float(0x41dc55b6), SkBits2Float(0x41d924df), SkBits2Float(0x41dc82cd), SkBits2Float(0x41d8f7cd));
+path.quadTo(SkBits2Float(0x41dcaf1e), SkBits2Float(0x41d8ca01), SkBits2Float(0x41dcdc4c), SkBits2Float(0x41d89bf0));
+path.quadTo(SkBits2Float(0x41ef6c33), SkBits2Float(0x41c5aec5), SkBits2Float(0x4204f72e), SkBits2Float(0x41c56cd2));
+path.close();
+testSimplifyCheck(reporter, path, filename, false);
+}
+
 static void (*skipTest)(skiatest::Reporter* , const char* filename) = 0;
 static void (*firstTest)(skiatest::Reporter* , const char* filename) = 0;
 static void (*stopTest)(skiatest::Reporter* , const char* filename) = 0;
 
 static TestDesc tests[] = {
+    TEST(fuzz763_4713_b),
     TEST(fuzz_59),
     TEST(fuzz_twister2),
     TEST(fuzz_twister),