Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2411333007:
[css-grid] Avoid storing pointers to orthogonal grid items (Closed)
|
Created:
4 years, 2 months ago by jfernandez Modified:
4 years, 2 months ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Avoid storing pointers to orthogonal grid items
We defined a Vector of LayoutBox pointers to the orthogonal grid items.
This vector has been used mainly to clear the override sizes when the
grid layout logic is executed several times for the same Layout Tree.
The problem, detected in the test cases attached in the bug report, is
that those pointers may point to invalid memory regions. This lead to
heap-use-after-free crashes which are potential security breaches.
This patch removes the Vector so we iterate over the children got
from the current Layout Tree structure at the time the layout logic is
executed. This is a less efficient approach, but definitively safer
because we avoid accessing pointers to invalid memory regions.
BUG=655632
Committed: https://crrev.com/738e26e1286554cb9a63610bce2b77da5a912eb3
Cr-Commit-Position: refs/heads/master@{#425665}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Patch for landing. #
Total comments: 4
Messages
Total messages: 23 (9 generated)
Description was changed from ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has used mainly to cleare the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the big report, is that those pointes may point to invalid memory regions. This lead to heap-use-after-free crashes which are pontential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is less a less efficient approach, but definitively safer because we avoid accesing pointers to invalid memory regions. BUG=655632 ========== to ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has used mainly to cleare the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the big report, is that those pointes may point to invalid memory regions. This lead to heap-use-after-free crashes which are pontential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is less a less efficient approach, but definitively safer because we avoid accesing pointers to invalid memory regions. BUG=655632 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com
Description was changed from ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has used mainly to cleare the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the big report, is that those pointes may point to invalid memory regions. This lead to heap-use-after-free crashes which are pontential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is less a less efficient approach, but definitively safer because we avoid accesing pointers to invalid memory regions. BUG=655632 ========== to ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has used mainly to clear the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the big report, is that those pointers may point to invalid memory regions. This lead to heap-use-after-free crashes which are potential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is less a less efficient approach, but definitively safer because we avoid accessing pointers to invalid memory regions. BUG=655632 ==========
Definitely safer indeed lgtm BTW could you rename the test to something more self-explicative? Otherwise we would need to go to the bug first in order to know what is it about. https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html (right): https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:10: function domfuzz_crawlNode(node) { Nit: weird function name, likely coming from the fuzzer. https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:15: } catch (e) { I guess you can remove the try-catch as you are not doing anything in case of exception https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:495: for (LayoutBox* child = firstChildBox(); child; for (auto* child
A few typos in the description: "has used mainly", "the big report", "less a less".
LGTM too. https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html (right): https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:9: <script> Nit: You don't need to close the <script> tag and open it again in the next line. https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:30: <div id="node" class="firstRowFirstColumn horizontalTB"></div> Nit: I guess all those classes are needed to reproduced it. Maybe we don't need "fit-content" or "firstRowFirstColumn", dunno really.
Description was changed from ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has used mainly to clear the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the big report, is that those pointers may point to invalid memory regions. This lead to heap-use-after-free crashes which are potential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is less a less efficient approach, but definitively safer because we avoid accessing pointers to invalid memory regions. BUG=655632 ========== to ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has been used mainly to clear the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the bug report, is that those pointers may point to invalid memory regions. This lead to heap-use-after-free crashes which are potential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is a less efficient approach, but definitively safer because we avoid accessing pointers to invalid memory regions. BUG=655632 ==========
https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html (right): https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:9: <script> On 2016/10/17 09:13:41, Manuel Rego wrote: > Nit: You don't need to close the <script> tag > and open it again in the next line. Acknowledged. https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:10: function domfuzz_crawlNode(node) { On 2016/10/17 08:28:19, svillar wrote: > Nit: weird function name, likely coming from the fuzzer. Acknowledged. https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:15: } catch (e) { On 2016/10/17 08:28:19, svillar wrote: > I guess you can remove the try-catch as you are not doing anything in case of > exception Acknowledged. https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:30: <div id="node" class="firstRowFirstColumn horizontalTB"></div> On 2016/10/17 09:13:41, Manuel Rego wrote: > Nit: I guess all those classes are needed to reproduced it. > Maybe we don't need "fit-content" or "firstRowFirstColumn", > dunno really. We indeed need fit-content, because we only clear the override sizes when it's necessary to recompute the grid's intrinsic size. We can remove the firstRowFirstColumn class, though.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com, rego@igalia.com Link to the patchset: https://codereview.chromium.org/2411333007/#ps40001 (title: "Patch for landing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:497: if (child->isOutOfFlowPositioned() || !isOrthogonalChild(*child)) Since you use nextInFlowSiblingBox, do you need the isOutOfFlowPositioned check?
https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html (right): https://codereview.chromium.org/2411333007/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/regression-test-for-issue-655632.html:15: } catch (e) { On 2016/10/17 08:28:19, svillar wrote: > I guess you can remove the try-catch as you are not doing anything in case of > exception Actually we can't remove the try-catch, since some properties don't allow null as value, so without catching the exception, the script is not completed. https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:497: if (child->isOutOfFlowPositioned() || !isOrthogonalChild(*child)) On 2016/10/17 12:16:45, cbiesinger wrote: > Since you use nextInFlowSiblingBox, do you need the isOutOfFlowPositioned check? You're right. I thought the same, but since we were already using it in our grid item positioned logic, I blindly trusted our previous implementation. What I'm going to do is to do a patch to fix both cases.
https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:497: if (child->isOutOfFlowPositioned() || !isOrthogonalChild(*child)) Note that we need to check that firstChildBox() is not out of flow. So or we implement a firstInFlowChildBox() or we probably simply use nextSiblingBox() and keep the if here.
https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:497: if (child->isOutOfFlowPositioned() || !isOrthogonalChild(*child)) On 2016/10/17 12:40:42, Manuel Rego wrote: > Note that we need to check that firstChildBox() is not out of flow. > So or we implement a firstInFlowChildBox() or we probably simply use > nextSiblingBox() and keep the if here. That's true. I think we should change both and just use nextSiblingBox instead.
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has been used mainly to clear the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the bug report, is that those pointers may point to invalid memory regions. This lead to heap-use-after-free crashes which are potential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is a less efficient approach, but definitively safer because we avoid accessing pointers to invalid memory regions. BUG=655632 ========== to ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has been used mainly to clear the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the bug report, is that those pointers may point to invalid memory regions. This lead to heap-use-after-free crashes which are potential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is a less efficient approach, but definitively safer because we avoid accessing pointers to invalid memory regions. BUG=655632 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has been used mainly to clear the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the bug report, is that those pointers may point to invalid memory regions. This lead to heap-use-after-free crashes which are potential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is a less efficient approach, but definitively safer because we avoid accessing pointers to invalid memory regions. BUG=655632 ========== to ========== [css-grid] Avoid storing pointers to orthogonal grid items We defined a Vector of LayoutBox pointers to the orthogonal grid items. This vector has been used mainly to clear the override sizes when the grid layout logic is executed several times for the same Layout Tree. The problem, detected in the test cases attached in the bug report, is that those pointers may point to invalid memory regions. This lead to heap-use-after-free crashes which are potential security breaches. This patch removes the Vector so we iterate over the children got from the current Layout Tree structure at the time the layout logic is executed. This is a less efficient approach, but definitively safer because we avoid accessing pointers to invalid memory regions. BUG=655632 Committed: https://crrev.com/738e26e1286554cb9a63610bce2b77da5a912eb3 Cr-Commit-Position: refs/heads/master@{#425665} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/738e26e1286554cb9a63610bce2b77da5a912eb3 Cr-Commit-Position: refs/heads/master@{#425665}
Message was sent while issue was closed.
Oh, great point, thanks! lgtm either way. On Mon, Oct 17, 2016 at 8:43 AM, <jfernandez@igalia.com> wrote: > > https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): > > https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:497: if > (child->isOutOfFlowPositioned() || !isOrthogonalChild(*child)) > On 2016/10/17 12:40:42, Manuel Rego wrote: >> Note that we need to check that firstChildBox() is not out of flow. >> So or we implement a firstInFlowChildBox() or we probably simply use >> nextSiblingBox() and keep the if here. > > That's true. I think we should change both and just use nextSiblingBox > instead. > > https://codereview.chromium.org/2411333007/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Oh, great point, thanks! lgtm either way. On Mon, Oct 17, 2016 at 8:43 AM, <jfernandez@igalia.com> wrote: > > https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): > > https://codereview.chromium.org/2411333007/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:497: if > (child->isOutOfFlowPositioned() || !isOrthogonalChild(*child)) > On 2016/10/17 12:40:42, Manuel Rego wrote: >> Note that we need to check that firstChildBox() is not out of flow. >> So or we implement a firstInFlowChildBox() or we probably simply use >> nextSiblingBox() and keep the if here. > > That's true. I think we should change both and just use nextSiblingBox > instead. > > https://codereview.chromium.org/2411333007/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
