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

Jeff Muizelaar jeff at infidigm.net
Thu Feb 26 09:28:56 PST 2009


On Thu, Feb 26, 2009 at 10:59:23AM +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 for the lifetime of
> that surface (quite reasonable 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. 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.
> 

It looks like the win32 surface isn't the only surface that doesn't
wan't the image surface returned by acquire_source_image() to live pass
the call to release_source_image(). The qpainter surface and the quartz
surface also make similar assumptions. The quartz surface doesn't seem
to break in the clone_similar() case but it looks like the qpainter
surface will.

> 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.

Perhaps a better temporary solution would be to wrap the return from
acquire_source_image in clone_similar() and call release_source_image in
the image surface destructor. In the long term we should probably define
the semantics of these more explicitly.

-Jeff


More information about the cairo mailing list