Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(452)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 19558006: Heap-use-after-free in WebCore::RenderFlexibleBox::firstLineBoxBaseline (Closed)

Created:
7 years, 5 months ago by Julien - ping for review
Modified:
7 years, 5 months ago
Reviewers:
cbiesinger, inferno, ojan
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering
Visibility:
Public.

Description

Heap-use-after-free in WebCore::RenderFlexibleBox::firstLineBoxBaseline This is a regression from introducing the OrderIterator. The issue is that we don't invalidate our iterator when a child is removed. This means that we could hold onto free'd memory until the next layout when we will recompute the iterator. The solution is simple: just clear the memory when we remove a child. Note that RenderGrid is not impacted by this bug as we don't use the iterator outside layout but as I intent to change that, the same treatment was applied to the class. BUG=261891 TEST=fast/flexbox/order-iterator-crash.html R=inferno@chromium.org, ojan@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154582

Patch Set 1 #

Patch Set 2 : Removed bad FINAL #

Total comments: 11

Patch Set 3 : Updated change after the review's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -4 lines) Patch
A LayoutTests/fast/flexbox/order-iterator-crash.html View 1 chunk +12 lines, -0 lines 0 comments Download
A LayoutTests/fast/flexbox/order-iterator-crash-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/rendering/OrderIterator.h View 2 chunks +3 lines, -4 lines 0 comments Download
M Source/core/rendering/OrderIterator.cpp View 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderFlexibleBox.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderFlexibleBox.cpp View 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderGrid.h View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderGrid.cpp View 1 2 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Julien - ping for review
The bots are really sad at the moment but I think the change is fine.
7 years, 5 months ago (2013-07-19 20:08:29 UTC) #1
inferno
lgtm https://codereview.chromium.org/19558006/diff/3001/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt File LayoutTests/fast/flexbox/order-iterator-crash-expected.txt (right): https://codereview.chromium.org/19558006/diff/3001/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt#newcode2 LayoutTests/fast/flexbox/order-iterator-crash-expected.txt:2: * { display: flex; } Why are these ...
7 years, 5 months ago (2013-07-19 20:25:07 UTC) #2
ojan
lgtm https://codereview.chromium.org/19558006/diff/3001/Source/core/rendering/OrderIterator.cpp File Source/core/rendering/OrderIterator.cpp (right): https://codereview.chromium.org/19558006/diff/3001/Source/core/rendering/OrderIterator.cpp#newcode82 Source/core/rendering/OrderIterator.cpp:82: m_orderedValues.clear(); Nit: can't we also do m_orderedValues.shrink(0)? https://codereview.chromium.org/19558006/diff/3001/Source/core/rendering/RenderGrid.cpp ...
7 years, 5 months ago (2013-07-19 20:27:49 UTC) #3
cbiesinger
wait, does this mean the iterator is invalid until the next layout? Is that a ...
7 years, 5 months ago (2013-07-19 20:30:30 UTC) #4
Julien - ping for review
> wait, does this mean the iterator is invalid until the next layout? You are ...
7 years, 5 months ago (2013-07-19 20:52:31 UTC) #5
inferno
https://codereview.chromium.org/19558006/diff/3001/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt File LayoutTests/fast/flexbox/order-iterator-crash-expected.txt (right): https://codereview.chromium.org/19558006/diff/3001/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt#newcode2 LayoutTests/fast/flexbox/order-iterator-crash-expected.txt:2: * { display: flex; } On 2013/07/19 20:52:31, Julien ...
7 years, 5 months ago (2013-07-19 20:57:22 UTC) #6
Julien - ping for review
https://codereview.chromium.org/19558006/diff/3001/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt File LayoutTests/fast/flexbox/order-iterator-crash-expected.txt (right): https://codereview.chromium.org/19558006/diff/3001/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt#newcode2 LayoutTests/fast/flexbox/order-iterator-crash-expected.txt:2: * { display: flex; } On 2013/07/19 20:57:22, inferno ...
7 years, 5 months ago (2013-07-19 21:05:45 UTC) #7
Julien - ping for review
https://codereview.chromium.org/19558006/diff/3001/Source/core/rendering/OrderIterator.cpp File Source/core/rendering/OrderIterator.cpp (right): https://codereview.chromium.org/19558006/diff/3001/Source/core/rendering/OrderIterator.cpp#newcode82 Source/core/rendering/OrderIterator.cpp:82: m_orderedValues.clear(); On 2013/07/19 20:52:31, Julien Chaffraix wrote: > On ...
7 years, 5 months ago (2013-07-19 21:25:44 UTC) #8
Julien - ping for review
7 years, 5 months ago (2013-07-19 21:27:00 UTC) #9
Message was sent while issue was closed. 
Committed patchset #3 manually as r154582 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698