Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514323003:
Fix UaF in RenderFrameImpl::OnBeforeUnload. (Closed)
|
Created:
4 years, 1 month ago by lfg Modified:
4 years ago Reviewers:
Charlie Reis CC:
chromium-reviews, nasko+codewatch_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix UaF in RenderFrameImpl::OnBeforeUnload.
BUG=666714
Committed: https://crrev.com/0dd441a0007aa46917779e782ee9094f111a02b3
Cr-Commit-Position: refs/heads/master@{#434226}
Patch Set 1 #
Total comments: 1
Patch Set 2 : add test #Patch Set 3 : fix linux build #
Total comments: 3
Patch Set 4 : add comments #
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lfg@chromium.org changed reviewers: + creis@chromium.org
Charlie, please take a look.
[CC dcheng] Thanks-- I'm ok with this as a quick defense for this case, but it does seem imperfect in general. I wonder if we should be preventing print() during beforeunload/unload, given the nested message loop... Also, is this something we can write a test for, by putting print() in a beforeunload() and closing the tab while another tab in the same process is open? I see we have things like TestJavaScriptDialogManager in web_contents_impl_browsertest.cc which might be able to help (or WaitForAppModalDialog?). The reason I'd like a test is that the memory bots may help us catch regressions if we add a reference to |this| after the dispatchBeforeUnload call in the future. https://codereview.chromium.org/2514323003/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2514323003/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1722: RenderThread::Get()->Send(new FrameHostMsg_BeforeUnload_ACK( Interesting. I guess that's what RenderFrameImpl::Send does anyway-- there's no filtering going on in this class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/21 23:17:14, Charlie Reis (slow) wrote: > [CC dcheng] > > Thanks-- I'm ok with this as a quick defense for this case, but it does seem > imperfect in general. I wonder if we should be preventing print() during > beforeunload/unload, given the nested message loop... > > Also, is this something we can write a test for, by putting print() in a > beforeunload() and closing the tab while another tab in the same process is > open? I see we have things like TestJavaScriptDialogManager in > web_contents_impl_browsertest.cc which might be able to help (or > WaitForAppModalDialog?). > > The reason I'd like a test is that the memory bots may help us catch regressions > if we add a reference to |this| after the dispatchBeforeUnload call in the > future. I think we can't write a test by relying on print(), the nested message loop runs while a sync IPC is happening, it's very timing-sensitive. But I wrote a RenderViewImplTest that almost works, the problem is that destroying the frame causes issues on test teardown. I'll try to move it to RenderFrameImplTest tomorrow, if print() works on an iframe, I think the test should work.
On 2016/11/21 23:17:14, Charlie Reis (slow) wrote: > [CC dcheng] > > Thanks-- I'm ok with this as a quick defense for this case, but it does seem > imperfect in general. I wonder if we should be preventing print() during > beforeunload/unload, given the nested message loop... We should, but it's still possible to end up in a nested message loop with a sync XHR. > > Also, is this something we can write a test for, by putting print() in a > beforeunload() and closing the tab while another tab in the same process is > open? I see we have things like TestJavaScriptDialogManager in > web_contents_impl_browsertest.cc which might be able to help (or > WaitForAppModalDialog?). > > The reason I'd like a test is that the memory bots may help us catch regressions > if we add a reference to |this| after the dispatchBeforeUnload call in the > future. > > https://codereview.chromium.org/2514323003/diff/1/content/renderer/render_fra... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2514323003/diff/1/content/renderer/render_fra... > content/renderer/render_frame_impl.cc:1722: RenderThread::Get()->Send(new > FrameHostMsg_BeforeUnload_ACK( > Interesting. I guess that's what RenderFrameImpl::Send does anyway-- there's no > filtering going on in this class.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Charlie, please take another look. I added a test that simulates a swap inside the beforeunload handler. The test crashes the same way as the original bug crashes without the changes in RenderFrameImpl.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the test! LGTM. (We'll need a separate fix for 666616, right? Lets follow up there.) https://codereview.chromium.org/2514323003/diff/40001/content/renderer/render... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/2514323003/diff/40001/content/renderer/render... content/renderer/render_view_browsertest.cc:2221: std::unique_ptr<ConsoleCallbackFilter> callback_filter( Nice. Let's add a comment explaining what this is doing.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2514323003/diff/40001/content/renderer/render... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/2514323003/diff/40001/content/renderer/render... content/renderer/render_view_browsertest.cc:2221: std::unique_ptr<ConsoleCallbackFilter> callback_filter( On 2016/11/23 07:07:00, Charlie Reis (slow) wrote: > Nice. Let's add a comment explaining what this is doing. Done. Can you do a quick sanity check to make sure the comments make sense?
https://codereview.chromium.org/2514323003/diff/40001/content/renderer/render... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/2514323003/diff/40001/content/renderer/render... content/renderer/render_view_browsertest.cc:2221: std::unique_ptr<ConsoleCallbackFilter> callback_filter( On 2016/11/23 16:48:02, lfg wrote: > On 2016/11/23 07:07:00, Charlie Reis (slow) wrote: > > Nice. Let's add a comment explaining what this is doing. > > Done. Can you do a quick sanity check to make sure the comments make sense? Thanks-- looks good.
The CQ bit was unchecked by lfg@chromium.org
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2514323003/#ps60001 (title: "add comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix UaF in RenderFrameImpl::OnBeforeUnload. BUG=666714 ========== to ========== Fix UaF in RenderFrameImpl::OnBeforeUnload. BUG=666714 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479924673811930,
"parent_rev": "5c5d3618024fdbf8394bd81fb9258ef118c1f14c", "commit_rev":
"4416ddbcc32f648229262b13a2c5342142e35ffe"}
Message was sent while issue was closed.
Description was changed from ========== Fix UaF in RenderFrameImpl::OnBeforeUnload. BUG=666714 ========== to ========== Fix UaF in RenderFrameImpl::OnBeforeUnload. BUG=666714 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix UaF in RenderFrameImpl::OnBeforeUnload. BUG=666714 ========== to ========== Fix UaF in RenderFrameImpl::OnBeforeUnload. BUG=666714 Committed: https://crrev.com/0dd441a0007aa46917779e782ee9094f111a02b3 Cr-Commit-Position: refs/heads/master@{#434226} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0dd441a0007aa46917779e782ee9094f111a02b3 Cr-Commit-Position: refs/heads/master@{#434226} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
