Adds a check to ensure that a NULL pointer from RenderViewHostImpl::FromID() is not dereferenced

Adds a check to ensure that a NULL pointer from RenderViewHostImpl::FromID() is not dereferenced.

CID=103978
BUG=139242
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148718

[Tyler Breisacher (Chromium), 2012-07-26 18:19:09]

Building off what jhawkins said, the point of DCHECKs is to make the code
"self-documenting" -- in this case, to document the fact that 'opener' should
never be null. But we can already see that 'opener' should never be null because
we dereference it on the very next line, so this doesn't really add anything
IMO.

What Coverity is saying is that FromID does return null sometimes, so when
you're calling it, you should always check whether it returned null and figure
out what should be done if it is null. Or, if we're sure that it can never
return null in this case, then just mark it as intentional in Coverity, possibly
with a comment saying "// owner can't be null because ..."

[Kyle Horimoto, 2012-07-26 18:27:32]

+creis (who wrote the code in question)

creis, we used the static code analysis tool Coverity, and it discovered this as
a possible case where we may be dereferencing a NULL pointer. Is this an
appropriate place to put a DCHECK()?

[Charlie Reis, 2012-07-26 19:13:06]

On 2012/07/26 18:27:32, Kyle Horimoto wrote:
> +creis (who wrote the code in question)
> 
> creis, we used the static code analysis tool Coverity, and it discovered this
as
> a possible case where we may be dereferencing a NULL pointer. Is this an
> appropriate place to put a DCHECK()?

As Tyler mentioned, the DCHECK would be redundant here, since we would safely
crash on the next line if it failed anyway.

Are you aware of a case where the opener pointer can be null?  That would be a
bug that we need to fix.  Otherwise it seems like suppressing the warning would
be the right thing to do.

Now that I look at it, I do wonder if it's possible for opener to be null.  If
the opener tab is different than the current tab and the IPC message arrived
after the browser process closed the opener tab, then FromID would return null. 
Ditto if the renderer process has been exploited and sends garbage in the IPC
message.

Maybe it's worth putting a null check instead of a DCHECK.

[Kyle Horimoto, 2012-07-26 23:02:20]

On 2012/07/26 19:13:06, creis wrote:
> On 2012/07/26 18:27:32, Kyle Horimoto wrote:
> > +creis (who wrote the code in question)
> > 
> > creis, we used the static code analysis tool Coverity, and it discovered
this
> as
> > a possible case where we may be dereferencing a NULL pointer. Is this an
> > appropriate place to put a DCHECK()?
> 
> As Tyler mentioned, the DCHECK would be redundant here, since we would safely
> crash on the next line if it failed anyway.
> 
> Are you aware of a case where the opener pointer can be null?  That would be a
> bug that we need to fix.  Otherwise it seems like suppressing the warning
would
> be the right thing to do.
> 
> Now that I look at it, I do wonder if it's possible for opener to be null.  If
> the opener tab is different than the current tab and the IPC message arrived
> after the browser process closed the opener tab, then FromID would return
null. 
> Ditto if the renderer process has been exploited and sends garbage in the IPC
> message.
> 
> Maybe it's worth putting a null check instead of a DCHECK.

I'm not really too sure about this code as I haven't ever worked on it - I only
worked on this bug because the code analysis tool caught it. You'd definitely be
more knowledgeable than I am. Would you suggest that I replace my DCHECK() with
"if(!opener) return;"?

[Charlie Reis, 2012-07-26 23:13:40]

On 2012/07/26 23:02:20, Kyle Horimoto wrote:
> On 2012/07/26 19:13:06, creis wrote:
> > On 2012/07/26 18:27:32, Kyle Horimoto wrote:
> > > +creis (who wrote the code in question)
> > > 
> > > creis, we used the static code analysis tool Coverity, and it discovered
> this
> > as
> > > a possible case where we may be dereferencing a NULL pointer. Is this an
> > > appropriate place to put a DCHECK()?
> > 
> > As Tyler mentioned, the DCHECK would be redundant here, since we would
safely
> > crash on the next line if it failed anyway.
> > 
> > Are you aware of a case where the opener pointer can be null?  That would be
a
> > bug that we need to fix.  Otherwise it seems like suppressing the warning
> would
> > be the right thing to do.
> > 
> > Now that I look at it, I do wonder if it's possible for opener to be null. 
If
> > the opener tab is different than the current tab and the IPC message arrived
> > after the browser process closed the opener tab, then FromID would return
> null. 
> > Ditto if the renderer process has been exploited and sends garbage in the
IPC
> > message.
> > 
> > Maybe it's worth putting a null check instead of a DCHECK.
> 
> I'm not really too sure about this code as I haven't ever worked on it - I
only
> worked on this bug because the code analysis tool caught it. You'd definitely
be
> more knowledgeable than I am. Would you suggest that I replace my DCHECK()
with
> "if(!opener) return;"?

