New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 638223 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Use-of-uninitialized-value in Break

Project Member Reported by ClusterFuzz, Aug 16 2016

Issue description

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

Fuzzer: libfuzzer_skia_path_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  Break
  add
  SuperBlitter::blitH
  
Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_msan&range=400429:400488

Minimized Testcase (0.05 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97WfdhLtdBZy2HuQjbZHEVGqkb0snOE3kKP_8nMpjiN-Lg3zYQ3ROeBoYp7EwpoWsTpuPvI1Oh199Ml7mvoB1pcaMSHOyvus8weBcmm-JueNYm4oyp718mM0NXJSa8Zz8BZ3F-zz-NeXdYFGT-rbTm4n5SyLQ?testcase_id=6523091515867136

Issue manually filed by: mmoroz

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 

Comment 1 by mmoroz@chromium.org, Aug 16 2016

Components: Internals>Skia
Labels: Pri-1
Owner: reed@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 3 by sheriffbot@chromium.org, Aug 17 2016

Labels: M-53
Project Member

Comment 4 by sheriffbot@chromium.org, Aug 17 2016

Labels: ReleaseBlock-Stable

Comment 5 by gov...@chromium.org, Aug 18 2016

Cc: awhalley@chromium.org
Please try to resolve this ASAP as we're very close to M53 Stable promotion. Please request a merge to M53 branch 2785 once change is landed/baked/verified in Canary. Thank you.

Comment 6 by gov...@chromium.org, Aug 22 2016

A friendly reminder that M53 Stable is launching VERY soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP (before 5:00 PM PT, Tuesday) so we can take it for this week LAST Beta release for Desktop. Thank you!

Note: Merge has to happen by Friday, August 26th, 5:00 PM PST in order to make into the desktop Stable final build cut. 

Comment 7 by gov...@chromium.org, Aug 22 2016

Cc: hcm@chromium.org

Comment 8 by hcm@chromium.org, Aug 22 2016

Cc: caryclark@chromium.org mtklein@chromium.org
Owner: reed@google.com
I am not sure this will make it with reed being out and the given security severity, but trying mtklein and caryclark in his absence, in case they can come up with a quick fix.
I'll take a look at it
Cc: jcgregorio@chromium.org
Here's an isolated example:

DEF_SIMPLE_GM(fuzzpath, canvas, 250, 250) {
    SkPath path;
    path.moveTo(SkBits2Float(0x47452a00), SkBits2Float(0x43211d01));  // 50474, 161.113f
    path.conicTo(SkBits2Float(0x401c0000), SkBits2Float(0x40680000), 
            SkBits2Float(0x02c25a81), SkBits2Float(0x981a1fa0), 
            SkBits2Float(0x6bf9abea));  // 2.4375f, 3.625f, 2.85577e-37f, -1.992e-24f, 6.03669e+26f
    SkPaint paint;
    paint.setAntiAlias(true);
    canvas->drawPath(path, paint);
}

In a Skia debug SampleApp environment, this triggers at SkScan_Path.cpp:274: walk_convex_edges():
    SkASSERT(local_top <= local_bot);  // local_bot: 14   local_top: 15

The edge list for leftE contains 17 consecutive entries that all have fFirstY = fLastY = 14. 
Since the edge does not advance, the assert fires. The 18th edge correctly has fFirstY = fLastY = 15.

The edges are generated by quadder.computeQuads(). 
Most of the generated quads have the same or similar point data:

quadder.countQuads()
    32

quadPts,32
    [0]: {fX=50474.0000 fY=161.113297 }
    [1]: {fX=2010.31287 fY=9.89024830 }
    [2]: {fX=1026.74988 fY=6.82120037 }
    [3]: {fX=43.1870117 fY=3.75215220 }
    [4]: {fX=23.2257614 fY=3.68986654 }
    [5]: {fX=3.26450515 fY=3.62758064 }
    [6]: {fX=2.85939431 fY=3.62631679 }
    [7]: {fX=2.45428395 fY=3.62505269 }
    [8]: {fX=2.44606209 fY=3.62502694 }
    [9]: {fX=2.43784046 fY=3.62500095 }
    [10]: {fX=2.43767357 fY=3.62500048 }
    [11]: {fX=2.43750691 fY=3.62499976 }
    [12]: {fX=2.43750381 fY=3.62500000 }
    [13]: {fX=2.43750000 fY=3.62499976 }
    [14]: {fX=2.43750000 fY=3.62499976 }
    [15]: {fX=2.43750000 fY=3.62499976 }
    [16]: {fX=2.43750000 fY=3.62500000 }
    [17]: {fX=2.43750000 fY=3.62499976 }
    [18]: {fX=2.43750000 fY=3.62499976 }
    [19]: {fX=2.43750000 fY=3.62499976 }
    [20]: {fX=2.43750000 fY=3.62500000 }
    [21]: {fX=2.43750000 fY=3.62499976 }
    [22]: {fX=2.43750000 fY=3.62499976 }
    [23]: {fX=2.43750000 fY=3.62499976 }
    [24]: {fX=2.43749976 fY=3.62500000 }
    [25]: {fX=2.43750000 fY=3.62499976 }
    [26]: {fX=2.43750000 fY=3.62499976 }
    [27]: {fX=2.43750000 fY=3.62499976 }
    [28]: {fX=2.43750000 fY=3.62500000 }
    [29]: {fX=2.43750000 fY=3.62499976 }
    [30]: {fX=2.43750000 fY=3.62499976 }
    [31]: {fX=2.43750000 fY=3.62499976 }

One possible solution is to detect when the chopped conic is a line. 
After the first chop in subdivide(), we get:

dst[0].fPts
    [0]: {fX=50474.0000 fY=161.113297 }
    [1]: {fX=2.43750000 fY=3.62499976 }
    [2]: {fX=2.43750000 fY=3.62499976 }
dst[0].fW
    1.73733807e+13
dst[1].fPts
    [0]: {fX=2.43750000 fY=3.62499976 }
    [1]: {fX=2.43750000 fY=3.62499976 }
    [2]: {fX=2.85576847e-37 fY=-1.99200000e-24 }
dst[1].fW
    1.73733807e+13

This would require a bit of restructuring so that chopIntoQuadsPOW2 
could return a samller number of quads than 1 << pow2.

If Mike would like to hand this off, I can work on a CL.
Cc: -caryclark@chromium.org caryclark@google.com
Cc: -jcgregorio@chromium.org jcgrego...@google.com
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4749f670b86702a7ced89ee5caab98018f2d2ff5

commit 4749f670b86702a7ced89ee5caab98018f2d2ff5
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Tue Aug 23 20:15:22 2016

Roll src/third_party/skia/ afdc177e7..5eb8bffe4 (4 commits).

https://chromium.googlesource.com/skia.git/+log/afdc177e77a4..5eb8bffe42f0

$ git log afdc177e7..5eb8bffe4 --date=short --no-merges --format='%ad %ae %s'
2016-08-23 halcanary experimental/tools/mskp_parser.py
2016-08-23 bungeman Revert of Moving SkBlurImageFilter into core (patchset #9 id:160001 of https://codereview.chromium.org/2255803003/ )
2016-08-23 caryclark fix conic path fuzz
2016-08-23 halcanary SkMultiSKP: version 2

BUG= 638223 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
TBR=bungeman@google.com

Review-Url: https://codereview.chromium.org/2271733002
Cr-Commit-Position: refs/heads/master@{#413825}

[modify] https://crrev.com/4749f670b86702a7ced89ee5caab98018f2d2ff5/DEPS

Project Member

Comment 15 by ClusterFuzz, Aug 25 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 16 by sheriffbot@chromium.org, Aug 25 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Request-53

Comment 18 by dimu@chromium.org, Aug 25 2016

Labels: -Merge-Request-53 Merge-Review-53 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Labels: -Merge-Review-53 Merge-Approved-53
Approving merge to M53 branch 2785 per chat with awhalley@. Please merge ASAP or latest by Friday, August 26th, 5:00 PM PT in order to make into the desktop Stable final build cut. Thank you.

Project Member

Comment 20 by ClusterFuzz, Aug 26 2016

ClusterFuzz has detected this issue as fixed in range 414243:414324.

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

Fuzzer: libfuzzer_skia_path_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  Break
  add
  SuperBlitter::blitH
  
Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_msan&range=400429:400488
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_msan&range=414243:414324

Minimized Testcase (0.05 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97WfdhLtdBZy2HuQjbZHEVGqkb0snOE3kKP_8nMpjiN-Lg3zYQ3ROeBoYp7EwpoWsTpuPvI1Oh199Ml7mvoB1pcaMSHOyvus8weBcmm-JueNYm4oyp718mM0NXJSa8Zz8BZ3F-zz-NeXdYFGT-rbTm4n5SyLQ?testcase_id=6523091515867136

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 21 by sheriffbot@chromium.org, Aug 29 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Time is short for today's early stable spin (which we shouldn't block) - will be merged in time second release.
Ok, sounds good. Thank you awhalley@.
Per comment #24, this is already merged to M53. If nothing is pending for M53, please remove "Merge-Approved-53" label. Thank you.
Labels: -Merge-Approved-53
Project Member

Comment 27 by sheriffbot@chromium.org, Dec 1 2016

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

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

Sign in to add a comment