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

[Chris Wilson]

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!
---
 src/cairo-win32-surface.c |   41 +++++++++++++++++++++++++++++++++++------
 1 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/src/cairo-win32-surface.c b/src/cairo-win32-surface.c
index f0c7aa2..289ff59 100644
--- a/src/cairo-win32-surface.c
+++ b/src/cairo-win32-surface.c
@@ -561,6 +561,36 @@ _cairo_win32_surface_get_subimage (cairo_win32_surface_t  *surface,
 }
 
 static cairo_status_t
+_wrap_image (cairo_surface_t *parent,
+	     cairo_image_surface_t **out)
+{
+    cairo_surface_t *surface;
+    cairo_image_surface_t *image;
+
+    image = (cairo_image_surface_t *) parent->image;
+    surface = cairo_image_surface_create_for_data (image->data,
+						   image->format,
+						   image->width,
+						   image->height,
+						   image->stride);
+    status = surface->status;
+    if (status)
+	return status;
+
+    status = _cairo_user_data_array_set_data (&surface->user_data,
+					      (cairo_user_data_key_t) parent,
+					      cairo_surface_reference (parent),
+					      (cairo_destroy_func_t) cairo_surface_destroy);
+    if (status) {
+	cairo_surface_destroy (surface);
+	return status;
+    }
+
+    *out = (cairo_image_surface_t *) surface;
+    return CAIRO_STATUS_SUCCESS;
+}
+
+static cairo_status_t
 _cairo_win32_surface_acquire_source_image (void                    *abstract_surface,
 					   cairo_image_surface_t  **image_out,
 					   void                   **image_extra)
@@ -570,9 +600,11 @@ _cairo_win32_surface_acquire_source_image (void                    *abstract_sur
     cairo_status_t status;
 
     if (surface->image) {
-	*image_out = (cairo_image_surface_t *)surface->image;
-	*image_extra = NULL;
+	status = _wrap_image (surface, image_out);
+	if (status)
+	    return status;
 
+	*image_extra = *image_out;
 	return CAIRO_STATUS_SUCCESS;
     }
 
@@ -593,10 +625,7 @@ _cairo_win32_surface_release_source_image (void                   *abstract_surf
 					   cairo_image_surface_t  *image,
 					   void                   *image_extra)
 {
-    cairo_win32_surface_t *local = image_extra;
-
-    if (local)
-	cairo_surface_destroy ((cairo_surface_t *)local);
+    cairo_surface_destroy (image_extra);
 }
 
 static cairo_status_t
-- 
1.6.0.4