[cairo] [PATCH] win32: Attempt to solve a nasty use-after-free by the caller of clone_similar()

[Owen Taylor]

On Wed, 2009-02-25 at 23:22 +0000, Chris Wilson wrote:
> Jeff Muizelaar tracked down a serious problem caused by a conflict of
> assumptions between the surface layer and the win32 backend. The win32
> backend only keeps the bits backing a surface only for the lifetime of
> that surface (quite reasonably behaviour) and documents that its
> surface->image is only valid for the lifetime of the parent surface.
> However, the backing bits are exposed when the image surface is returned
> by acquire_source_image() which is used when we need to clone the win32
> surface in order to paint to an image surface (or indeed any other
> non-win32 surface). The _cairo_image_clone_surface(), however, does not
> clone the underlying bits, but, merely returns a fresh reference when it
> is given an image source (again, such behaviour is allowed by the internal
> API documentation). The issue now arises that we have an image surface
> that is no longer bound to the lifetime of its containing win32 surface.
> 
> There appear to be at least 3 methods of tackling this issue.
> 
> 1. Actually copy the bits in _cairo_image_surface_clone().
> 2. Restructure _cairo_win32_surface_finish() to defer destroying the
>    underlying bitmap until the image surface is destroyed.
> 3. Create a temporary image surface that holds a reference to the
>    containing win32 surface.
> 
> This patch attempts to do 3, since that appears to be the simplest and
> least controversial. Beware that this has not even be compiled tested!

acquire_source_image is not designed to give you a surface that you can
keep around persistently. That's why there's a special release()
function and the image_extra argument... if you could just ref the image
and keep it around, then cairo_surface_destroy() would have been
sufficient.

It's meant to be locally scoped, and since the parent surface has to be
passed in to release(), we know it is around for the entire scope.

That could be considered a problem with the API design, but even if you
figured out the memory management, you still have the problem of
mutation of the source's content. If I:

 cr.set_source_image(some_win32_surface)
 cr.paint()
 cr.set_source_image(null)

I'm free to, using cairo or win32 api's change the contents of my win32
surface and it can't have any effect on the previous drawing. If for the
metafile/pdf/cairo-script/whatever backend, we need to take a persistent
snapshot, then that has to be done as a separate API. I don't think the
performance hit of always making an extraneous copy when, say, copying
from a Win32 DIB to an image surface is attractive.

- Owen