New issue
Advanced search Search tips

Issue 749387 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Integer-overflow in SkEdge::setLine

Project Member Reported by ClusterFuzz, Jul 27 2017

Issue description

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

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.
 
Components: Internals>Skia
Labels: M-62 Test-Predator-Wrong-CLs

Comment 2 by enne@chromium.org, Jul 27 2017

Owner: enne@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by enne@chromium.org, Jul 27 2017

Cc: mtklein@chromium.org reed@google.com
What's Skia's philosophy on undefined integer overflow? Are you ignoring these? Release asserting? Clamping?
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.

Comment 5 by piman@chromium.org, Jul 29 2017

Cc: jsc...@chromium.org
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.
I'll check it out for sure.
Project Member

Comment 7 by ClusterFuzz, 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.
Project Member

Comment 8 by ClusterFuzz, Aug 2 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
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