Heap-use-after-free in WebCore::GraphicsContext::drawImage

Heap-use-after-free in WebCore::GraphicsContext::drawImage

Replaced getShapeImageAndRect() in ShapeOutsideInfo.cpp with
ShapeOutsideInfo::createShapeForImage(). The original function
failed to protect the PassRefPtr return value returned by StyleImage::image(). 

BUG=378469
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175176

[Hans Muller, 2014-05-30 21:03:41]

Uncached gradient images are now safe for shapes.

I'd appreciate a review.

[eseidel, 2014-05-30 21:06:45]

lgtm

https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
Source/core/rendering/shapes/ShapeOutsideInfo.cpp:117: Image* image = 0;
Might as well just make this a RefPtr and then call .release() when passing it
off to createRasterShape?

[pdr., 2014-05-30 21:08:15]

On 2014/05/30 21:06:45, eseidel wrote:
> lgtm
> 
>
https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
> File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):
> 
>
https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
> Source/core/rendering/shapes/ShapeOutsideInfo.cpp:117: Image* image = 0;
> Might as well just make this a RefPtr and then call .release() when passing it
> off to createRasterShape?

LGTM2 with the above.

[rwlbuis, 2014-05-30 21:14:05]

LGTM

https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
Source/core/rendering/shapes/ShapeOutsideInfo.cpp:118: RefPtr<Image>
generatedImage;
Nit: could move this into the else if section since that is the only scope it is
used in.

[Hans Muller, 2014-05-30 21:17:53]

On 2014/05/30 21:08:15, pdr wrote:
> On 2014/05/30 21:06:45, eseidel wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
> > File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):
> > 
> >
>
https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
> > Source/core/rendering/shapes/ShapeOutsideInfo.cpp:117: Image* image = 0;
> > Might as well just make this a RefPtr and then call .release() when passing
it
> > off to createRasterShape?
> 
> LGTM2 with the above.

Is this what you're suggesting? I'm probably missing something but it seems like
I've got to keep the generatedImage RefPtr around, so that generatedImage isn't
destroyed until after createRasterShape() returns.  

    RefPtr<Image> image;
    RefPtr<Image> generatedImage;

    if (styleImage->isImageResource() || styleImage->isImageResourceSet()) {
        image = styleImage->cachedImage()->imageForRenderer(&m_renderer);
    } else if (styleImage->isGeneratedImage()) {
        generatedImage = styleImage->image(const_cast<RenderBox*>(&m_renderer),
imageSize).get();
        image = generatedImage.get();
    }

    return Shape::createRasterShape(image.release(), shapeImageThreshold,
imageRect, marginRect, writingMode, margin);

[Hans Muller, 2014-05-30 21:20:24]

On 2014/05/30 21:14:05, rwlbuis wrote:
> LGTM
> 
>
https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
> File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):
> 
>
https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
> Source/core/rendering/shapes/ShapeOutsideInfo.cpp:118: RefPtr<Image>
> generatedImage;
> Nit: could move this into the else if section since that is the only scope it
is
> used in.

I need the scope of generatedImage to be outside the else clause, so that it
will survive until the function exits (until createRasterShape() completes).

[eseidel, 2014-05-30 21:22:08]

https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/305173003/diff/1/Source/core/rendering/shapes...
Source/core/rendering/shapes/ShapeOutsideInfo.cpp:124: image =
generatedImage.get();
I was more reacting to this line which is strange.  This creates a raw pointer
to generatedImage which is RefCounted and who knows when it might go away.  If
image is a RefPtr then this looks like:

image = generatedImage;

and then the later line is:

createRasterShape(image.release(), ...)

And then there is no question about some future refactoring causing
generatedImage to be dead before the raw ptr image is cleared, because image is
no longer a raw ptr and keeps its own ref.

[eseidel, 2014-05-30 21:23:53]

If you were concerned about the ref-churn, you could even do:
image = generatedImage.release();

Then again, all this becomes raw ptrs with oilpan and we no longer have to have
these theoretical, defense-against-the-some-future-refactoring discussions. :)

[eseidel, 2014-05-30 21:28:52]

createRasterShape takes a PassRefPtr, correct?  If so, then it doesn't matter
how long generatedImage survives, you're passing it a ref'd live object via the
PassRefPtr.

[Hans Muller, 2014-05-30 21:34:48]

On 2014/05/30 21:28:52, eseidel wrote:
> createRasterShape takes a PassRefPtr, correct?  If so, then it doesn't matter
> how long generatedImage survives, you're passing it a ref'd live object via
the
> PassRefPtr.

No, createRasterShape() just takes an Image*.  Like
GraphicsContext::drawImage().  That's why I was bossing the raw Image* around.

[Hans Muller, 2014-05-30 22:07:04]

On 2014/05/30 21:34:48, Hans Muller wrote:
> On 2014/05/30 21:28:52, eseidel wrote:
> > createRasterShape takes a PassRefPtr, correct?  If so, then it doesn't
matter
> > how long generatedImage survives, you're passing it a ref'd live object via
> the
> > PassRefPtr.
> 
> No, createRasterShape() just takes an Image*.  Like
> GraphicsContext::drawImage().  That's why I was bossing the raw Image* around.

I'm going to commit this patch because I'm a little short on time.  If there's a
better  way to manage the ref pointery, I'll be happy to commit another version
later.

[Hans Muller, 2014-05-30 22:08:06]

The CQ bit was checked by hmuller@adobe.com

[commit-bot: I haz the power, 2014-05-30 22:09:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hmuller@adobe.com/305173003/20001

[commit-bot: I haz the power, 2014-05-31 00:10:15]

Change committed as 175176