Re: Crash in cairo_surface_get_mime_data()

From 38ce0850b8dba23c398faebb58905b5942e9c024 Mon Sep 17 00:00:00 2001
From: Uli Schlachter <[email protected]>
Date: Tue, 4 Oct 2011 14:58:41 +0200
Subject: [PATCH 1/2] test: Add mime-surface-api

This test checks if setting and unsetting mime data works correctly. E.g. this
verifies that we get the same pointer back which we passed in (=no copy made).

This test currently crashes in its last call to cairo_surface_get_mime_data().

Signed-off-by: Uli Schlachter <[email protected]>
---
 test/Makefile.sources   |    1 +
 test/mime-surface-api.c |  151 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 152 insertions(+), 0 deletions(-)
 create mode 100644 test/mime-surface-api.c

diff --git a/test/Makefile.sources b/test/Makefile.sources
index afb5c97..14d416c 100644
--- a/test/Makefile.sources
+++ b/test/Makefile.sources
@@ -202,6 +202,7 @@ test_sources = \
 	mesh-pattern-transformed.c		        \
 	mime-data.c					\
 	mime-surface.c					\
+	mime-surface-api.c				\
 	miter-precision.c				\
 	move-to-show-surface.c				\
 	new-sub-path.c					\
diff --git a/test/mime-surface-api.c b/test/mime-surface-api.c
new file mode 100644
index 0000000..ce12653
--- /dev/null
+++ b/test/mime-surface-api.c
@@ -0,0 +1,151 @@
+/*
+ * Copyright © 2011 Uli Schlachter
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy,
+ * modify, merge, publish, distribute, sublicense, and/or sell copies
+ * of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Author: Uli Schlachter <[email protected]>
+ */
+
+#include "cairo-test.h"
+
+static void
+mime_data_destroy_func (void *data)
+{
+    cairo_bool_t *called = data;
+    *called = TRUE;
+}
+
+static cairo_test_status_t
+check_mime_data (cairo_test_context_t *ctx, cairo_surface_t *surface,
+		 const char *mimetype, const unsigned char *data,
+		 unsigned long length)
+{
+    const unsigned char *data_ret;
+    unsigned long length_ret;
+
+    cairo_surface_get_mime_data (surface, mimetype, &data_ret, &length_ret);
+    if (data_ret != data || length_ret != length) {
+	cairo_test_log (ctx,
+			"Surface has mime data %p with length %lu, "
+			"but expected %p with length %lu\n",
+			data_ret, length_ret, data, length);
+       return CAIRO_TEST_ERROR;
+    }
+
+    return CAIRO_TEST_SUCCESS;
+}
+
+static cairo_test_status_t
+set_and_check_mime_data (cairo_test_context_t *ctx, cairo_surface_t *surface,
+			 const char *mimetype, const unsigned char *data,
+			 unsigned long length, cairo_bool_t *destroy_called)
+{
+    cairo_status_t status;
+
+    status = cairo_surface_set_mime_data (surface, mimetype,
+					  data, length,
+					  mime_data_destroy_func,
+					  destroy_called);
+    if (status) {
+	cairo_test_log (ctx, "Could not set mime data to %s: %s\n",
+			data, cairo_status_to_string(status));
+	return CAIRO_TEST_ERROR;
+    }
+
+    return check_mime_data (ctx, surface, mimetype, data, length);
+}
+
+static cairo_test_status_t
+preamble (cairo_test_context_t *ctx)
+{
+    const char *mimetype = "text/x-uri";
+    const char *data1 = "http://www.cairographics.org";
+    const char *data2 = "http://cairographics.org/examples/";
+    cairo_bool_t destroy1_called = FALSE;
+    cairo_bool_t destroy2_called = FALSE;
+    cairo_surface_t *surface;
+    cairo_test_status_t test_status = CAIRO_TEST_SUCCESS;
+
+    surface = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, 0, 0);
+    if (cairo_surface_status (surface)) {
+	cairo_test_log (ctx, "Could not create image surface\n");
+	test_status = CAIRO_TEST_ERROR;
+	goto out;
+    }
+
+    test_status = check_mime_data (ctx, surface, mimetype, NULL, 0);
+    if (test_status)
+	goto out;
+
+    test_status = set_and_check_mime_data (ctx, surface, mimetype,
+					   (const unsigned char *) data1,
+					   strlen (data1),
+					   &destroy1_called);
+    if (test_status)
+	goto out;
+
+    if (destroy1_called) {
+	cairo_test_log (ctx, "MIME data 1 destroyed too early\n");
+	test_status = CAIRO_TEST_ERROR;
+	goto out;
+    }
+
+    test_status = set_and_check_mime_data (ctx, surface, mimetype,
+					   (const unsigned char *) data2,
+					   strlen (data2),
+					   &destroy2_called);
+    if (test_status)
+	goto out;
+
+    if (!destroy1_called) {
+	cairo_test_log (ctx, "MIME data 1 destroy callback not called\n");
+	test_status = CAIRO_TEST_ERROR;
+	goto out;
+    }
+    if (destroy2_called) {
+	cairo_test_log (ctx, "MIME data 2 destroyed too early\n");
+	test_status = CAIRO_TEST_ERROR;
+	goto out;
+    }
+
+    test_status = set_and_check_mime_data (ctx, surface, mimetype,
+					   NULL, 0, NULL);
+    if (test_status)
+	goto out;
+
+    if (!destroy2_called) {
+	cairo_test_log (ctx, "MIME data destroy callback not called\n");
+	test_status = CAIRO_TEST_ERROR;
+	goto out;
+    }
+
+out:
+    cairo_surface_destroy (surface);
+
+    return test_status;
+}
+
+CAIRO_TEST (mime_surface_api,
+	    "Check the mime data API",
+	    "api", /* keywords */
+	    NULL, /* requirements */
+	    0, 0,
+	    preamble, NULL)
-- 
1.7.6.3

From 960ae5ab2ee7910c9ef7b0b7df824d701c4d1729 Mon Sep 17 00:00:00 2001
From: Uli Schlachter <[email protected]>
Date: Tue, 4 Oct 2011 15:03:27 +0200
Subject: [PATCH 2/2] Make the new mime-surface-api succeed

When removing mime data, _cairo_user_data_array_set_data () is called with a
NULL argument. This leaves behind an entry with key == NULL in the user data
array. Skip those entries instead of dereferencing NULL.

(The NULL entry in the array let's us avoid moving data around and/or doing a
memory allocation later, so I guess it might be a good idea to keep that)

Signed-off-by: Uli Schlachter <[email protected]>
---
 src/cairo-surface.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/cairo-surface.c b/src/cairo-surface.c
index 8488380..3be6d42 100644
--- a/src/cairo-surface.c
+++ b/src/cairo-surface.c
@@ -1035,7 +1035,7 @@ cairo_surface_get_mime_data (cairo_surface_t		*surface,
     num_slots = surface->mime_data.num_elements;
     slots = _cairo_array_index (&surface->mime_data, 0);
     for (i = 0; i < num_slots; i++) {
-	if (strcmp ((char *) slots[i].key, mime_type) == 0) {
+	if (slots[i].key != NULL && strcmp ((char *) slots[i].key, mime_type) == 0) {
 	    cairo_mime_data_t *mime_data = slots[i].user_data;
 
 	    *data = mime_data->data;
-- 
1.7.6.3