From 74b429086122e19e82fe090fc999120ed48d3532 Mon Sep 17 00:00:00 2001 From: Cary Clark Date: Fri, 9 Mar 2018 07:38:47 -0500 Subject: some fuzzer fixes should fix three or four of the PathOp asserts triggered by the fuzzer tool. R=kjlubick@google.com Bug: skia: Change-Id: I470895addf1e922da6a7c41d44d54eca92e68fb6 Reviewed-on: https://skia-review.googlesource.com/113163 Reviewed-by: Kevin Lubick Commit-Queue: Cary Clark --- src/pathops/SkIntersections.cpp | 29 +++++++++++++++++++---------- src/pathops/SkOpCoincidence.cpp | 3 ++- src/pathops/SkOpEdgeBuilder.cpp | 4 +++- src/pathops/SkOpSpan.cpp | 9 +-------- src/pathops/SkOpSpan.h | 1 - src/pathops/SkPathOpsCubic.cpp | 3 +-- src/pathops/SkPathOpsOp.cpp | 3 +-- src/pathops/SkPathOpsTSect.h | 17 +++++++++++------ tests/PathOpsConicQuadIntersectionTest.cpp | 9 ++++++++- tests/PathOpsCubicConicIntersectionTest.cpp | 16 +++++++++++++++- tests/PathOpsSimplifyFailTest.cpp | 14 ++++++++++++++ 11 files changed, 75 insertions(+), 33 deletions(-) diff --git a/src/pathops/SkIntersections.cpp b/src/pathops/SkIntersections.cpp index f17e5dbc38..541cfb0ab6 100644 --- a/src/pathops/SkIntersections.cpp +++ b/src/pathops/SkIntersections.cpp @@ -45,23 +45,32 @@ int SkIntersections::insert(double one, double two, const SkDPoint& pt) { return -1; } if (more_roughly_equal(oldOne, one) && more_roughly_equal(oldTwo, two)) { - if ((precisely_zero(one) && !precisely_zero(oldOne)) - || (precisely_equal(one, 1) && !precisely_equal(oldOne, 1)) - || (precisely_zero(two) && !precisely_zero(oldTwo)) - || (precisely_equal(two, 1) && !precisely_equal(oldTwo, 1))) { - SkASSERT(one >= 0 && one <= 1); - SkASSERT(two >= 0 && two <= 1); - fT[0][index] = one; - fT[1][index] = two; - fPt[index] = pt; + if ((!precisely_zero(one) || precisely_zero(oldOne)) + && (!precisely_equal(one, 1) || precisely_equal(oldOne, 1)) + && (!precisely_zero(two) || precisely_zero(oldTwo)) + && (!precisely_equal(two, 1) || precisely_equal(oldTwo, 1))) { + return -1; } - return -1; + SkASSERT(one >= 0 && one <= 1); + SkASSERT(two >= 0 && two <= 1); + // remove this and reinsert below in case replacing would make list unsorted + int remaining = fUsed - index - 1; + memmove(&fPt[index], &fPt[index + 1], sizeof(fPt[0]) * remaining); + memmove(&fT[0][index], &fT[0][index + 1], sizeof(fT[0][0]) * remaining); + memmove(&fT[1][index], &fT[1][index + 1], sizeof(fT[1][0]) * remaining); + int clearMask = ~((1 << index) - 1); + fIsCoincident[0] -= (fIsCoincident[0] >> 1) & clearMask; + fIsCoincident[1] -= (fIsCoincident[1] >> 1) & clearMask; + --fUsed; + break; } #if ONE_OFF_DEBUG if (pt.roughlyEqual(fPt[index])) { SkDebugf("%s t=%1.9g pts roughly equal\n", __FUNCTION__, one); } #endif + } + for (index = 0; index < fUsed; ++index) { if (fT[0][index] > one) { break; } diff --git a/src/pathops/SkOpCoincidence.cpp b/src/pathops/SkOpCoincidence.cpp index 2f9d28f9ee..8e5c6c1d4d 100644 --- a/src/pathops/SkOpCoincidence.cpp +++ b/src/pathops/SkOpCoincidence.cpp @@ -694,7 +694,7 @@ bool SkOpCoincidence::addOrOverlap(SkOpSegment* coinSeg, SkOpSegment* oppSeg, SkASSERT(!cs || !cs->deleted()); SkASSERT(!os || !os->deleted()); SkASSERT(!ce || !ce->deleted()); - SkASSERT(!oe || !oe->deleted()); + FAIL_IF(oe && oe->deleted()); const SkOpPtT* csExisting = !cs ? coinSeg->existing(coinTs, nullptr) : nullptr; const SkOpPtT* ceExisting = !ce ? coinSeg->existing(coinTe, nullptr) : nullptr; FAIL_IF(csExisting && csExisting == ceExisting); @@ -723,6 +723,7 @@ bool SkOpCoincidence::addOrOverlap(SkOpSegment* coinSeg, SkOpSegment* oppSeg, csWritable->span()->addOpp(osWritable->span()); cs = csWritable; os = osWritable->active(); + FAIL_IF(!os); FAIL_IF((ce && ce->deleted()) || (oe && oe->deleted())); } if (!ce || !oe) { diff --git a/src/pathops/SkOpEdgeBuilder.cpp b/src/pathops/SkOpEdgeBuilder.cpp index cda6850701..16cbfc410c 100644 --- a/src/pathops/SkOpEdgeBuilder.cpp +++ b/src/pathops/SkOpEdgeBuilder.cpp @@ -323,7 +323,9 @@ bool SkOpEdgeBuilder::walk() { } SkPoint* curve = SkPath::kCubic_Verb == split->fVerb ? split->fPts : split->fReduced; - SkAssertResult(can_add_curve(split->fVerb, curve)); + if (!can_add_curve(split->fVerb, curve)) { + return false; + } fContourBuilder.addCurve(split->fVerb, curve); } } diff --git a/src/pathops/SkOpSpan.cpp b/src/pathops/SkOpSpan.cpp index 28989d9511..f5195edb38 100644 --- a/src/pathops/SkOpSpan.cpp +++ b/src/pathops/SkOpSpan.cpp @@ -24,8 +24,7 @@ const SkOpPtT* SkOpPtT::active() const { return ptT; } } - SkASSERT(0); // should never return deleted - return this; + return nullptr; // should never return deleted; caller must abort } bool SkOpPtT::contains(const SkOpPtT* check) const { @@ -269,12 +268,6 @@ tryNextRemainder: fSpanAdds += span->fSpanAdds; } -SkOpSpanBase* SkOpSpanBase::active() { - SkOpSpanBase* result = fPrev ? fPrev->next() : upCast()->next()->prev(); - SkASSERT(this == result || fDebugDeleted); - return result; -} - // please keep in sync with debugCheckForCollapsedCoincidence() void SkOpSpanBase::checkForCollapsedCoincidence() { SkOpCoincidence* coins = this->globalState()->coincidence(); diff --git a/src/pathops/SkOpSpan.h b/src/pathops/SkOpSpan.h index fea3309598..af1481e920 100644 --- a/src/pathops/SkOpSpan.h +++ b/src/pathops/SkOpSpan.h @@ -177,7 +177,6 @@ protected: class SkOpSpanBase { public: - SkOpSpanBase* active(); void addOpp(SkOpSpanBase* opp); void bumpSpanAdds() { diff --git a/src/pathops/SkPathOpsCubic.cpp b/src/pathops/SkPathOpsCubic.cpp index 33229fa8ae..eb2ddb7c7d 100644 --- a/src/pathops/SkPathOpsCubic.cpp +++ b/src/pathops/SkPathOpsCubic.cpp @@ -35,8 +35,7 @@ double SkDCubic::binarySearch(double min, double max, double axisIntercept, double calcPos = (&cubicAtT.fX)[xAxis]; double calcDist = calcPos - axisIntercept; do { - double priorT = t - step; - SkOPASSERT(priorT >= min); + double priorT = std::max(min, t - step); SkDPoint lessPt = ptAtT(priorT); if (approximately_equal_half(lessPt.fX, cubicAtT.fX) && approximately_equal_half(lessPt.fY, cubicAtT.fY)) { diff --git a/src/pathops/SkPathOpsOp.cpp b/src/pathops/SkPathOpsOp.cpp index 479b9a5c24..ee2b200f62 100644 --- a/src/pathops/SkPathOpsOp.cpp +++ b/src/pathops/SkPathOpsOp.cpp @@ -205,8 +205,7 @@ static const bool gOutInverse[kReverseDifference_SkPathOp + 1][2][2] = { SK_DECLARE_STATIC_MUTEX(debugWorstLoop); -SkOpGlobalState debugWorstState(nullptr, nullptr SkDEBUGPARAMS(false) SkDEBUGPARAMS(nullptr) - SkDEBUGPARAMS(nullptr)); +SkOpGlobalState debugWorstState(nullptr, nullptr SkDEBUGPARAMS(false) SkDEBUGPARAMS(nullptr)); void ReportPathOpsDebugging() { debugWorstState.debugLoopReport(); diff --git a/src/pathops/SkPathOpsTSect.h b/src/pathops/SkPathOpsTSect.h index fb00d9abc9..833e735da6 100644 --- a/src/pathops/SkPathOpsTSect.h +++ b/src/pathops/SkPathOpsTSect.h @@ -272,7 +272,7 @@ private: } bool binarySearchCoin(SkTSect* , double tStart, double tStep, double* t, - double* oppT); + double* oppT, SkTSpan** oppFirst); SkTSpan* boundsMax() const; bool coincidentCheck(SkTSect* sect2); void coincidentForce(SkTSect* sect2, double start1s, double start1e); @@ -908,7 +908,7 @@ SkTSpan* SkTSect::addOne() { template bool SkTSect::binarySearchCoin(SkTSect* sect2, double tStart, - double tStep, double* resultT, double* oppT) { + double tStep, double* resultT, double* oppT, SkTSpan** oppFirst) { SkTSpan work; double result = work.fStartT = work.fEndT = tStart; SkDEBUGCODE(work.fDebugSect = this); @@ -916,7 +916,7 @@ bool SkTSect::binarySearchCoin(SkTSect* sect SkDPoint oppPt; bool flip = false; bool contained = false; - SkDEBUGCODE(bool down = tStep < 0); + bool down = tStep < 0; const OppCurve& opp = sect2->fCurve; do { tStep *= 0.5; @@ -943,7 +943,10 @@ bool SkTSect::binarySearchCoin(SkTSect* sect *oppT = oppTTest; oppPt = work.fCoinStart.perpPt(); contained = true; - SkASSERT(down ? result > work.fStartT : result < work.fStartT); + if (down ? result <= work.fStartT : result >= work.fStartT) { + *oppFirst = nullptr; // signal caller to fail + return false; + } result = work.fStartT; continue; } @@ -1199,7 +1202,7 @@ bool SkTSect::extractCoincident( SkTSpan* cutFirst; if (prev && prev->fEndT == startT && this->binarySearchCoin(sect2, startT, prev->fStartT - startT, &coinStart, - &oppStartT) + &oppStartT, &oppFirst) && prev->fStartT < coinStart && coinStart < startT && (cutFirst = prev->oppT(oppStartT))) { oppFirst = cutFirst; @@ -1218,8 +1221,10 @@ bool SkTSect::extractCoincident( } } } else { + if (!oppFirst) { + return false; + } SkDEBUGCODE(coinStart = first->fStartT); - FAIL_IF(!oppFirst); SkDEBUGCODE(oppStartT = oppMatched ? oppFirst->fStartT : oppFirst->fEndT); } // FIXME: incomplete : if we're not at the end, find end of coin diff --git a/tests/PathOpsConicQuadIntersectionTest.cpp b/tests/PathOpsConicQuadIntersectionTest.cpp index 99411dc98c..bdf1d42d37 100644 --- a/tests/PathOpsConicQuadIntersectionTest.cpp +++ b/tests/PathOpsConicQuadIntersectionTest.cpp @@ -15,6 +15,13 @@ static struct conicQuad { ConicPts conic; QuadPts quad; } conicQuadTests[] = { + {{{{{0.00000000000000000, -1.8135968446731567 }, + {0.00000000000000000, -1.0033817291259766 }, + {-0.0073835160583257675, 0.00000000000000000 }}}, 2.26585215e+11f}, + {{{0.00000000000000000, -1.0000113248825073 }, + {-2.4824290449032560e-05, -1.0000115633010864 }, + {-0.0073835160583257675, 0.00000000000000000 }}}}, + {{{{{494.348663,224.583771}, {494.365143,224.633194}, {494.376404,224.684067}}}, 0.998645842f}, {{{494.30481,224.474213}, {494.334961,224.538284}, {494.355774,224.605927}}}}, @@ -69,5 +76,5 @@ DEF_TEST(PathOpsConicQuadIntersection, reporter) { } DEF_TEST(PathOpsConicQuadIntersectionOneOff, reporter) { - conicQuadIntersection(reporter, 1); + conicQuadIntersection(reporter, 0); } diff --git a/tests/PathOpsCubicConicIntersectionTest.cpp b/tests/PathOpsCubicConicIntersectionTest.cpp index d7924634b2..ff4dec9864 100644 --- a/tests/PathOpsCubicConicIntersectionTest.cpp +++ b/tests/PathOpsCubicConicIntersectionTest.cpp @@ -15,6 +15,20 @@ static struct cubicConic { CubicPts cubic; ConicPts conic; } cubicConicTests[] = { +#if 0 +// FIXME: this triggers an assert in bool SkTSect::extractCoincident() at +// SkOPASSERT(oppStartT < oppEndT); +// Throwing an error here breaks one test, but only in release. +// More work to be done to figure this out. + {{{{2.1883804947719909e-05, 3.6366123822517693e-05 }, + {2.9145950975362211e-05, 2.9117207304807380e-05 }, + {2.9113532946212217e-05, 2.9173743314458989e-05 }, + {0.00000000000000000, 5.8282588724978268e-05 }}}, + {{{{0.00000000000000000, 5.8282581449020654e-05 }, + {0.00000000000000000, 5.8282563259126619e-05 }, + {5.8282588724978268e-05, 0.00000000000000000 }}}, 53684.6563f}}, +#endif + {{{{188.60000610351562, 2041.5999755859375}, {188.60000610351562, 2065.39990234375}, {208, 2084.800048828125}, {231.80000305175781, 2084.800048828125}}}, {{{{231.80000305175781, 2084.800048828125}, {188.60000610351562, 2084.800048828125}, @@ -73,5 +87,5 @@ DEF_TEST(PathOpsCubicConicIntersection, reporter) { } DEF_TEST(PathOpsCubicConicIntersectionOneOff, reporter) { - cubicConicIntersection(reporter, 1); + cubicConicIntersection(reporter, 0); } diff --git a/tests/PathOpsSimplifyFailTest.cpp b/tests/PathOpsSimplifyFailTest.cpp index b6f210e8ea..58179a2175 100644 --- a/tests/PathOpsSimplifyFailTest.cpp +++ b/tests/PathOpsSimplifyFailTest.cpp @@ -215,9 +215,23 @@ path.close(); testSimplifyFuzz(reporter, path, filename); } +static void fuzz_x3(skiatest::Reporter* reporter, const char* filename) { + SkPath path; +path.setFillType(SkPath::kWinding_FillType); +path.moveTo(SkBits2Float(0x00000000), SkBits2Float(0x00000000)); // 0, 0 +path.cubicTo(SkBits2Float(0x92743420), SkBits2Float(0x74747474), SkBits2Float(0x0f747c74), SkBits2Float(0xff538565), SkBits2Float(0x74744374), SkBits2Float(0x20437474)); // -7.70571e-28f, 7.74708e+31f, 1.20541e-29f, -2.8116e+38f, 7.74102e+31f, 1.65557e-19f +path.conicTo(SkBits2Float(0x7474926d), SkBits2Float(0x7c747474), SkBits2Float(0x00170f74), SkBits2Float(0x3a7410d7), SkBits2Float(0x3a3a3a3a)); // 7.7508e+31f, 5.07713e+36f, 2.11776e-39f, 0.000931037f, 0.000710401f +path.quadTo(SkBits2Float(0x203a3a3a), SkBits2Float(0x7459f43a), SkBits2Float(0x74747474), SkBits2Float(0x2043ad6e)); // 1.57741e-19f, 6.90724e+31f, 7.74708e+31f, 1.65745e-19f +path.conicTo(SkBits2Float(0x7474b374), SkBits2Float(0x74747474), SkBits2Float(0x0f747c74), SkBits2Float(0xff537065), SkBits2Float(0x74744374)); // 7.75488e+31f, 7.74708e+31f, 1.20541e-29f, -2.81051e+38f, 7.74102e+31f +path.cubicTo(SkBits2Float(0x3a3a3a3a), SkBits2Float(0x3a2c103a), SkBits2Float(0x7474263a), SkBits2Float(0x74976507), SkBits2Float(0x000000ff), SkBits2Float(0x00000000)); // 0.000710401f, 0.00065637f, 7.7374e+31f, 9.59578e+31f, 3.57331e-43f, 0 + testSimplifyFuzz(reporter, path, filename); +} + + #define TEST(test) test(reporter, #test) DEF_TEST(PathOpsSimplifyFail, reporter) { + TEST(fuzz_x3); TEST(fuzz763_2); TEST(fuzz763_1); TEST(fuzz_x2); -- cgit v1.2.3