Integer-overflow in SkEdge::setLine |
|||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5453705091743744 Fuzzer: libFuzzer_paint_op_buffer_fuzzer Job Type: libfuzzer_chrome_ubsan Platform Id: linux Crash Type: Integer-overflow Crash Address: Crash State: SkEdge::setLine SkEdgeBuilder::addLine SkEdgeBuilder::addClipper Sanitizer: undefined (UBSAN) Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=489418:489502 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5453705091743744 Issue filed automatically. See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
,
Jul 27 2017
,
Jul 27 2017
What's Skia's philosophy on undefined integer overflow? Are you ignoring these? Release asserting? Clamping?
,
Jul 27 2017
We are now solidly aware that undefined == possibly optimized through as if it can't happen, and we're pretty aware of what's defined and not. I think we'd prefer to fix them, but we've never been able to get much momentum rolling behind that. We tend to treat these as low priority, low severity. I think our ideal rollout for this would be: 1) find a way to detect overflow and underflow on as many platforms as possible with an API that doesn't get itself too much in the way of readability and implementation that doesn't get too much in the way of performance (e.g. we'd like to avoid dividing only to check that a multiply won't overflow); 2) make sure we've got these sanitizers turned on and green on our local bots, spot fixing each problem as situationally appropriate. I'd generally prefer not to release-assert if we have a sane default or max or min to fall back on, or the ability to rewrite the code to just return a failure. 3) tackle any remaining Chrome ubsan issues as they come up. I think we've got a fair bunch of ideas for how to do 1), with some edge cases to work out. GCC and Clang have __builtin_op_overflow() for several op, but only since GCC 5, and we still seem to be building with 4.8. 32-bit platforms can generally promote to 64-bit, and GCC and Clang can even promote up to __int128. My quick attempts to use _umul128() on Win64 haven't yet worked correctly, but we've always got the option to punt on any particular platform, essentially keeping the unchecked status quo.
,
Jul 29 2017
Any interest in adopting base/numerics/checked_math.h and friends in skia? Justin spent some fair amount of time making sure they optimize ideally on all compilers/archs, and have good syntactic sugar on top of them.
,
Jul 29 2017
I'll check it out for sure.
,
Aug 2 2017
ClusterFuzz has detected this issue as fixed in range 491107:491189. Detailed report: https://clusterfuzz.com/testcase?key=5453705091743744 Fuzzer: libFuzzer_paint_op_buffer_fuzzer Job Type: libfuzzer_chrome_ubsan Platform Id: linux Crash Type: Integer-overflow Crash Address: Crash State: SkEdge::setLine SkEdgeBuilder::addLine SkEdgeBuilder::addClipper Sanitizer: undefined (UBSAN) Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=489418:489502 Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=491107:491189 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5453705091743744 See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Aug 2 2017
ClusterFuzz testcase 5453705091743744 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by msrchandra@chromium.org
, Jul 27 2017Labels: M-62 Test-Predator-Wrong-CLs