See RenderViewHostImpl::Shutdown for what I think would make sense.  It just
wraps the lines that refer to opener with an "if (opener)" block.

Also, let's have Darin review, since I'm not sure I understand the implications
of RunModal.  (It appears to be creating a new RenderViewHost solely for the
dialog, and cleaning up when that RenderViewHost is shut down.)

Please file a Chrome bug to use in your bug description, and please remove the
TBR.  This should be reviewed before landing.  Thanks for catching it!

[Kyle Horimoto, 2012-07-26 23:53:13]

On 2012/07/26 23:13:40, creis wrote:
> On 2012/07/26 23:02:20, Kyle Horimoto wrote:
> > On 2012/07/26 19:13:06, creis wrote:
> > > On 2012/07/26 18:27:32, Kyle Horimoto wrote:
> > > > +creis (who wrote the code in question)
> > > > 
> > > > creis, we used the static code analysis tool Coverity, and it discovered
> > this
> > > as
> > > > a possible case where we may be dereferencing a NULL pointer. Is this an
> > > > appropriate place to put a DCHECK()?
> > > 
> > > As Tyler mentioned, the DCHECK would be redundant here, since we would
> safely
> > > crash on the next line if it failed anyway.
> > > 
> > > Are you aware of a case where the opener pointer can be null?  That would
be
> a
> > > bug that we need to fix.  Otherwise it seems like suppressing the warning
> > would
> > > be the right thing to do.
> > > 
> > > Now that I look at it, I do wonder if it's possible for opener to be null.

> If
> > > the opener tab is different than the current tab and the IPC message
arrived
> > > after the browser process closed the opener tab, then FromID would return
> > null. 
> > > Ditto if the renderer process has been exploited and sends garbage in the
> IPC
> > > message.
> > > 
> > > Maybe it's worth putting a null check instead of a DCHECK.
> > 
> > I'm not really too sure about this code as I haven't ever worked on it - I
> only
> > worked on this bug because the code analysis tool caught it. You'd
definitely
> be
> > more knowledgeable than I am. Would you suggest that I replace my DCHECK()
> with
> > "if(!opener) return;"?
> 
> See RenderViewHostImpl::Shutdown for what I think would make sense.  It just
> wraps the lines that refer to opener with an "if (opener)" block.
> 
> Also, let's have Darin review, since I'm not sure I understand the
implications
> of RunModal.  (It appears to be creating a new RenderViewHost solely for the
> dialog, and cleaning up when that RenderViewHost is shut down.)
> 
> Please file a Chrome bug to use in your bug description, and please remove the
> TBR.  This should be reviewed before landing.  Thanks for catching it!

Done. darin, thoughts?

[darin (slow to review), 2012-07-26 23:54:45]

Reading http://crbug.com/139242, my take on this issue is that we should avoid
crashing the browser, and instead, we should crash the renderer.  It sent us
junk, so perhaps it is malicious.

[Charlie Reis, 2012-07-27 00:05:39]

On 2012/07/26 23:54:45, darin wrote:
> Reading http://crbug.com/139242, my take on this issue is that we should avoid
> crashing the browser, and instead, we should crash the renderer.  It sent us
> junk, so perhaps it is malicious.

Darin, is there something preventing the opener tab from being closed between
the time the message was legitimately sent and when it was received by the
browser process?  I'm not convinced that a null opener means the renderer is
malicious.

[darin (slow to review), 2012-07-27 00:09:35]

On Thu, Jul 26, 2012 at 5:05 PM, <creis@chromium.org> wrote:

> On 2012/07/26 23:54:45, darin wrote:
>
>> Reading http://crbug.com/139242, my take on this issue is that we should
>> avoid
>> crashing the browser, and instead, we should crash the renderer.  It sent
>> us
>> junk, so perhaps it is malicious.
>>
>
> Darin, is there something preventing the opener tab from being closed
> between
> the time the message was legitimately sent and when it was received by the
> browser process?  I'm not convinced that a null opener means the renderer
> is
> malicious.
>

Oh, that's a great question.  (Sorry if I missed that in the backlog.)

Since this message is sent as a synchronous IPC, the only way the opener
could
be closed from the perspective of the renderer is if the entire process was
shutdown.
At browser shutdown, we get aggressive about shutting down renderers.

However, it does seem like there could be a legit race between the user
trying to
close a tab and the corresponding web page trying to call showModalDialog.

