Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276053002:
Fix use-after-free in the renderer process. (Closed)
|
Created:
4 years, 4 months ago by lfg Modified:
4 years, 4 months 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix use-after-free in the renderer process.
If a frame that contains the focused plugin is destroyed,
we leave a dangling pointer in the RenderWidget.
BUG=640733
Committed: https://crrev.com/4eb5bd96518311cc565323e59f08c6cfc140dcae
Cr-Commit-Position: refs/heads/master@{#414176}
Patch Set 1 #Patch Set 2 : adding ifdef #
Total comments: 1
Messages
Total messages: 25 (14 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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.
Description was changed from ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=none ========== to ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=none ==========
Thanks for catching this. I assume we don't have repro steps and you just found it in the code? It's probably still worth filing a bug with a description of the problem, which is useful for source history or if it needs to be reverted. One question about the fix below. https://codereview.chromium.org/2276053002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2276053002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2833: GetRenderWidget()->set_focused_pepper_plugin(nullptr); Looks like there's a focused_pepper_plugin_ on both RenderFrameImpl and RenderWidget? I'm not sure why we would check one and clear the other. Shouldn't we be checking and clearing the same one, or keeping them in sync?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=none ========== to ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=640733 ==========
On 2016/08/24 20:38:44, Charlie Reis (OOO til 8-30) wrote: > Thanks for catching this. > > I assume we don't have repro steps and you just found it in the code? It's > probably still worth filing a bug with a description of the problem, which is > useful for source history or if it needs to be reverted. No, I have repro steps for this (though I relied on an external example of a swf file that executes javascript, so I can't create a test out of this without being able to create a new swf file). > One question about the fix below. > > https://codereview.chromium.org/2276053002/diff/20001/content/renderer/render... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2276053002/diff/20001/content/renderer/render... > content/renderer/render_frame_impl.cc:2833: > GetRenderWidget()->set_focused_pepper_plugin(nullptr); > Looks like there's a focused_pepper_plugin_ on both RenderFrameImpl and > RenderWidget? RenderWidget::focused_pepper_plugin_ is sort of a cache of the last focused plugin in frames that are in the local root rendered by that widget. > I'm not sure why we would check one and clear the other. Shouldn't we be > checking and clearing the same one, or keeping them in sync? frameDetached destroys the RenderFrameImpl, so it doesn't matter if we clear the same. Keeping it in sync is also a bit of a grey area, since we can have multiple RenderFrameImpls in that RenderWidget, so even if the plugin is null in one RenderFrame, it can be non-null in another RenderFrame. I think we should redesign how IME works here. I don't think it makes sense to have RenderWidget having knowledge of plugins. I believe I spoke with ekaramad@ about this, and I think it was done this way because RenderWidget doesn't have access to RenderFrame. We should probably look into either relaxing that, or moving the IME implementation into RenderFrame, or maybe partially to RenderFrame. In any case it'll require quite a bit of new design work. For now, I'm hoping this should be an acceptable fix/compromise in the short term.
On 2016/08/24 21:05:41, lfg wrote: > On 2016/08/24 20:38:44, Charlie Reis (OOO til 8-30) wrote: > > Thanks for catching this. > > > > I assume we don't have repro steps and you just found it in the code? It's > > probably still worth filing a bug with a description of the problem, which is > > useful for source history or if it needs to be reverted. > > No, I have repro steps for this (though I relied on an external example of > a swf file that executes javascript, so I can't create a test out of this > without being able to create a new swf file). Forgot to mention the link to the bug: https://crbug.com/640733
On 2016/08/24 21:05:41, lfg wrote: > On 2016/08/24 20:38:44, Charlie Reis (OOO til 8-30) wrote: > > Thanks for catching this. > > > > I assume we don't have repro steps and you just found it in the code? It's > > probably still worth filing a bug with a description of the problem, which is > > useful for source history or if it needs to be reverted. > > No, I have repro steps for this (though I relied on an external example of > a swf file that executes javascript, so I can't create a test out of this > without being able to create a new swf file). Ok. Thanks for filing the bug. > > > One question about the fix below. > > > > > https://codereview.chromium.org/2276053002/diff/20001/content/renderer/render... > > File content/renderer/render_frame_impl.cc (right): > > > > > https://codereview.chromium.org/2276053002/diff/20001/content/renderer/render... > > content/renderer/render_frame_impl.cc:2833: > > GetRenderWidget()->set_focused_pepper_plugin(nullptr); > > Looks like there's a focused_pepper_plugin_ on both RenderFrameImpl and > > RenderWidget? > > RenderWidget::focused_pepper_plugin_ is sort of a cache of the last focused > plugin in frames that are in the local root rendered by that widget. > > > I'm not sure why we would check one and clear the other. Shouldn't we be > > checking and clearing the same one, or keeping them in sync? > > frameDetached destroys the RenderFrameImpl, so it doesn't matter if we clear > the same. Keeping it in sync is also a bit of a grey area, since we can have > multiple RenderFrameImpls in that RenderWidget, so even if the plugin is null > in one RenderFrame, it can be non-null in another RenderFrame. Is there any chance the RenderWidget could have a different value than this detaching RenderFrame? This still feels a bit fragile to check one value and assume that means something about the other. (For example, I could imagine comparing focused_pepper_plugin_ with RenderWidget's value before clearing it, though apparently we don't expose RenderWidget's value.) If a mismatch here isn't possible, then I'm ok landing this as is. LGTM in that case. > > I think we should redesign how IME works here. I don't think it makes > sense to have RenderWidget having knowledge of plugins. I believe I spoke > with ekaramad@ about this, and I think it was done this way because > RenderWidget doesn't have access to RenderFrame. We should probably look > into either relaxing that, or moving the IME implementation into RenderFrame, > or maybe partially to RenderFrame. In any case it'll require quite a bit > of new design work. > > For now, I'm hoping this should be an acceptable fix/compromise in the short > term. Yes, let's continue discussing this with Ehsan after this CL lands.
On 2016/08/24 21:18:26, Charlie Reis (OOO til 8-30) wrote: > Is there any chance the RenderWidget could have a different value than this > detaching RenderFrame? This still feels a bit fragile to check one value and > assume that means something about the other. (For example, I could imagine > comparing focused_pepper_plugin_ with RenderWidget's value before clearing it, > though apparently we don't expose RenderWidget's value.) There are no strong guarantees on content/ that this can't happen. But I think blink should guarantee that there's only a single focused element, which should enforce this indirectly, since this is always local within the renderer. Going forward, we should probably add this at least as a DCHECK on the content/ side. > If a mismatch here isn't possible, then I'm ok landing this as is. LGTM in that > case. I don't think this is possible; though if it is, it already happens regardless of this change: see https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=....
On 2016/08/24 21:26:37, lfg wrote: > On 2016/08/24 21:18:26, Charlie Reis (OOO til 8-30) wrote: > > Is there any chance the RenderWidget could have a different value than this > > detaching RenderFrame? This still feels a bit fragile to check one value and > > assume that means something about the other. (For example, I could imagine > > comparing focused_pepper_plugin_ with RenderWidget's value before clearing it, > > though apparently we don't expose RenderWidget's value.) > > There are no strong guarantees on content/ that this can't happen. But I think > blink should guarantee that there's only a single focused element, which should > enforce this indirectly, since this is always local within the renderer. Going > forward, we should probably add this at least as a DCHECK on the content/ side. > > > If a mismatch here isn't possible, then I'm ok landing this as is. LGTM in > that > > case. > > I don't think this is possible; though if it is, it already happens regardless > of > this change: see > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=.... Thanks-- that block clarifies things. Seems like we'll keep them in sync, so I'm ok with this.
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/24 22:19:05, Charlie Reis (OOO til 8-30) wrote: > On 2016/08/24 21:26:37, lfg wrote: > > On 2016/08/24 21:18:26, Charlie Reis (OOO til 8-30) wrote: > > > Is there any chance the RenderWidget could have a different value than this > > > detaching RenderFrame? This still feels a bit fragile to check one value > and > > > assume that means something about the other. (For example, I could imagine > > > comparing focused_pepper_plugin_ with RenderWidget's value before clearing > it, > > > though apparently we don't expose RenderWidget's value.) > > > > There are no strong guarantees on content/ that this can't happen. But I think > > blink should guarantee that there's only a single focused element, which > should > > enforce this indirectly, since this is always local within the renderer. Going > > forward, we should probably add this at least as a DCHECK on the content/ > side. > > > > > If a mismatch here isn't possible, then I'm ok landing this as is. LGTM in > > that > > > case. > > > > I don't think this is possible; though if it is, it already happens regardless > > of > > this change: see > > > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=.... > > Thanks-- that block clarifies things. Seems like we'll keep them in sync, so > I'm ok with this. Thanks Lucas for finding this. I think, if worst comes to worst and two frames set the focused plugin, we will just be out of sync with TextInputState and will not crash. I could not track the code inside RenderFrameImpl and PepperPluginInstanceImpl to figure this out but the IME of it seems a bit strange to me as it detours blink sometimes. As for the redesign, I do not think it is a good idea to move IME logic to RenderFrame. But I also am not opposed to the idea of not having plugin knowledge in RenderWidget. Except for IME, I don't think there is any other reasons for keeping it there. The way we use the reference to plugin in RenderWidget is to 1) Get IME state to send to browser, and 2) forward IPCs to the plugin. Ideally, if we had knowledge of the RWH for plugin in the browser process and specifically on the frame tree (not sure if there is RWH btw), then we could directly route IPC and obtain state. But this is not the case here. I think the best solution would be to route everything through blink. This will trim the code in content/ for better readability in general. All IME calls end up in WebWidget and use focusedFrame eventually. So we might as well get the plugin for the focusedFrame, if any, and get state/route calls to that plugin. There is probably some exceptions/issues here otherwise somebody would have done it already (maybe?). Maybe it is not possible to obtain the PepperWebPlugin from inside blink. I will take another look at this issue when I start refactoring IME in blink.
Message was sent while issue was closed.
Description was changed from ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=640733 ========== to ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=640733 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=640733 ========== to ========== Fix use-after-free in the renderer process. If a frame that contains the focused plugin is destroyed, we leave a dangling pointer in the RenderWidget. BUG=640733 Committed: https://crrev.com/4eb5bd96518311cc565323e59f08c6cfc140dcae Cr-Commit-Position: refs/heads/master@{#414176} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4eb5bd96518311cc565323e59f08c6cfc140dcae Cr-Commit-Position: refs/heads/master@{#414176} |
