Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2111473004:
fix asan bug triggered by pathops fuzz tests (Closed)
Descriptionfix asan bug triggered by pathops fuzz tests
R=dogben@google.com
TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2111473004
Committed: https://skia.googlesource.com/skia/+/3cf2e20139f07b906aa3b25de85464a56d9d6f3f
Patch Set 1 #
Total comments: 1
Messages
Total messages: 20 (8 generated)
Description was changed from ========== fix asan bug triggered by pathops fuzz tests R=dogben@google.com ========== to ========== fix asan bug triggered by pathops fuzz tests R=dogben@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2111473004 ==========
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== fix asan bug triggered by pathops fuzz tests R=dogben@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2111473004 ========== to ========== fix asan bug triggered by pathops fuzz tests R=dogben@google.com TBR=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2111473004 ==========
caryclark@google.com changed reviewers: + reed@google.com
benjaminwagner@google.com changed reviewers: + benjaminwagner@google.com
https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h File src/pathops/SkOpSegment.h (right): https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h#n... src/pathops/SkOpSegment.h:340: if (*sumWinding == SK_MinS32) { It appears that SpanSign can return a value > 1 if windValue() is > 1. Should this instead check sumWinding - Sk_MinS32 <= deltaSum?
On 2016/06/29 20:46:17, Ben Wagner wrote: > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h > File src/pathops/SkOpSegment.h (right): > > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h#n... > src/pathops/SkOpSegment.h:340: if (*sumWinding == SK_MinS32) { > It appears that SpanSign can return a value > 1 if windValue() is > 1. Should > this instead check sumWinding - Sk_MinS32 <= deltaSum? The winding is set to Sk_MinS32 to note that it is uninitialized. Anytime setupWinding() is called and the winding is uninitialized, something went badly wrong earlier. The test triggering this case has random numbers that cause pathops numerics to fail. Since this is already a failure case, it's OK to do nothing and let pathops figure out later on that the input is garbage.
On 2016/06/29 20:46:17, Ben Wagner wrote: > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h > File src/pathops/SkOpSegment.h (right): > > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h#n... > src/pathops/SkOpSegment.h:340: if (*sumWinding == SK_MinS32) { > It appears that SpanSign can return a value > 1 if windValue() is > 1. Should > this instead check sumWinding - Sk_MinS32 <= deltaSum? The winding is set to Sk_MinS32 to note that it is uninitialized. Anytime setupWinding() is called and the winding is uninitialized, something went badly wrong earlier. The test triggering this case has random numbers that cause pathops numerics to fail. Since this is already a failure case, it's OK to do nothing and let pathops figure out later on that the input is garbage.
On 2016/06/29 20:46:17, Ben Wagner wrote: > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h > File src/pathops/SkOpSegment.h (right): > > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h#n... > src/pathops/SkOpSegment.h:340: if (*sumWinding == SK_MinS32) { > It appears that SpanSign can return a value > 1 if windValue() is > 1. Should > this instead check sumWinding - Sk_MinS32 <= deltaSum? The winding is set to Sk_MinS32 to note that it is uninitialized. Anytime setupWinding() is called and the winding is uninitialized, something went badly wrong earlier. The test triggering this case has random numbers that cause pathops numerics to fail. Since this is already a failure case, it's OK to do nothing and let pathops figure out later on that the input is garbage.
On 2016/06/29 20:49:53, caryclark wrote: > On 2016/06/29 20:46:17, Ben Wagner wrote: > > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h > > File src/pathops/SkOpSegment.h (right): > > > > > https://codereview.chromium.org/2111473004/diff/1/src/pathops/SkOpSegment.h#n... > > src/pathops/SkOpSegment.h:340: if (*sumWinding == SK_MinS32) { > > It appears that SpanSign can return a value > 1 if windValue() is > 1. Should > > this instead check sumWinding - Sk_MinS32 <= deltaSum? > > The winding is set to Sk_MinS32 to note that it is uninitialized. Anytime > setupWinding() is called and the winding is uninitialized, something went badly > wrong earlier. > > The test triggering this case has random numbers that cause pathops numerics to > fail. Since this is already a failure case, it's OK to do nothing and let > pathops figure out later on that the input is garbage. Ok, that makes sense.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caryclark@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== fix asan bug triggered by pathops fuzz tests R=dogben@google.com TBR=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2111473004 ========== to ========== fix asan bug triggered by pathops fuzz tests R=dogben@google.com TBR=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2111473004 Committed: https://skia.googlesource.com/skia/+/3cf2e20139f07b906aa3b25de85464a56d9d6f3f ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/3cf2e20139f07b906aa3b25de85464a56d9d6f3f
Message was sent while issue was closed.
CQ bit was unchecked. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
