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

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 been compiled tested!
---
 src/cairo-win32-surface.c |   66 ++++++++++++++++++++++++++++++++-------------
 1 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/src/cairo-win32-surface.c b/src/cairo-win32-surface.c
index f0c7aa2..8743435 100644
--- a/src/cairo-win32-surface.c
+++ b/src/cairo-win32-surface.c
@@ -561,6 +561,38 @@ _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,
+					      parent,
+					      (cairo_destroy_func_t) cairo_surface_destroy);
+    if (status) {
+	cairo_surface_destroy (surface);
+	return status;
+    }
+
+    cairo_surface_reference (parent);
+
+    *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)
@@ -569,23 +601,22 @@ _cairo_win32_surface_acquire_source_image (void                    *abstract_sur
     cairo_win32_surface_t *local = NULL;
     cairo_status_t status;
 
-    if (surface->image) {
-	*image_out = (cairo_image_surface_t *)surface->image;
-	*image_extra = NULL;
-
-	return CAIRO_STATUS_SUCCESS;
-    }
-
-    status = _cairo_win32_surface_get_subimage (abstract_surface, 0, 0,
-						surface->extents.width,
-						surface->extents.height, &local);
-    if (status)
-	return status;
+    if (surface->image == NULL) {
+	status = _cairo_win32_surface_get_subimage (abstract_surface,
+						    0, 0,
+						    surface->extents.width,
+						    surface->extents.height,
+						    &local);
+	if (status)
+	    return status;
+    } else
+	local = cairo_surface_reference (surface);
 
-    *image_out = (cairo_image_surface_t *)local->image;
-    *image_extra = local;
+    /* ensure that the image holds a reference to the win32 container */
+    status = _wrap_image (local, image_out);
+    cairo_surface_destroy (local);
 
-    return CAIRO_STATUS_SUCCESS;
+    return status;
 }
 
 static void
@@ -593,10 +624,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);
 }
 
 static cairo_status_t
-- 
1.6.0.4