Hmm...

However, don't we generally perform a close / close-ack dance when closing
a tab
from the browser-side?  That would require IPCs being processed on the main
thread
of the renderer, but if that thread is blocked by a synchronous IPC, then
we wouldn't
have seen the close-ack on the browser side yet.  So, the browser would
still see the
opener RenderViewHost.

Hmm...

Seems like malicious would be the only possibility given the above, but I
think it
requires more study to confirm my assumptions.

-Darin



>
>
http://codereview.chromium.**org/10820017/<http://codereview.chromium.org/108...
>

[Charlie Reis, 2012-07-27 00:25:53]

On 2012/07/27 00:09:35, darin wrote:
> Oh, that's a great question.  (Sorry if I missed that in the backlog.)
> 
> Since this message is sent as a synchronous IPC, the only way the opener
> could
> be closed from the perspective of the renderer is if the entire process was
> shutdown.
> At browser shutdown, we get aggressive about shutting down renderers.
> 
> However, it does seem like there could be a legit race between the user
> trying to
> close a tab and the corresponding web page trying to call showModalDialog.
> 
> Hmm...
> 
> However, don't we generally perform a close / close-ack dance when closing
> a tab
> from the browser-side?  That would require IPCs being processed on the main
> thread
> of the renderer, but if that thread is blocked by a synchronous IPC, then
> we wouldn't
> have seen the close-ack on the browser side yet.  So, the browser would
> still see the
> opener RenderViewHost.

Except that we have a 1 second timeout waiting for the close-ack, and then we go
ahead with it.  (Plus a fast-path if we know there's no unload handler
attached.)

I just confirmed that it's possible to get a null opener in practice.  Repro
steps:
1) Attach a debugger to the renderer and set a breakpoint in
RenderViewImpl::runModal.
2) Visit a page that opens a modal dialog.  The dialog popup will appear, and
the breakpoint will be hit.
3) Close the tab that created the dialog.
4) Resume the debugger.

The browser process now crashes because the opener is null.

That seems legit to me.  LGTM to this fix for it.  Sound reasonable o you,
Darin?

Charlie

> 
> Hmm...
> 
> Seems like malicious would be the only possibility given the above, but I
> think it
> requires more study to confirm my assumptions.
> 
> -Darin
> 
> 
> 
> >
> >
>
http://codereview.chromium.**org/10820017/%3Chttp://codereview.chromium.org/1...>
> >

[Charlie Reis, 2012-07-27 00:25:53]

On 2012/07/27 00:09:35, darin wrote:
> Oh, that's a great question.  (Sorry if I missed that in the backlog.)
> 
> Since this message is sent as a synchronous IPC, the only way the opener
> could
> be closed from the perspective of the renderer is if the entire process was
> shutdown.
> At browser shutdown, we get aggressive about shutting down renderers.
> 
> However, it does seem like there could be a legit race between the user
> trying to
> close a tab and the corresponding web page trying to call showModalDialog.
> 
> Hmm...
> 
> However, don't we generally perform a close / close-ack dance when closing
> a tab
> from the browser-side?  That would require IPCs being processed on the main
> thread
> of the renderer, but if that thread is blocked by a synchronous IPC, then
> we wouldn't
> have seen the close-ack on the browser side yet.  So, the browser would
> still see the
> opener RenderViewHost.

Except that we have a 1 second timeout waiting for the close-ack, and then we go
ahead with it.  (Plus a fast-path if we know there's no unload handler
attached.)

I just confirmed that it's possible to get a null opener in practice.  Repro
steps:
1) Attach a debugger to the renderer and set a breakpoint in
RenderViewImpl::runModal.
2) Visit a page that opens a modal dialog.  The dialog popup will appear, and
the breakpoint will be hit.
3) Close the tab that created the dialog.
4) Resume the debugger.

The browser process now crashes because the opener is null.

That seems legit to me.  LGTM to this fix for it.  Sound reasonable o you,
Darin?

Charlie

> 
> Hmm...
> 
> Seems like malicious would be the only possibility given the above, but I
> think it
> requires more study to confirm my assumptions.
> 
> -Darin
> 
> 
> 
> >
> >
>
http://codereview.chromium.**org/10820017/%3Chttp://codereview.chromium.org/1...>
> >

[darin (slow to review), 2012-07-27 03:02:22]

Yeah, makes sense.  LGTM!

[darin (slow to review), 2012-07-27 03:02:46]

NOTE:  Please fix the CL description before committing.

[commit-bot: I haz the power, 2012-07-27 03:08:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10820017/3

[commit-bot: I haz the power, 2012-07-27 04:03:46]

Change committed as 148718