Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(193)

Issue 2018513003: fix security bug (Closed)

Created:
4 years, 7 months ago by caryclark
Modified:
4 years, 7 months ago
Reviewers:
reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

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 Committed: https://skia.googlesource.com/skia/+/2bec26a71698105729c6a7cb0163f499b4361840

Patch Set 1 #

Patch Set 2 : remove extra spaces #

Unified diffs Side-by-side diffs Delta from patch set Stats (+939 lines, -156 lines) Patch
M src/pathops/SkPathOpsTSect.h View 1 chunk +1 line, -1 line 0 comments Download
M tests/PathOpsExtendedTest.h View 1 chunk +0 lines, -1 line 0 comments Download
M tests/PathOpsExtendedTest.cpp View 3 chunks +2 lines, -14 lines 0 comments Download
M tests/PathOpsFuzz763Test.cpp View 3 chunks +4 lines, -3 lines 0 comments Download
M tests/PathOpsOpCircleThreadedTest.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M tests/PathOpsOpCubicThreadedTest.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M tests/PathOpsOpLoopThreadedTest.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M tests/PathOpsOpTest.cpp View 1 2 chunks +21 lines, -0 lines 0 comments Download
M tests/PathOpsSimplifyQuadThreadedTest.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M tests/PathOpsSimplifyTest.cpp View 1 chunk +38 lines, -0 lines 0 comments Download
M tools/pathops_visualizer.htm View 2 chunks +873 lines, -133 lines 0 comments Download

Messages

Total messages: 13 (8 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018513003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018513003/20001
4 years, 7 months ago (2016-05-26 15:35:03 UTC) #4
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 7 months ago (2016-05-26 15:57:48 UTC) #6
commit-bot: I haz the power
Dry run: None
4 years, 7 months ago (2016-05-26 15:57:55 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018513003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018513003/20001
4 years, 7 months ago (2016-05-26 16:00:36 UTC) #11
commit-bot: I haz the power
4 years, 7 months ago (2016-05-26 16:01:52 UTC) #13
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/2bec26a71698105729c6a7cb0163f499b4361840

Powered by Google App Engine
This is Rietveld 408576698