[cairo-xlib] Handle malloc failures for cairo_xlib_visual_info_t.

Tidy the error paths whilst handling visuals, in particular avoiding a
couple of potential NULL deferences, missed status checks and fresh
leaks.

4 files changed, 83 insertions, 33 deletions

diff --git a/src/cairo-xlib-private.h b/src/cairo-xlib-private.h
index 5edb5d2c8..5bfc2ec56 100644
--- a/src/cairo-xlib-private.h
+++ b/src/cairo-xlib-private.h
@@ -133,11 +133,18 @@ _cairo_xlib_screen_get_gc (cairo_xlib_screen_info_t *info, int depth);
 cairo_private cairo_status_t
 _cairo_xlib_screen_put_gc (cairo_xlib_screen_info_t *info, int depth, GC gc, cairo_bool_t reset_clip);
 
-cairo_private cairo_xlib_visual_info_t *
+cairo_private cairo_status_t
 _cairo_xlib_screen_get_visual_info (cairo_xlib_screen_info_t *info,
-				    Visual *visual);
+				    Visual *visual,
+				    cairo_xlib_visual_info_t **out);
+
+cairo_private cairo_status_t
+_cairo_xlib_visual_info_create (Display *dpy,
+	                        int screen,
+				VisualID visualid,
+				cairo_xlib_visual_info_t **out);
 
-cairo_private cairo_xlib_visual_info_t *
-_cairo_xlib_visual_info_create (Display *dpy, int screen, VisualID visualid);
+cairo_private void
+_cairo_xlib_visual_info_destroy (Display *dpy, cairo_xlib_visual_info_t *info);
 
 #endif /* CAIRO_XLIB_PRIVATE_H */
diff --git a/src/cairo-xlib-screen.c b/src/cairo-xlib-screen.c
index cdba294d1..8a3da6675 100644
--- a/src/cairo-xlib-screen.c
+++ b/src/cairo-xlib-screen.c
@@ -286,7 +286,7 @@ _cairo_xlib_screen_info_destroy (cairo_xlib_screen_info_t *info)
     }
     visuals = _cairo_array_index (&info->visuals, 0);
     for (i = 0; i < _cairo_array_num_elements (&info->visuals); i++)
-	free (visuals[i]);
+	_cairo_xlib_visual_info_destroy (info->display->display, visuals[i]);
     CAIRO_MUTEX_UNLOCK (info->display->mutex);
 
     _cairo_xlib_screen_info_close_display (info);
@@ -422,17 +422,19 @@ _cairo_xlib_screen_put_gc (cairo_xlib_screen_info_t *info, int depth, GC gc, cai
     return status;
 }
 
