New issue
Advanced search Search tips

Issue 5415 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Area: ----
Priority: Medium
Type: Defect



Sign in to add a comment

Possible pointer overflow in SkPathRef::Iter::setPathRef.

Reported by chefm...@gmail.com, Jun 10 2016

Issue description

What steps will reproduce the problem?
1. Go to web page with chromium (http://browserbench.org/JetStream/)
2.
3.

What is the expected output? What do you see instead?

Expected output in shell - nothing.
Seen output in shell: 
../../third_party/skia/src/core/SkPath.cpp:1951:53: runtime error: unsigned integer overflow: 0 - 4 cannot be represented in type 'sizetype'

What version of the product are you using? On what operating system?

Skia version: 553
Target platform: arm-linux-gnueabi
Please provide any additional information below.

Hi,

when testing my home-made unsigned integer overflow tool (based on UBSan) on chromium, I noticed such a strange warning:

../../third_party/skia/src/core/SkPath.cpp:1951:53: runtime error: unsigned integer overflow: 0 - 4 cannot be represented in type 'sizetype'

Looking to the source code, I see:

void SkPath::RawIter::setPath(const SkPath& path) {
    fPts = path.fPathRef->points();
    fVerbs = path.fPathRef->verbs();
    fVerbStop = path.fPathRef->verbsMemBegin();
    fConicWeights = path.fPathRef->conicWeights() - 1; // begin one behind
}

Here, if path.fPathRef->conicWeights() == nullptr, we actually have a pointer wrapping to (-4) for fConicWeights and this looks quite scary to me (UB?). Is this intentional? If so, sorry for a noise.
 

Comment 1 by chefm...@gmail.com, Jun 14 2016

And one can I see, this code is still present in trunk Skia version, it was just moved to src/core/SkPathRef.cpp:

void SkPathRef::Iter::setPathRef(const SkPathRef& path) {
    fPts = path.points();
    fVerbs = path.verbs();
    fVerbStop = path.verbsMemBegin();
    fConicWeights = path.conicWeights() - 1; // begin one behind
}

Project Member

Comment 2 by hcm@google.com, Jun 14 2016

Cc: hcm@google.com
Owner: caryclark@google.com
Cary, can you chime in here?
Project Member

Comment 3 by caryclark@google.com, Jun 14 2016

Your homemade unsigned integer overflow tool?

https://codereview.chromium.org/2061833005 will correct the warning, but I am confused why you think this is a bug. Does this fail to create the correct output on some platform? Does it crash? Does it create a performance concern? Does it generate a warning on a mainstream tool?
Project Member

Comment 4 by caryclark@google.com, Jun 15 2016

Status: Unconfirmed (was: New)

Comment 5 by chefm...@gmail.com, Jun 15 2016

> Your homemade unsigned integer overflow tool?

Just like -fsanitize=unsigned-integer-overflow, but extended for pointer plus expressions.

I don't see any observable bugs right now, but using invalid pointer later may cause undefined behavior (say, compiler may optimize some range checks such as https://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html).
Project Member

Comment 6 by caryclark@google.com, Oct 4 2016

Status: Started (was: Unconfirmed)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 4 2016

The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/6942442ef7cc018ac136dd379ad6a30902a060e5

commit 6942442ef7cc018ac136dd379ad6a30902a060e5
Author: caryclark <caryclark@google.com>
Date: Tue Oct 04 20:06:17 2016

disallow -4 pointer

A user's homegrown unsigned integer overflow tool
complains if a nullptr is decremented.
The conicWeight pointer likes to predecrement
before walking, but this is unnecessary if
its value is nullptr.

R=reed@google.com
BUG= skia:5415 
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2061833005

Review-Url: https://codereview.chromium.org/2061833005

[modify] https://crrev.com/6942442ef7cc018ac136dd379ad6a30902a060e5/src/core/SkPath.cpp
[modify] https://crrev.com/6942442ef7cc018ac136dd379ad6a30902a060e5/src/core/SkPathRef.cpp

Project Member

Comment 8 by caryclark@google.com, Oct 4 2016

Status: Fixed (was: Started)

Sign in to add a comment