From dbc59ba23b5cb70cdf77cfdb6cbe372223a17832 Mon Sep 17 00:00:00 2001 From: Cary Clark Date: Thu, 19 Apr 2018 07:37:29 -0400 Subject: path is rect track corners This was triggered by an exploit that started the first edge well outside the final rectangle, causing the captured to exceed the correct result. Ivan observes that we really only want the first and third corners to compute the bounds, so remove the tracking code that looks for a valid range of points, and record the corners instead. R=robertphillips@google.com Bug: 824145,skia:7792 Change-Id: If228573d0f05c7158dba8142c144d13834e691ec Reviewed-on: https://skia-review.googlesource.com/122081 Commit-Queue: Cary Clark Commit-Queue: Robert Phillips Reviewed-by: Robert Phillips Auto-Submit: Cary Clark --- gm/pathfill.cpp | 22 ++++++++++++++ src/core/SkPath.cpp | 88 +++++++++++++++++++++-------------------------------- tests/PathTest.cpp | 27 +++++++++++++--- 3 files changed, 80 insertions(+), 57 deletions(-) diff --git a/gm/pathfill.cpp b/gm/pathfill.cpp index 23fd5302ae..2f44503c4e 100644 --- a/gm/pathfill.cpp +++ b/gm/pathfill.cpp @@ -588,4 +588,26 @@ DEF_SIMPLE_GM(bug7792, canvas, 800, 800) { path.lineTo(150, 75); path.close(); canvas->drawPath(path, p); + // from skbug.com/7792#c41 + canvas->translate(0, 200); + path.reset(); + path.moveTo(75, 75); + path.lineTo(150, 75); + path.lineTo(150, 150); + path.lineTo(140, 150); + path.lineTo(140, 75); + path.moveTo(75, 75); + path.close(); + canvas->drawPath(path, p); + // from skbug.com/7792#c53 + canvas->translate(0, 200); + path.reset(); + path.moveTo(75, 75); + path.lineTo(150, 75); + path.lineTo(150, 150); + path.lineTo(140, 150); + path.lineTo(140, 75); + path.moveTo(75, 75); + path.close(); + canvas->drawPath(path, p); } diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp index 0bfeae1d91..0683c174f1 100644 --- a/src/core/SkPath.cpp +++ b/src/core/SkPath.cpp @@ -454,16 +454,15 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts SkPoint lineStart; // used to construct line from previous point const SkPoint* firstPt = nullptr; // first point in the rect (last of first moves) const SkPoint* lastPt = nullptr; // last point in the rect (last of lines or first if closed) - const SkPoint* lastCountedPt = nullptr; // point creating 3rd corner + SkPoint firstCorner; + SkPoint thirdCorner; const SkPoint* pts = *ptsPtr; const SkPoint* savePts = nullptr; // used to allow caller to iterate through a pair of rects lineStart.set(0, 0); - signed char directions[] = {-1, -1, -1, -1}; // -1 to 3; -1 is uninitialized + signed char directions[] = {-1, -1, -1, -1, -1}; // -1 to 3; -1 is uninitialized bool closedOrMoved = false; - bool addedLine = false; bool autoClose = false; bool insertClose = false; - bool accumulatingRect = false; int verbCnt = fPathRef->countVerbs(); while (*currVerb < verbCnt && (!allowPartial || !autoClose)) { uint8_t verb = insertClose ? (uint8_t) kClose_Verb : fPathRef->atVerb(*currVerb); @@ -472,11 +471,7 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts savePts = pts; autoClose = true; insertClose = false; - accumulatingRect = false; case kLine_Verb: { - if (accumulatingRect) { - lastCountedPt = pts; - } if (kClose_Verb != verb) { lastPt = pts; } @@ -488,13 +483,12 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts if (lineStart == lineEnd) { break; // single point on side OK } - addedLine = true; int nextDirection = rect_make_dir(lineDelta.fX, lineDelta.fY); // 0 to 3 if (0 == corners) { directions[0] = nextDirection; - lineStart = lineEnd; corners = 1; closedOrMoved = false; + lineStart = lineEnd; break; } if (closedOrMoved) { @@ -504,28 +498,34 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts break; // colinear with first } closedOrMoved = autoClose; - lineStart = lineEnd; if (directions[corners - 1] == nextDirection) { if (3 == corners && kLine_Verb == verb) { - lastCountedPt = lastPt; + thirdCorner = lineEnd; } + lineStart = lineEnd; break; // colinear segment } - if (corners >= 4) { - return false; // too many direction changes - } directions[corners++] = nextDirection; // opposite lines must point in opposite directions; xoring them should equal 2 - if (3 == corners) { - if ((directions[0] ^ directions[2]) != 2) { - return false; - } - accumulatingRect = false; - } else if (4 == corners) { - if ((directions[1] ^ directions[3]) != 2) { - return false; - } + switch (corners) { + case 2: + firstCorner = lineStart; + break; + case 3: + if ((directions[0] ^ directions[2]) != 2) { + return false; + } + thirdCorner = lineEnd; + break; + case 4: + if ((directions[1] ^ directions[3]) != 2) { + return false; + } + break; + default: + return false; // too many direction changes } + lineStart = lineEnd; break; } case kQuad_Verb: @@ -538,15 +538,13 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts *currVerb -= 1; // try move again afterwards goto addMissingClose; } - if (!addedLine) { + if (!corners) { firstPt = pts; - accumulatingRect = true; } else { closeXY = *firstPt - *lastPt; if (closeXY.fX && closeXY.fY) { return false; // we're diagonal, abort } - accumulatingRect = false; } lineStart = *pts++; closedOrMoved = true; @@ -563,40 +561,24 @@ addMissingClose: if (corners < 3 || corners > 4) { return false; } - closeXY = *firstPt - *lastPt; - // If autoClose, check if close generates diagonal - bool result = 4 == corners && (closeXY.isZero() || (autoClose && (!closeXY.fX || !closeXY.fY))); - if (!result) { - if (closeXY.fX && closeXY.fY) { - return false; // we're diagonal, abort - } - // check if we are just an incomplete rectangle, in which case we can - // return true, but not claim to be closed. - // e.g. - // 3 sided rectangle - // 4 sided but the last edge is not long enough to reach the start - // - int closeDirection = rect_make_dir(closeXY.fX, closeXY.fY); - // make sure the close-segment doesn't double-back on itself - if (3 == corners || 2 == (closeDirection ^ directions[1])) { - result = true; - autoClose = false; // we are not closed - } - } if (savePts) { *ptsPtr = savePts; } - if (result && rect) { - ptrdiff_t count = lastCountedPt - firstPt + 1; - rect->set(firstPt, (int) count); + // check if close generates diagonal + closeXY = *firstPt - *lastPt; + if (closeXY.fX && closeXY.fY) { + return false; } - if (result && isClosed) { + if (rect) { + rect->set(firstCorner, thirdCorner); + } + if (isClosed) { *isClosed = autoClose; } - if (result && direction) { + if (direction) { *direction = directions[0] == ((directions[1] + 1) & 3) ? kCW_Direction : kCCW_Direction; } - return result; + return true; } bool SkPath::isRect(SkRect* rect, bool* isClosed, Direction* direction) const { diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp index b108b7f1bf..477b02b7c0 100644 --- a/tests/PathTest.cpp +++ b/tests/PathTest.cpp @@ -2033,7 +2033,7 @@ static void test_isRect(skiatest::Reporter* reporter) { { c3, SK_ARRAY_COUNT(c3), false, true }, { d1, SK_ARRAY_COUNT(d1), false, false }, - { d2, SK_ARRAY_COUNT(d2), false, false }, + { d2, SK_ARRAY_COUNT(d2), false, true }, { d3, SK_ARRAY_COUNT(d3), false, false }, }; @@ -2055,7 +2055,8 @@ static void test_isRect(skiatest::Reporter* reporter) { bool isClosed; SkPath::Direction direction; SkPathPriv::FirstDirection cheapDirection; - expected.set(tests[testIndex].fPoints, tests[testIndex].fPointCount); + int pointCount = tests[testIndex].fPointCount - (d2 == tests[testIndex].fPoints); + expected.set(tests[testIndex].fPoints, pointCount); REPORTER_ASSERT(reporter, SkPathPriv::CheapComputeFirstDirection(path, &cheapDirection)); REPORTER_ASSERT(reporter, path.isRect(&computed, &isClosed, &direction)); REPORTER_ASSERT(reporter, expected == computed); @@ -5020,11 +5021,11 @@ DEF_TEST(Path_isRect, reporter) { REPORTER_ASSERT(reporter, !path.isRect(&rect)); // isolated from skbug.com/7792#c39 SkPath::Verb verbs39[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, - SkPath::kLine_Verb }; + SkPath::kLine_Verb }; SkPoint points39[] = { {150, 75}, {150, 150}, {75, 150}, {75, 100} }; path = makePath2(points39, verbs39, SK_ARRAY_COUNT(verbs39)); REPORTER_ASSERT(reporter, !path.isRect(&rect)); - // from zero_length_paths_aa + // isolated from zero_length_paths_aa SkPath::Verb verbsAA[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kClose_Verb }; @@ -5034,4 +5035,22 @@ DEF_TEST(Path_isRect, reporter) { REPORTER_ASSERT(reporter, path.isRect(&rect)); compare.set(&pointsAA[0], SK_ARRAY_COUNT(pointsAA)); REPORTER_ASSERT(reporter, rect == compare); + // isolated from skbug.com/7792#c41 + SkPath::Verb verbs41[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, + SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kMove_Verb, + SkPath::kClose_Verb }; + SkPoint points41[] = { {75, 75}, {150, 75}, {150, 150}, {140, 150}, {140, 75}, {75, 75} }; + path = makePath2(points41, verbs41, SK_ARRAY_COUNT(verbs41)); + REPORTER_ASSERT(reporter, path.isRect(&rect)); + compare.set(&points41[1], 4); + REPORTER_ASSERT(reporter, rect == compare); + // isolated from skbug.com/7792#c53 + SkPath::Verb verbs53[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb, + SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kMove_Verb, + SkPath::kClose_Verb }; + SkPoint points53[] = { {75, 75}, {150, 75}, {150, 150}, {140, 150}, {140, 75}, {75, 75} }; + path = makePath2(points53, verbs53, SK_ARRAY_COUNT(verbs53)); + REPORTER_ASSERT(reporter, path.isRect(&rect)); + compare.set(&points53[1], 4); + REPORTER_ASSERT(reporter, rect == compare); } -- cgit v1.2.3