-cairo_xlib_visual_info_t *
+cairo_status_t
 _cairo_xlib_screen_get_visual_info (cairo_xlib_screen_info_t *info,
-				    Visual *visual)
+				    Visual *visual,
+				    cairo_xlib_visual_info_t **out)
 {
     cairo_xlib_visual_info_t **visuals, *ret = NULL;
     cairo_status_t status;
-    int i;
+    int i, n_visuals;
 
     CAIRO_MUTEX_LOCK (info->display->mutex);
     visuals = _cairo_array_index (&info->visuals, 0);
-    for (i = 0; i < _cairo_array_num_elements (&info->visuals); i++) {
+    n_visuals = _cairo_array_num_elements (&info->visuals);
+    for (i = 0; i < n_visuals; i++) {
 	if (visuals[i]->visualid == visual->visualid) {
 	    ret = visuals[i];
 	    break;
@@ -440,23 +442,41 @@ _cairo_xlib_screen_get_visual_info (cairo_xlib_screen_info_t *info,
     }
     CAIRO_MUTEX_UNLOCK (info->display->mutex);
 
-    if (ret)
-	return ret;
+    if (ret != NULL) {
+	*out = ret;
+	return CAIRO_STATUS_SUCCESS;
+    }
 
-    ret = _cairo_xlib_visual_info_create (info->display->display,
-					  XScreenNumberOfScreen (info->screen),
-					  visual->visualid);
-    if (ret == NULL)
-	return ret;
+    status = _cairo_xlib_visual_info_create (info->display->display,
+					     XScreenNumberOfScreen (info->screen),
+					     visual->visualid,
+					     &ret);
+    if (status)
+	return status;
 
     CAIRO_MUTEX_LOCK (info->display->mutex);
-    status = _cairo_array_append (&info->visuals, &ret);
+    if (n_visuals != _cairo_array_num_elements (&info->visuals)) {
+	/* check that another thread has not added our visual */
+	int new_visuals = _cairo_array_num_elements (&info->visuals);
+	visuals = _cairo_array_index (&info->visuals, 0);
+	for (i = n_visuals; i < new_visuals; i++) {
+	    if (visuals[i]->visualid == visual->visualid) {
+		_cairo_xlib_visual_info_destroy (info->display->display, ret);
+		ret = visuals[i];
+		break;
+	    }
+	}
+	if (i == new_visuals)
+	    status = _cairo_array_append (&info->visuals, &ret);
+    } else
+	status = _cairo_array_append (&info->visuals, &ret);
     CAIRO_MUTEX_UNLOCK (info->display->mutex);
 
     if (status) {
-	free (ret);
-	ret = NULL;
+	_cairo_xlib_visual_info_destroy (info->display->display, ret);
+	return status;
     }
 
-    return ret;
+    *out = ret;
+    return CAIRO_STATUS_SUCCESS;
 }
diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
index 0662b70bd..c0ffd05d5 100644
--- a/src/cairo-xlib-surface.c
+++ b/src/cairo-xlib-surface.c
@@ -695,8 +695,13 @@ _get_image_surface (cairo_xlib_surface_t    *surface,
 
 	    format = CAIRO_FORMAT_RGB24;
 
-	    visual_info = _cairo_xlib_screen_get_visual_info (surface->screen_info,
-							      surface->visual);
+	    status = _cairo_xlib_screen_get_visual_info (surface->screen_info,
+							 surface->visual,
+							 &visual_info);
+	    if (status) {
+		XDestroyImage (ximage);
+		return status;
+	    }
 
 	    colors = visual_info->colors;
 	}
@@ -845,7 +850,7 @@ _draw_image_surface (cairo_xlib_surface_t   *surface,
     unsigned long *rgb333_to_pseudocolor = NULL;
 
     _pixman_format_to_masks (image->pixman_format, &image_masks);
-    
+
     ximage.width = image->width;
     ximage.height = image->height;
     ximage.format = ZPixmap;
@@ -859,7 +864,7 @@ _draw_image_surface (cairo_xlib_surface_t   *surface,
     ximage.blue_mask = surface->b_mask;
     ximage.xoffset = 0;
 
-    if (image_masks.red_mask   == surface->r_mask   &&
+    if (image_masks.red_mask   == surface->r_mask &&
 	image_masks.green_mask == surface->g_mask &&
 	image_masks.blue_mask  == surface->b_mask)
     {
@@ -887,7 +892,10 @@ _draw_image_surface (cairo_xlib_surface_t   *surface,
 	stride = CAIRO_STRIDE_FOR_WIDTH_BPP (ximage.width,
 					     ximage.bits_per_pixel);
 	ximage.bytes_per_line = stride;
-	ximage.data = malloc (stride * ximage.height);
+	ximage.data = _cairo_malloc_ab (stride, ximage.height);
+	if (ximage.data == NULL)
+	    return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+
 	own_data = TRUE;
 
 	XInitImage (&ximage);
@@ -900,8 +908,11 @@ _draw_image_surface (cairo_xlib_surface_t   *surface,
 	} else {
 	    cairo_xlib_visual_info_t *visual_info;
 
-	    visual_info = _cairo_xlib_screen_get_visual_info (surface->screen_info,
-							      surface->visual);
+	    status = _cairo_xlib_screen_get_visual_info (surface->screen_info,
+							 surface->visual,
+							 &visual_info);
+	    if (status)
+		goto BAIL;
 
 	    rgb333_to_pseudocolor = visual_info->rgb333_to_pseudocolor;
 	}
@@ -933,16 +944,17 @@ _draw_image_surface (cairo_xlib_surface_t   *surface,
 
     status = _cairo_xlib_surface_ensure_gc (surface);
     if (status)
-	return status;
+	goto BAIL;
 
     XPutImage(surface->dpy, surface->drawable, surface->gc,
 	      &ximage, src_x, src_y, dst_x, dst_y,
 	      width, height);
 
+  BAIL:
     if (own_data)
 	free (ximage.data);
 
-    return CAIRO_STATUS_SUCCESS;
+    return status;
 }
 
 static cairo_status_t
diff --git a/src/cairo-xlib-visual.c b/src/cairo-xlib-visual.c
index 882028ac0..a45be53f3 100644
--- a/src/cairo-xlib-visual.c
+++ b/src/cairo-xlib-visual.c
@@ -56,8 +56,11 @@ _color_distance (unsigned short r1, unsigned short g1, unsigned short b1,
 	    (b2 - b1) * (b2 - b1));
 }
 
-cairo_xlib_visual_info_t *
-_cairo_xlib_visual_info_create (Display *dpy, int screen, VisualID visualid)
+cairo_status_t
+_cairo_xlib_visual_info_create (Display *dpy,
+	                        int screen,
+				VisualID visualid,
+				cairo_xlib_visual_info_t **out)
 {
     cairo_xlib_visual_info_t *info;
     Colormap colormap = DefaultColormap (dpy, screen);
@@ -75,7 +78,7 @@ _cairo_xlib_visual_info_create (Display *dpy, int screen, VisualID visualid)
 
     info = malloc (sizeof (cairo_xlib_visual_info_t));
     if (info == NULL)
-	return NULL;
+	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
 
     info->visualid = visualid;
 
@@ -133,5 +136,13 @@ _cairo_xlib_visual_info_create (Display *dpy, int screen, VisualID visualid)
 	}
     }
 
-    return info;
+    *out = info;
+    return CAIRO_STATUS_SUCCESS;
+}
+
+void
+_cairo_xlib_visual_info_destroy (Display *dpy, cairo_xlib_visual_info_t *info)
+{
+    /* No need for XFreeColors() whilst using DefaultColormap */
+    free (info);
 }