New issue
Advanced search Search tips

Issue 628868 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Integer-overflow in SkIRect::height

Project Member Reported by ClusterFuzz, Jul 16 2016

Issue description

Components: Internals>Skia
Labels: findit-for-crash M-52
Owner: mtklein@chromium.org
Status: Assigned (was: Available)
Suspected CLs	No CL in the regression range changes the crashed files. The result is the blame information.

Author: reed@android.com
Project: chromium-skia
Changelist: https://chromium.googlesource.com/skia.git/+/8a1c16ff38322f0210116fa7293eb8817c7e477e
Time: Wed Dec 17 15:59:43 2008
The CL last changed line 78 of file SkRect.h, which is stack frame 0.

Author: reed
Project: chromium-skia
Changelist: https://chromium.googlesource.com/skia.git/+/157f36d358814a2235aa6284b78a67b725076063
Time: Wed Oct 15 14:05:09 2014
The CL last changed line 80 of file SkRect.h, which is stack frame 1.

Author: mtklein
Project: chromium-skia
Changelist: https://chromium.googlesource.com/skia.git/+/feaadee1c38e1d4e1ec0069a3509ef6fbc5fbeff
Time: Wed Apr 08 18:25:48 2015
The CL last changed line 645 of file SkCanvas.cpp, which is stack frame 2.

Author: mtklein
Project: chromium-skia
Changelist: https://chromium.googlesource.com/skia.git/+/feaadee1c38e1d4e1ec0069a3509ef6fbc5fbeff
Time: Wed Apr 08 18:25:48 2015
The CL last changed line 59 of file SkRecorder.cpp, which is stack frame 3.

Author: mtklein
Project: chromium-skia
Changelist: https://chromium.googlesource.com/skia.git/+/d711d115d28b9838303dcc232516aa2f552f3a2a
Time: Wed Jul 01 14:04:37 2015
The CL last changed line 45 of file SkPictureRecorder.cpp, which is stack frame 4.

Author: fmalita
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/6f694423303b95fedc99848ad436817e169f022c
Time: Thu Mar 31 02:49:24 2016
The CL last changed line 269 of file GraphicsContext.cpp, which is stack frame 5.

Author: ortuno
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/3fd03b556675834c683e1453a168a874269b83fc
Time: Wed Jun 15 18:10:34 2016
The CL last changed line 256 of file optional.h, which is stack frame 6.

Suspected Project: chromium-skia
Suspected Component: Internals>Skia

Possible suspect : https://chromium.googlesource.com/skia.git/+/feaadee1c38e1d4e1ec0069a3509ef6fbc5fbeff

Please reassign if this is not related to your change.
Cc: reed@google.com
Looks like either we've either got an unsorted SkRect (bottom -2147483648, top 204) or we've roundedOut() into that situation when turning it into an SkIRect.

Will try something like,

static SkIRect safe_round_out(SkRect bounds) {
    // Do we need / want bounds.sort() ?
    if (bounds.intersect(SkIRect::MakeLargest())) {
        return bounds.roundOut();
    }
    return SkIRect::MakeEmpty();
}

Do we already have anything like that?  Didn't notice one.

Comment 4 by reed@google.com, Jul 18 2016

off-hand, its easy to make a height overflow:

top = -max_int
bottom = max_int

height = bottom - top


I've got the same issue basically in Blink's IntRect due to an in-range x() added to and in-range width() to get an out-of-range right edge position. https://bugs.chromium.org/p/chromium/issues/detail?id=628870

It seems like it's not a problem because negative values in integers are fine, at least for us. Checking for bounds on every integer operation will kill performance, so I want to suppress these ubsan warnings.

Is Skia in the same situation?
It's possible, but I'm not sure.  I'd like to continue digging into this tomorrow with a debugger.  This might be one of either of these two cases:
  - large and inverted float rect that could round out to a large inverted int rect
  - large and non-inverted float rect than cannot round out to int rect (it's not guaranteed, but on x86 typically out of range float->int casts produce -2147483648 like we see here)

Mike's right that the realistic limits for SkIRect are a lot lower than the types make them out to be, and in the context of a device-space clip for an SkPicture, they're even lower than that.  We are not yet in a technological position to support an SkPicture describing a petapixel space.

I suspect that code snippet in #3 is still going to be the right approach, perhaps with MakeLargest() replaced with something like MakeSaneClipUpperLimit().
Project Member

Comment 7 by ClusterFuzz, Jul 28 2016

ClusterFuzz has detected this issue as fixed in range 407167:408336.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6723049080750080

Fuzzer: miaubiz_svg_fuzzer
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  SkIRect::height
  SkCanvas::resetForNextPicture
  SkRecorder::reset
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=370022:370027
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=407167:408336

Minimized Testcase (1.58 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97vGl1zMMvk4K0owrSC5w1mv1km6ijMBFpssJBT-gi1a_CznA3MquaiEqEcvmIaEyEU3ITaHvnL7H2de1_yQiWm8TC_gDPr2Sy4V-TB3_waGVFOgO34FfhFL4vhIO6FqPDC_7Qwggn5xExSPfDlL786c6Lttw?testcase_id=6723049080750080

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs 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, Jul 28 2016

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 9 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment