Bug 225574 – Add back protection of the pixel buffer in ImageBufferCGBackend::toCFData removed in r277237

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Add back protection of the pixel buffer in ImageBufferCGBackend::toCFData removed in r277237

[Sam Weinig]

Created attachment 428108 [details]
Patch

[Darin Adler]

Comment on attachment 428108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428108&action=review

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:526
> +    size_t dataSize = rowBytes * imageSize.height();

Not new: This computation is peculiar because it’s done as a 32-bit integer and then the results are put into a size_t. If it’s too big for 32-bits, it will overflow.

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:530
> +        reinterpret_cast<ImageData*>(context)->deref();

Since this is void*, we can and should use static_cast instead of reinterpret_cast.

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:201
> +            reinterpret_cast<ImageData*>(context)->deref();

Ditto.

[EWS]

Committed r277255 (237524@main): <https://commits.webkit.org/237524@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428108 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77723692>

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 428108 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428108&action=review
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:526
> > +    size_t dataSize = rowBytes * imageSize.height();
> 
> Not new: This computation is peculiar because it’s done as a 32-bit integer
> and then the results are put into a size_t. If it’s too big for 32-bits, it
> will overflow.

I agree i's odd, but I don't believe it can overflow, since ImageData checks for that case in all its create functions and fails if it overflows. My guess about the size_t is that is what CGDataProviderCreateWithData takes and so someone was just matching types.

> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:530
> > +        reinterpret_cast<ImageData*>(context)->deref();
> 
> Since this is void*, we can and should use static_cast instead of
> reinterpret_cast.
> 
> > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:201
> > +            reinterpret_cast<ImageData*>(context)->deref();
> 
> Ditto.

Going to fix this in the next patch, where I touch this once again.