Avoid null-pointer dereference for PPAPI Instance BindGraphics....

Avoid null-pointer dereference for PPAPI Instance BindGraphics.

BUG=None
TEST=NaCl SDK pi_generator example encounters this

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

[dmichael(do not use this one), 2011-01-24 20:09:55]

Looking at the way this appears to work, I'm not sure how the buffer was NULL. 
I'm still working on figuring out what's wrong with pi_generator.  But I think
we want to avoid allowing untrusted code to cause a NULL dereference.  This got
me past a tab crash I was seeing.

[David Springer, 2011-01-24 20:19:29]

One request for clarification, o/w/ LGTM.

http://codereview.chromium.org/6384009/diff/1/webkit/plugins/ppapi/ppapi_plug...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6384009/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:462:
bound_graphics_2d()->image_data()->GetMappedBitmap();
Is there any chance that these accessors could return a NULL ptr?  E.g. I can
imagine image_data() returning a NULL ptr while initialization is going on in
the NaCl process.  It seems to me that the chance of a NULL ptr is greater when
using a NaCl process than when using a trusted plugin.

[brettw, 2011-01-24 21:00:05]

http://codereview.chromium.org/6384009/diff/1/webkit/plugins/ppapi/ppapi_plug...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6384009/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:463: if (old_backing_bitmap !=
NULL) {
Instead of checking to see if there's no mapped bitmap, this should just map the
data. Can you use an ImageDataAutoMapper on the image data? Then GetMappedBitmap
will always return success.

[neb, 2011-01-24 21:32:21]

http://codereview.chromium.org/6384009/diff/6001/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6384009/diff/6001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:463: if (old_backing_bitmap !=
NULL) {
Shouldn't it fail if NULL? That should not be possible short of internal error
or malicious code.

[dmichael(do not use this one), 2011-01-24 21:58:58]

It seemed to me also like it shouldn't ever be NULL, assuming everything happens
on the main thread.  This is happening in pi_generator in the SDK, even when I
hack pi_generator down to a single threaded app;  so it tells me that there may
be a problem in the NaCl proxy code.

The high-level view is that DidChangeView gets called on the ScriptableObject,
which then makes a new pp::Graphics2d and calls BindGraphics on itself.  That
ultimately lands at this point in the code with a NULL bitmap.

The root cause is probably a mistake in the proxy...  but I thought it better to
avoid the crash in trusted code.  If there's something I can do to help you
debug the proxy code (if you think it might be there), let me know.  I can get
you a stack trace or tell you how to tweak pi_generator to cause this.

[neb, 2011-01-24 22:07:39]

sehr@ already found he place where we mistakenly close the handle in the proxy.
I agree that this needs to be changed to avoid crashing if untrusted side plays
with the handles. I just thought we don't need to handle it more gracefully than
just failing.

[brettw, 2011-01-24 22:38:06]

If we want to make it robust against failure to map, we should do such checking
every place we call GetMappedBitmap rather than just this random one.

[dmichael(do not use this one), 2011-01-24 23:26:12]

Poking around, it looks like many places in the code do check, via
ImageDataAutoMapper.
http://codesearch.google.com/codesearch?as_q=GetMappedBitmap&btnG=Search+Code...
The exceptions appear to be DeviceContext2D::Paint and
DeviceContext2D::ConvertImageData (although I think most/all cases of
ConvertImageData calls are wrapped).  And the code in this CL doesn't check.

It seems appropriate to me to put the ImageDataAutoMapper usage in here based on
what I see elsewhere, but I'll happily defer to the experts.

On 2011/01/24 22:38:06, brettw wrote:
> If we want to make it robust against failure to map, we should do such
checking
> every place we call GetMappedBitmap rather than just this random one.

[brettw, 2011-01-25 00:41:24]

LGTM