Attaching SKP. Causes assertion:
../../include/core/SkRect.h:269: fatal error: ""left < right && top < bottom""

Deleted: layer_0.skp 13.7 MB

layer_0.skp 13.7 MB Download

The textblob contains NaN in one of its (Y) coordinates. This in turn triggers bad bounds calculations.

- detect/fix/abort during textblob construction?
- detect/etc. during each draw?
- other?

  letter-spacing: 170141183460469231731687303715884105727mm;

That's apparently large enough to overflow floats and we get degenerate glyph offsets.

Dominik, should the shaper or CSS parser sanitize this?  We could catch it at blob construction time, but it seems it should be handled earlier.

A friendly reminder that M51 Stable is launching soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch by May 17. All changes MUST be merged into the release branch by 5pm on May 20 to make into the desktop Stable final build cut. Thanks!

Tim, do you know of more such examples? Where should this be captured, at the parsing stage or later, when assigning it to FontDescription?

M51 Stable is launching very soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged ASAP. All changes MUST be merged into the release branch by 5pm on May 20 to make into the desktop Stable final build cut. Thank you!

The high letter-spacing is not the root of the issue here. It seems to be that we're taking the horizontal glyph buffer fast path (fillFastHorizontalGlyphBuffer) for a combination of a Chinese character, an Arabic ligatures, vertical text and non-null letter spacing. I've reduced it to:

疺ﲷ
<style>* { writing-mode: vertical-lr; letter-spacing: 1;}

Actually, just 

ﲷ
<style>* { writing-mode: vertical-lr; letter-spacing: 1;}

triggers the crash as well.

You're talking about the !glyphData.offset.height() assert, right?  I think that's a separate issue triggered by the same CF test.

I have a fix for it (http://crrev.com/1982313002) but it doesn't help with the numberic overflow in the CF crashes.

Great! Thanks a lot for the fix. I'll look into the remaining overflow issue.

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

commit 239180b1fd08ebcf0f3098c1238077f6c0998588
Author: fmalita <fmalita@chromium.org>
Date: Wed May 18 07:40:48 2016

Re-evaluate ShapeResult::m_hasVerticalOffsets after applying spacing

ShapeResult::applySpacing() may introduce vertical offsets, so we need
to update m_hasVerticalOffsets also.

BUG= 610645 
R=drott@chromium.org

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

[add] https://crrev.com/239180b1fd08ebcf0f3098c1238077f6c0998588/third_party/WebKit/LayoutTests/fast/text/vertical-spacing-crash-expected.txt
[add] https://crrev.com/239180b1fd08ebcf0f3098c1238077f6c0998588/third_party/WebKit/LayoutTests/fast/text/vertical-spacing-crash.html
[modify] https://crrev.com/239180b1fd08ebcf0f3098c1238077f6c0998588/third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp

CL up for fixing overflow issue: https://codereview.chromium.org/1987223002

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

commit 6dd652af9ab5089938ce8c24513b4a768d0c5b45
Author: drott <drott@chromium.org>
Date: Wed May 18 12:19:43 2016

Clamp, not cast length value in CSS length conversion

computeLength() in CSSPrimitiveValue was returning only static_cast'ed,
but not clamped values, leading to float overflows in font and Skia code
down the line.

The test result needs to be compared visually as the crash does not trigger
using dumpAsText().

BUG= 610645 
TEST=fast/text/letter-spacing-crash.html

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

[add] https://crrev.com/6dd652af9ab5089938ce8c24513b4a768d0c5b45/third_party/WebKit/LayoutTests/fast/text/letter-spacing-crash-expected.html
[add] https://crrev.com/6dd652af9ab5089938ce8c24513b4a768d0c5b45/third_party/WebKit/LayoutTests/fast/text/letter-spacing-crash.html
[modify] https://crrev.com/6dd652af9ab5089938ce8c24513b4a768d0c5b45/third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp

Before we approve merge to M51, Could you please confirm whether this bug is baked/verified in Canary and safe to merge?

Verified that the 51 beta still crashes on Windows with the test case from r394390, and that the Windows 52.2741 Canary, which is at r394609 and includes the CL, fixes the issue.

I consider the CL safe to merge. It's essentially a one line fix that avoids truncation in a double to float conversion.

Your change meets the bar and is auto-approved for M51 (branch: 2704)

Please merge your change to M51 branch 2704 ASAP as we're getting closer to M51 stable launch.

ClusterFuzz has detected this issue as fixed in range 394251:394739.

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

Fuzzer: inferno_twister
Job Type: linux_lsan_chrome_mp
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x61a00002b200
Crash State:
  SkAAClipBlitter::blitMask
  DrawOneGlyph::blitMask
  DrawOneGlyph::operator
  
Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=383194:384397
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=394251:394739

Minimized Testcase (0.51 Kb): https://cluster-fuzz.appspot.com/download/AMIfv957rb0tM2hPk-8QKIp9ON_AE-SgmKuBU-eulGkoVrofAM_HlPo33o4b9Qo86V6_xPOCC9HmFo1hQ9eMpivQW44sgMGFCO5dnncZZ8wHRcz20zf6-ILS-jyjeNt8MjDThz7jprzAD9z0h3_9v3wJ9UZ3eyo7hA

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.

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

commit f48d7f9108d7718324e9c9e5cd7008a8862dc12c
Author: Dominik Röttsches <drott@chromium.org>
Date: Fri May 20 07:18:40 2016

Clamp, not cast length value in CSS length conversion

computeLength() in CSSPrimitiveValue was returning only static_cast'ed,
but not clamped values, leading to float overflows in font and Skia code
down the line.

The test result needs to be compared visually as the crash does not trigger
using dumpAsText().

BUG= 610645 
TEST=fast/text/letter-spacing-crash.html

Review-Url: https://codereview.chromium.org/1987223002
Cr-Commit-Position: refs/heads/master@{#394390}
(cherry picked from commit 6dd652af9ab5089938ce8c24513b4a768d0c5b45)

Review URL: https://codereview.chromium.org/2003453002 .

Cr-Commit-Position: refs/branch-heads/2704@{#614}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[add] https://crrev.com/f48d7f9108d7718324e9c9e5cd7008a8862dc12c/third_party/WebKit/LayoutTests/fast/text/letter-spacing-crash-expected.html
[add] https://crrev.com/f48d7f9108d7718324e9c9e5cd7008a8862dc12c/third_party/WebKit/LayoutTests/fast/text/letter-spacing-crash.html
[modify] https://crrev.com/f48d7f9108d7718324e9c9e5cd7008a8862dc12c/third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp

Issue 619370 has been merged into this issue.

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

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

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