This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

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

fmalita, can you please find a good owner for this, or handle it? Thanks. mstensho seems to own the function at the top of the stack.

The crash repros in Linux ASAN builds too, and the trigger appears to be an int overflow from (-2147483648 % -1).

Looks like a problem with degenerate value handling in LayoutMultiColumnSet.  Assigning to eae@ for layout expertise.

Clusterfuzz considers this a security issue, over to Morten.

High impact == stable blocker, don't need to block beta as far as I'm concerned.  awhalley@, correct me if you disagree.

What happended to Clusterfuzz and its ability to minimize test cases?

Deleted: reduced.html 520 bytes

reduced.html 520 bytes View Download

Any updates?

Yes, I'm investigating. I don't understand how this can be a security issue, though.

Integer overflows lead to undefined behavior, so they are considered security vulnerabilities. For example, if that int is used to allocate a buffer, the resulting buffer will be too small, and memory gets corrupted.

Is the fix not a matter of checking for the overflow condition in the code and failing the operation?

No buffer allocations use the result here. It's just a block-axis offset in layout.

The fix will most likely be to avoid ending up with silly values here (figure out the root cause), rather than guarding against them here locally.

Agreed. This definitely doesn't seem like a security issue.

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

ClusterFuzz has detected this issue as fixed in range 471323:471361.

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

Fuzzer: bj_broddelwerk
Job Type: windows_asan_chrome
Platform Id: windows

Crash Type: Int-overflow
Crash Address: 0x00000000
Crash State:
  blink::LayoutMultiColumnSet::pageRemainingLogicalHeightForOffset
  blink::LayoutFlowThread::pageRemainingLogicalHeightForOffset
  blink::LayoutBox::pageRemainingLogicalHeightForOffset
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome&range=456190:456233
Fixed: https://clusterfuzz.com/revisions?job=windows_asan_chrome&range=471323:471361

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


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.