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

Issue 715208 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Crash in IntMod

Project Member Reported by ClusterFuzz, Apr 25 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5424138018881536

Fuzzer: ifratric-browserfuzzer-v3
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: UNKNOWN
Crash Address: 
Crash State:
  IntMod
  blink::LayoutMultiColumnSet::PageRemainingLogicalHeightForOffset
  blink::LayoutBox::PageRemainingLogicalHeightForOffset
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=435261:438085

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5424138018881536


Issue filed automatically.

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: msten...@opera.com
Components: Blink>Layout
Labels: Test-Layout M-60 Test-Predator-Wrong
Author: mstensho
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src/+/506506eac78a8106c4e92866a59b8c724ddc8b88
Time: Fri Dec 02 11:50:25 2016
Lines 818 of file LayoutBlockFlow.cpp which potentially caused crash are changed in this cl (frame #5, "blink::LayoutBlockFlow::LayoutBlockChild").
Minimum distance from crash line to modified line: 0. (file: LayoutBlockFlow.cpp, crashed on: 818, modified: 818).

Comment 2 by msten...@opera.com, Apr 26 2017

Owner: msten...@opera.com

Comment 3 by msten...@opera.com, Apr 26 2017

tc.html
252 bytes View Download

Comment 4 by msten...@opera.com, Apr 26 2017

Looks similar to  issue 712626 .
tc2.html
351 bytes View Download
Project Member

Comment 5 by ClusterFuzz, Apr 28 2017

ClusterFuzz has detected this issue as fixed in range 467574:467606.

Detailed report: https://clusterfuzz.com/testcase?key=5424138018881536

Fuzzer: ifratric-browserfuzzer-v3
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: UNKNOWN
Crash Address: 
Crash State:
  IntMod
  blink::LayoutMultiColumnSet::PageRemainingLogicalHeightForOffset
  blink::LayoutBox::PageRemainingLogicalHeightForOffset
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=435261:438085
Fixed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=467574:467606

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5424138018881536


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 6 by ClusterFuzz, Apr 28 2017

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

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

Comment 7 by msten...@opera.com, May 11 2017

Status: Assigned (was: Verified)
Probably stopped crashing because motion-path has been removed (it's now offset-path). tc2.html (which uses offset-path) still crashes.
Project Member

Comment 8 by bugdroid1@chromium.org, May 12 2017

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

commit 458588591a96346f4f2fc8c71a2e76a70f3eec5c
Author: mstensho <mstensho@opera.com>
Date: Fri May 12 16:39:08 2017

Distinguish between row (fragmentainer group) height and column (fragmentainer) height.

TLDR; The spec [1] says that a fragmentainer height must always be 1px or
greater, to ensure progress. If we just do this, we'll avoid a lot of trouble
with limitations in the data types that we use.

While *column* heights will now be clamped to >= 1px, we still allow the height
of a *row* (fragmentainer group) to be less than 1px. We don't want the row to
take up more space than it should in its container.
E.g. <div style="columns:2; height:0.5px;"><div style="height:1px";></div></div>
will give a row height of 0.5px, as specified. The column height, on the other
hand, should be clamped up to 1px.

And here, for the nastiness that this CL aims to fix:
<div style="columns:2; height:0.25px;">
    <div style="height:10000000px;"></div>
</div>
The content to fragment is 10 million pixels tall, and the column height has
been specified as 0.25px. Internally in our code, heights are stored as
LayoutUnit, which is a fixed-point unit with 6 bits reserved for decimals. On
the other side of the decimal point we have room for 32-6 bits = 26 bits, which
is what we have for a signed integer. That's 25 bits for the absolute value.
That's just over 30 million. LayoutUnit uses saturated arithmetic so there'll
never be any integer overflow or underflow, but there may be other ill effects.
Like in this case, if we actually allow a column height of less than 1px (i.e.
0.25px), when the engine for example wants to figure out the *actual* column
count (column count was *specified* as 2, but there's no way we're going to be
able to fit a 10 millions pixels tall thing in two columns when the column
height is 0.25px, so the actual count will be way higher), we take the flow
thread portion (10000000px) and divide by the column height (0.25px). If we
divide something by something (positive) less than 1, we of course end up with
a quotient larger than the dividend. While the dividend may be small enough to
fit unclamped in a LayoutUnit (10000000px fits just fine), the quotient
(40000000) may not. So, while the actual column count really is 40 million (if
we allow columns to be shorter than 1px), the engine will clamp the 40 million
to fit inside a LayoutUnit. That's 33554431. This is the root of the problem,
and this incorrect column count value may in turn lead to other bad things,
even negative column heights in subsequent rows (and good luck calculating a
used column count off that!). It would probably be possible to cope with this,
if we only take extra care everywhere, when dealing with close-to-insane
numbers.

Or we can just do what the spec says, and clamp column heights to >= 1px.

[1] https://drafts.csswg.org/css-break/#breaking-rules

BUG= 712626 , 715208 

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

[add] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/LayoutTests/fast/multicol/infinite-height-causing-fractional-row-height-crash.html
[add] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/LayoutTests/fast/multicol/insane-column-count-and-padding-nested-crash.html
[add] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/LayoutTests/fast/multicol/zero-height-with-children.html
[modify] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/Source/core/layout/FragmentainerIterator.cpp
[modify] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp
[modify] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp
[modify] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.h
[modify] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp
[modify] https://crrev.com/458588591a96346f4f2fc8c71a2e76a70f3eec5c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h

Comment 9 by msten...@opera.com, May 12 2017

Status: Fixed (was: Assigned)

Sign in to add a comment