Doug: could you please help triage this? It looks like it's happening in layout. Thanks!

Layout team will pick it up if you just leave it in the untriaged state.

Assigning to eae (as per discussion with dstockwell). eae - could you please help triage?

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

Tanin, did you see FindIt result in report. There is information on regresse.

Author: svillar
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/1c073ae5204b186aa0695ecedc80abc5225d85a4
Time: Thu Jul 14 19:06:11 2016
File LayoutGrid.cpp is changed in this cl (and is part of stack frame #7, "blink::LayoutGrid::populateExplicitGridAndOrderIterator")
Minimum distance from crash line to modified line: 25. (file: LayoutGrid.cpp, crashed on: 1616, modified: 1641).

Suspected Project: chromium
Suspected Component: Blink>Layout

Any updates?  M54's branch point is coming up and this is a Beta blocker.

svillar: Have you had a chance to look into this? It's a high-priority security issue. It looks like a release assert or (better yet) a size check be sufficient to fix it but you know the code better than I.

raymes: In the future please leave bugs like this as untriaged instead of assigning, they'll be picked up quicker than way given how our triage process works! Thanks.

svillar: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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

Interestingly this only reproduces in layout test mode, not when running normally in content shell or chrome, unless I did something wrong. Attaching a full stack trace.

This is the problematic line:
    m_grid.grow(maximumRowIndex + abs(m_smallestRowStart));

But we're passing 1 + abs(0) here, and m_grid.size() is 2. Vector capacity() is 2. I don't know why this is ending up passing -16??

Deleted: stack.txt 11.6 KB

stack.txt 11.6 KB View Download

Ah, I understand the problem -- we call grow(1) but the size is 2. Vector does not support "growing" to a smaller size and would assert in a debug build.

Debug builds actually assert earlier on this testcase.

Sergio/Rego can you take a look? Thanks!

Also, this should not be a beta blocker as grid is not enabled by default.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in?q=RuntimeEnable&sq=package:chromium&l=61

Clearing release block label.

Although... maybe we should not ship security issues in "enable experimental web features"? Adding it back for now.

As @svillar is on holidays, I've been taking a look.
In my case I'm getting a different backtrace:

ASSERTION FAILED: m_gridItemsIndexesMap.isEmpty()
../../third_party/WebKit/Source/core/layout/LayoutGrid.cpp(1570) : void blink::LayoutGrid::populateExplicitGridAndOrderIterator()
1   0x7f5505c9172f
2   0x7f5505c8c9d3
3   0x7f5505c8b911
4   0x7f5505bfa73f blink::LayoutBlock::layout()
5   0x7f5505c0cd70 blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded(blink::LayoutBox&, blink::LayoutUnit, blink::BlockChildrenLayoutInfo&)
6   0x7f5505c0d110 blink::LayoutBlockFlow::layoutBlockChild(blink::LayoutBox&, blink::BlockChildrenLayoutInfo&)
7   0x7f5505c1184e blink::LayoutBlockFlow::layoutBlockChildren(bool, blink::SubtreeLayoutScope&, blink::LayoutUnit, blink::LayoutUnit)
8   0x7f5505c1d12b
9   0x7f5505c0bf93 blink::LayoutBlockFlow::layoutBlock(bool)
10  0x7f5505bfa73f blink::LayoutBlock::layout()
11  0x7f5505c0cd70 blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded(blink::LayoutBox&, blink::LayoutUnit, blink::BlockChildrenLayoutInfo&)
12  0x7f5505c0d110 blink::LayoutBlockFlow::layoutBlockChild(blink::LayoutBox&, blink::BlockChildrenLayoutInfo&)
13  0x7f5505c1184e blink::LayoutBlockFlow::layoutBlockChildren(bool, blink::SubtreeLayoutScope&, blink::LayoutUnit, blink::LayoutUnit)
14  0x7f5505c1d12b
15  0x7f5505c0bf93 blink::LayoutBlockFlow::layoutBlock(bool)
16  0x7f5505bfa73f blink::LayoutBlock::layout()
17  0x7f5505c0cd70 blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded(blink::LayoutBox&, blink::LayoutUnit, blink::BlockChildrenLayoutInfo&)
18  0x7f5505c0d110 blink::LayoutBlockFlow::layoutBlockChild(blink::LayoutBox&, blink::BlockChildrenLayoutInfo&)
19  0x7f5505c1184e blink::LayoutBlockFlow::layoutBlockChildren(bool, blink::SubtreeLayoutScope&, blink::LayoutUnit, blink::LayoutUnit)
20  0x7f5505c1d12b
21  0x7f5505c0bf93 blink::LayoutBlockFlow::layoutBlock(bool)
22  0x7f5505bfa73f blink::LayoutBlock::layout()
23  0x7f5505d38f15 blink::LayoutView::layoutContent()
24  0x7f5505d39632 blink::LayoutView::layout()
25  0x7f55057f822c
26  0x7f55057f7d53 blink::FrameView::performLayout(bool)
27  0x7f55057f55d0 blink::FrameView::layout()


BTW, the test case seems very related to  bug #621517 .
I'm attaching a reduced test case.

It seems that for some strange reason we're calling LayoutGrid::populateExplicitGridAndOrderIterator(
when m_gridIsDirty is FALSE.
If we remove the call to computeAutoRepeatTracksCount(ForColumns) in LayoutGrid::placeItemsOnGrid()
then the crash disappears.

Deleted: fuzz-23-reduced.html 183 bytes

fuzz-23-reduced.html 183 bytes View Download

Thanks for investigating. To get the stack trace that I/clusterfuzz produced, you need to build a release ASAN build (https://www.chromium.org/developers/testing/addresssanitizer). But that could well be the same underlying issue.

Yeah @cbiesinger I managed to reproduce the issue in an ASAN build too.

BTW, I've just uploaded a simple patch that avoids the crash in debug and ASAN:
https://codereview.chromium.org/2254843002/

Anyway this probably requires further investigation to review if we're doing some stuff that we could avoid in computeAutoRepeatTracksCount().

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

commit 49bc5ba13c492f09971882e2cb842e3514d19922
Author: rego <rego@igalia.com>
Date: Wed Aug 17 21:14:53 2016

[css-grid] Avoid computing twice the number of auto-repeat columns

During the track sizing layout, we were computing twice the number
of auto-repeat columns.
That was causing crashes on debug and ASAN builds.
The patch computes the value once and pass it to
LayoutGrid::placeItemsOnGrid(), which avoids the issue.

Add new cases that were crashing to test introduced in r405529.

BUG= 633474 
TBR=svillar@igalia.com
TEST=fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html

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

[modify] https://crrev.com/49bc5ba13c492f09971882e2cb842e3514d19922/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash-expected.txt
[modify] https://crrev.com/49bc5ba13c492f09971882e2cb842e3514d19922/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html
[modify] https://crrev.com/49bc5ba13c492f09971882e2cb842e3514d19922/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/49bc5ba13c492f09971882e2cb842e3514d19922/third_party/WebKit/Source/core/layout/LayoutGrid.h

ClusterFuzz has detected this issue as fixed in range 412599:412724.

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

Fuzzer: inferno_twister
Job Type: linux_asan_content_shell_drt
Platform Id: linux

Crash Type: Negative-size-param
Crash Address: 
Crash State:
  blink::LayoutGrid::populateExplicitGridAndOrderIterator
  blink::LayoutGrid::placeItemsOnGrid
  blink::LayoutGrid::layoutBlock
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=405500:405563
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=412599:412724

Minimized Testcase (2.01 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95y70Bi5ReQ7cmHChRGHR-sjHnV1pVgynGmisT5PPhABCJz0kdBdAhlwmAAUUbAEpDraGQ--GK_hpUPYIt8tx__luZOhjFERRYOjBAyCRhdvO0MXGQ75ea6x5lbZleOC-fcouSqiYJd-a6oKOa_MtKC15Tx2g?testcase_id=5188057160744960

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.

ClusterFuzz testcase is verified as fixed, closing issue.

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

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

commit fd60fc3c2c5ebc47c9fe7e1bbb7fe1bb6dc19ae1
Author: svillar <svillar@igalia.com>
Date: Thu Aug 25 15:51:42 2016

[css-grid] Do not recursively call layout during auto repeat computation

The computation of auto repeat tracks was incorrectly recursively triggering
a layout in order to compute the available size. That was happening whenever
the width was indefinite. In such cases we should treat the width always as
indefinite without having to run any extra code. During the layout phase
we'll have the actual width available.

This partially reverts commit 49bc5ba13c492f09971882e2cb842e3514d19922 which
was a quick fix for a security issue but that was not actually fixing the
real problem behind.

BUG= 633474 

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

[modify] https://crrev.com/fd60fc3c2c5ebc47c9fe7e1bbb7fe1bb6dc19ae1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/fd60fc3c2c5ebc47c9fe7e1bbb7fe1bb6dc19ae1/third_party/WebKit/Source/core/layout/LayoutGrid.h

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

commit 90ea1ed689536a8500f9d92652b2a4a0baa24db6
Author: svillar <svillar@igalia.com>
Date: Tue Oct 18 14:44:12 2016

[css-grid] Remove a duplicated auto repeat computation for intrinsic sizes

After crrev.com/414446 we can safely remove the temporary fix
(crrev.com/405529) we had to avoid crashes and recursive calls when
computing the auto repeat tracks count for grids with intrinsic sizes.

This change unveiled a couple of bugs in the computation of auto repeat
tracks for intrinsic sizes. The first one is that we were not considering the
existence of definite minimum sizes. In those cases, instead of just returning 1
repetition, we should return the minimum number of repetitions that fulfill the
minimum size requirement.

The second bug was that grids were not properly recomputing the number of
auto repeat tracks when the min|max-content contributions of grid items
changed (whenever the grid container had an intrinsic width).

One Mozilla test had to be updated too because it was wrong.
BUG= 621517 , 633474 

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

[add] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-intrinsic.html
[add] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks-expected.txt
[add] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks.html
[modify] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001-expected.html
[modify] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/Source/core/layout/LayoutGrid.h

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

commit b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64
Author: rego <rego@igalia.com>
Date: Fri Nov 18 12:31:52 2016

Revert of [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes (patchset #4 id:60001 of https://codereview.chromium.org/2287533002/ )

Reason for revert: Caused crash on Mac. See http://crbug.com/662016

We're reverting only the code change as we want to keep the tests,
that will be fixed after the refactoring happening in  bug #627812 
is done.
So, we need to mark 2 tests as failure in TestExpectations file.

BUG=662016, 666688 , 627812 

Original issue's description:
> [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
>
> After crrev.com/414446 we can safely remove the temporary fix
> (crrev.com/405529) we had to avoid crashes and recursive calls when
> computing the auto repeat tracks count for grids with intrinsic sizes.
>
> This change unveiled a couple of bugs in the computation of auto repeat
> tracks for intrinsic sizes. The first one is that we were not considering the
> existence of definite minimum sizes. In those cases, instead of just returning 1
> repetition, we should return the minimum number of repetitions that fulfill the
> minimum size requirement.
>
> The second bug was that grids were not properly recomputing the number of
> auto repeat tracks when the min|max-content contributions of grid items
> changed (whenever the grid container had an intrinsic width).
>
> One Mozilla test had to be updated too because it was wrong.
> BUG= 621517 , 633474 
>
> Review-Url: https://codereview.chromium.org/2287533002
> Cr-Commit-Position: refs/heads/master@{#425958}

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

[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.h

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

commit ab661a19616ff4cd7ff522cef71354971c4d5694
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Mon Nov 21 11:38:41 2016

Revert of [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes (patchset #4 id:60001 of https://codereview.chromium.org/2287533002/ )

Reason for revert: Caused crash on Mac. See http://crbug.com/662016

We're reverting only the code change as we want to keep the tests,
that will be fixed after the refactoring happening in  bug #627812 
is done.
So, we need to mark 2 tests as failure in TestExpectations file.

BUG=662016, 666688 , 627812 

Original issue's description:
> [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
>
> After crrev.com/414446 we can safely remove the temporary fix
> (crrev.com/405529) we had to avoid crashes and recursive calls when
> computing the auto repeat tracks count for grids with intrinsic sizes.
>
> This change unveiled a couple of bugs in the computation of auto repeat
> tracks for intrinsic sizes. The first one is that we were not considering the
> existence of definite minimum sizes. In those cases, instead of just returning 1
> repetition, we should return the minimum number of repetitions that fulfill the
> minimum size requirement.
>
> The second bug was that grids were not properly recomputing the number of
> auto repeat tracks when the min|max-content contributions of grid items
> changed (whenever the grid container had an intrinsic width).
>
> One Mozilla test had to be updated too because it was wrong.
> BUG= 621517 , 633474 
>
> Review-Url: https://codereview.chromium.org/2287533002
> Cr-Commit-Position: refs/heads/master@{#425958}

Review-Url: https://codereview.chromium.org/2510393002
Cr-Commit-Position: refs/heads/master@{#433183}
(cherry picked from commit b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64)

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

Cr-Commit-Position: refs/branch-heads/2924@{#24}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.h

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