From 7eb33099d34234dcccb8f96caba94b38fa385f16 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Fri, 27 Apr 2012 12:39:40 +0100 Subject: [PATCH] snapshot: Perform the cow under a mutex In order to prevent a race between concurrent destroy and use in another thread, we need to acquire a reference to the snapshot->target under a mutex. Whilst we hold that reference, it prevents the internal destroy mechanism from freeing the memory we are using (if we have a pointer to the original surface) and the client drops their final reference. Oh boy, talk about opening a can of worms... Signed-off-by: Chris Wilson --- src/cairo-analysis-surface.c | 12 +++------- src/cairo-image-source.c | 33 +++++++++++++++++++++++----- src/cairo-pattern.c | 4 +--- src/cairo-pdf-surface.c | 24 +++++++++++++------- src/cairo-ps-surface.c | 33 ++++++++++++++++++---------- src/cairo-script-surface.c | 5 +++-- src/cairo-surface-snapshot-inline.h | 9 +++++++- src/cairo-surface-snapshot-private.h | 2 ++ src/cairo-surface-snapshot.c | 7 +++++- src/cairo-surface-subsurface.c | 3 ++- src/cairo-surface.c | 5 ++++- src/cairo-traps-compositor.c | 11 ++-------- 12 files changed, 96 insertions(+), 52 deletions(-) diff --git a/src/cairo-analysis-surface.c b/src/cairo-analysis-surface.c index 5583f251e..8516094c2 100644 --- a/src/cairo-analysis-surface.c +++ b/src/cairo-analysis-surface.c @@ -171,11 +171,7 @@ _analyze_recording_surface_pattern (cairo_analysis_surface_t *surface, cairo_matrix_multiply (&tmp->ctm, &p2d, &surface->ctm); tmp->has_ctm = ! _cairo_matrix_is_identity (&tmp->ctm); - if (_cairo_surface_is_snapshot (source)) - source = _cairo_surface_snapshot_get_target (source); - if (_cairo_surface_is_subsurface (source)) - source = _cairo_surface_subsurface_get_target (source); - + source = _cairo_surface_get_source (source, NULL); status = _cairo_recording_surface_replay_and_create_regions (source, &tmp->base); analysis_status = tmp->has_unsupported ? CAIRO_INT_STATUS_IMAGE_FALLBACK : CAIRO_INT_STATUS_SUCCESS; @@ -412,8 +408,7 @@ _cairo_analysis_surface_mask (void *abstract_surface, if (source->type == CAIRO_PATTERN_TYPE_SURFACE) { cairo_surface_t *src_surface = ((cairo_surface_pattern_t *)source)->surface; - if (_cairo_surface_is_snapshot (src_surface)) - src_surface = _cairo_surface_snapshot_get_target (src_surface); + src_surface = _cairo_surface_get_source (src_surface, NULL); if (_cairo_surface_is_recording (src_surface)) { backend_source_status = _analyze_recording_surface_pattern (surface, source); @@ -424,8 +419,7 @@ _cairo_analysis_surface_mask (void *abstract_surface, if (mask->type == CAIRO_PATTERN_TYPE_SURFACE) { cairo_surface_t *mask_surface = ((cairo_surface_pattern_t *)mask)->surface; - if (_cairo_surface_is_snapshot (mask_surface)) - mask_surface = _cairo_surface_snapshot_get_target (mask_surface); + mask_surface = _cairo_surface_get_source (mask_surface, NULL); if (_cairo_surface_is_recording (mask_surface)) { backend_mask_status = _analyze_recording_surface_pattern (surface, mask); diff --git a/src/cairo-image-source.c b/src/cairo-image-source.c index ec557c53a..c5bd228c4 100644 --- a/src/cairo-image-source.c +++ b/src/cairo-image-source.c @@ -432,6 +432,13 @@ _acquire_source_cleanup (pixman_image_t *pixman_image, free (data); } +static void +_defer_free_cleanup (pixman_image_t *pixman_image, + void *closure) +{ + cairo_surface_destroy (closure); +} + static uint16_t expand_channel (uint16_t v, uint32_t bits) { @@ -816,11 +823,14 @@ _pixman_image_for_surface (cairo_image_surface_t *dst, (! is_mask || ! pattern->base.has_component_alpha || (pattern->surface->content & CAIRO_CONTENT_COLOR) == 0)) { + cairo_surface_t *defer_free = NULL; cairo_image_surface_t *source = (cairo_image_surface_t *) pattern->surface; cairo_surface_type_t type; - if (_cairo_surface_is_snapshot (&source->base)) - source = (cairo_image_surface_t *) _cairo_surface_snapshot_get_target (&source->base); + if (_cairo_surface_is_snapshot (&source->base)) { + defer_free = _cairo_surface_snapshot_get_target (&source->base); + source = (cairo_image_surface_t *) defer_free; + } type = source->base.backend->type; if (type == CAIRO_SURFACE_TYPE_IMAGE) { @@ -839,15 +849,19 @@ _pixman_image_for_surface (cairo_image_surface_t *dst, sample->x >= source->width || sample->y >= source->height) { - if (extend == CAIRO_EXTEND_NONE) + if (extend == CAIRO_EXTEND_NONE) { + cairo_surface_destroy (defer_free); return _pixman_transparent_image (); + } } else { pixman_image = _pixel_to_solid (source, sample->x, sample->y); - if (pixman_image) + if (pixman_image) { + cairo_surface_destroy (defer_free); return pixman_image; + } } } @@ -858,6 +872,7 @@ _pixman_image_for_surface (cairo_image_surface_t *dst, pattern->base.filter, ix, iy)) { + cairo_surface_destroy (defer_free); return pixman_image_ref (source->pixman_image); } #endif @@ -867,8 +882,16 @@ _pixman_image_for_surface (cairo_image_surface_t *dst, source->height, (uint32_t *) source->data, source->stride); - if (unlikely (pixman_image == NULL)) + if (unlikely (pixman_image == NULL)) { + cairo_surface_destroy (defer_free); return NULL; + } + + if (defer_free) { + pixman_image_set_destroy_function (pixman_image, + _defer_free_cleanup, + defer_free); + } } else if (type == CAIRO_SURFACE_TYPE_SUBSURFACE) { cairo_surface_subsurface_t *sub; cairo_bool_t is_contained = FALSE; diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c index d21200a52..940227d29 100644 --- a/src/cairo-pattern.c +++ b/src/cairo-pattern.c @@ -3668,9 +3668,7 @@ _cairo_pattern_get_ink_extents (const cairo_pattern_t *pattern, (const cairo_surface_pattern_t *) pattern; cairo_surface_t *surface = surface_pattern->surface; - if (_cairo_surface_is_snapshot (surface)) - surface = _cairo_surface_snapshot_get_target (surface); - + surface = _cairo_surface_get_source (surface, NULL); if (_cairo_surface_is_recording (surface)) { cairo_matrix_t imatrix; cairo_box_t box; diff --git a/src/cairo-pdf-surface.c b/src/cairo-pdf-surface.c index 4ff381a93..a0176014e 100644 --- a/src/cairo-pdf-surface.c +++ b/src/cairo-pdf-surface.c @@ -1221,23 +1221,28 @@ _get_source_surface_size (cairo_surface_t *source, *width = extents->width; *height = extents->height; } else { + cairo_surface_t *free_me = NULL; cairo_rectangle_int_t surf_extents; cairo_box_t box; cairo_bool_t bounded; if (_cairo_surface_is_snapshot (source)) - source = _cairo_surface_snapshot_get_target (source); + free_me = source = _cairo_surface_snapshot_get_target (source); status = _cairo_recording_surface_get_ink_bbox ((cairo_recording_surface_t *)source, &box, NULL); - if (unlikely (status)) + if (unlikely (status)) { + cairo_surface_destroy (free_me); return status; - - _cairo_box_round_to_rectangle (&box, extents); + } bounded = _cairo_surface_get_extents (source, &surf_extents); + cairo_surface_destroy (free_me); + *width = surf_extents.width; *height = surf_extents.height; + + _cairo_box_round_to_rectangle (&box, extents); } return CAIRO_STATUS_SUCCESS; @@ -2674,12 +2679,13 @@ _cairo_pdf_surface_emit_recording_surface (cairo_pdf_surface_t *surface, cairo_box_double_t bbox; cairo_int_status_t status; int alpha = 0; + cairo_surface_t *free_me = NULL; cairo_surface_t *source; assert (pdf_source->type == CAIRO_PATTERN_TYPE_SURFACE); source = pdf_source->surface; if (_cairo_surface_is_snapshot (source)) - source = _cairo_surface_snapshot_get_target (source); + free_me = source = _cairo_surface_snapshot_get_target (source); old_width = surface->width; old_height = surface->height; @@ -2700,12 +2706,12 @@ _cairo_pdf_surface_emit_recording_surface (cairo_pdf_surface_t *surface, _get_bbox_from_extents (pdf_source->hash_entry->height, &pdf_source->hash_entry->extents, &bbox); status = _cairo_pdf_surface_open_content_stream (surface, &bbox, &pdf_source->hash_entry->surface_res, TRUE); if (unlikely (status)) - return status; + goto err; if (cairo_surface_get_content (source) == CAIRO_CONTENT_COLOR) { status = _cairo_pdf_surface_add_alpha (surface, 1.0, &alpha); if (unlikely (status)) - return status; + goto err; _cairo_output_stream_printf (surface->output, "q /a%d gs 0 0 0 rg 0 0 %f %f re f Q\n", @@ -2720,7 +2726,7 @@ _cairo_pdf_surface_emit_recording_surface (cairo_pdf_surface_t *surface, CAIRO_RECORDING_REGION_NATIVE); assert (status != CAIRO_INT_STATUS_UNSUPPORTED); if (unlikely (status)) - return status; + goto err; status = _cairo_pdf_surface_close_content_stream (surface); @@ -2731,6 +2737,8 @@ _cairo_pdf_surface_emit_recording_surface (cairo_pdf_surface_t *surface, old_height); surface->paginated_mode = old_paginated_mode; +err: + cairo_surface_destroy (free_me); return status; } diff --git a/src/cairo-ps-surface.c b/src/cairo-ps-surface.c index d92029d2d..1285108a5 100644 --- a/src/cairo-ps-surface.c +++ b/src/cairo-ps-surface.c @@ -1696,16 +1696,19 @@ _cairo_ps_surface_acquire_source_surface_from_pattern (cairo_ps_surface_t *width = sub->extents.width; *height = sub->extents.height; } else { + cairo_surface_t *free_me = NULL; cairo_recording_surface_t *recording_surface; cairo_box_t bbox; cairo_rectangle_int_t extents; recording_surface = (cairo_recording_surface_t *) surf; - if (_cairo_surface_is_snapshot (&recording_surface->base)) - recording_surface = (cairo_recording_surface_t *) - _cairo_surface_snapshot_get_target (&recording_surface->base); + if (_cairo_surface_is_snapshot (&recording_surface->base)) { + free_me = _cairo_surface_snapshot_get_target (&recording_surface->base); + recording_surface = (cairo_recording_surface_t *) free_me; + } status = _cairo_recording_surface_get_bbox (recording_surface, &bbox, NULL); + cairo_surface_destroy (free_me); if (unlikely (status)) return status; @@ -2848,6 +2851,7 @@ _cairo_ps_surface_emit_recording_surface (cairo_ps_surface_t *surface, cairo_matrix_t old_cairo_to_ps; cairo_content_t old_content; cairo_rectangle_int_t old_page_bbox; + cairo_surface_t *free_me = NULL; cairo_surface_clipper_t old_clipper; cairo_box_t bbox; cairo_int_status_t status; @@ -2862,14 +2866,14 @@ _cairo_ps_surface_emit_recording_surface (cairo_ps_surface_t *surface, _cairo_ps_surface_clipper_intersect_clip_path); if (_cairo_surface_is_snapshot (recording_surface)) - recording_surface = _cairo_surface_snapshot_get_target (recording_surface); + free_me = recording_surface = _cairo_surface_snapshot_get_target (recording_surface); status = _cairo_recording_surface_get_bbox ((cairo_recording_surface_t *) recording_surface, &bbox, NULL); if (unlikely (status)) - return status; + goto err; #if DEBUG_PS _cairo_output_stream_printf (surface->stream, @@ -2907,11 +2911,11 @@ _cairo_ps_surface_emit_recording_surface (cairo_ps_surface_t *surface, CAIRO_RECORDING_REGION_NATIVE); assert (status != CAIRO_INT_STATUS_UNSUPPORTED); if (unlikely (status)) - return status; + goto err; status = _cairo_pdf_operators_flush (&surface->pdf_operators); if (unlikely (status)) - return status; + goto err; _cairo_output_stream_printf (surface->stream, " Q\n"); @@ -2928,7 +2932,9 @@ _cairo_ps_surface_emit_recording_surface (cairo_ps_surface_t *surface, _cairo_pdf_operators_set_cairo_to_pdf_matrix (&surface->pdf_operators, &surface->cairo_to_ps); - return CAIRO_STATUS_SUCCESS; +err: + cairo_surface_destroy (free_me); + return status; } static cairo_int_status_t @@ -2941,6 +2947,7 @@ _cairo_ps_surface_emit_recording_subsurface (cairo_ps_surface_t *surface, cairo_content_t old_content; cairo_rectangle_int_t old_page_bbox; cairo_surface_clipper_t old_clipper; + cairo_surface_t *free_me = NULL; cairo_int_status_t status; old_content = surface->content; @@ -2971,7 +2978,7 @@ _cairo_ps_surface_emit_recording_subsurface (cairo_ps_surface_t *surface, _cairo_output_stream_printf (surface->stream, " q\n"); if (_cairo_surface_is_snapshot (recording_surface)) - recording_surface = _cairo_surface_snapshot_get_target (recording_surface); + free_me = recording_surface = _cairo_surface_snapshot_get_target (recording_surface); if (recording_surface->content == CAIRO_CONTENT_COLOR) { surface->content = CAIRO_CONTENT_COLOR; @@ -2989,11 +2996,11 @@ _cairo_ps_surface_emit_recording_subsurface (cairo_ps_surface_t *surface, CAIRO_RECORDING_REGION_NATIVE); assert (status != CAIRO_INT_STATUS_UNSUPPORTED); if (unlikely (status)) - return status; + goto err; status = _cairo_pdf_operators_flush (&surface->pdf_operators); if (unlikely (status)) - return status; + goto err; _cairo_output_stream_printf (surface->stream, " Q\n"); @@ -3010,7 +3017,9 @@ _cairo_ps_surface_emit_recording_subsurface (cairo_ps_surface_t *surface, _cairo_pdf_operators_set_cairo_to_pdf_matrix (&surface->pdf_operators, &surface->cairo_to_ps); - return CAIRO_STATUS_SUCCESS; +err: + cairo_surface_destroy (free_me); + return status; } static void diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c index e6867c1c7..ab4dcd65c 100644 --- a/src/cairo-script-surface.c +++ b/src/cairo-script-surface.c @@ -1549,7 +1549,7 @@ _emit_surface_pattern (cairo_script_surface_t *surface, { cairo_script_context_t *ctx = to_context (surface); cairo_surface_pattern_t *surface_pattern; - cairo_surface_t *source, *snapshot; + cairo_surface_t *source, *snapshot, *free_me = NULL; cairo_surface_t *take_snapshot = NULL; cairo_int_status_t status; @@ -1568,7 +1568,7 @@ _emit_surface_pattern (cairo_script_surface_t *surface, if (_cairo_surface_snapshot_is_reused (source)) take_snapshot = source; - source = _cairo_surface_snapshot_get_target (source); + free_me = source = _cairo_surface_snapshot_get_target (source); } switch ((int) source->backend->type) { @@ -1585,6 +1585,7 @@ _emit_surface_pattern (cairo_script_surface_t *surface, status = _emit_image_surface_pattern (surface, source); break; } + cairo_surface_destroy (free_me); if (unlikely (status)) return status; diff --git a/src/cairo-surface-snapshot-inline.h b/src/cairo-surface-snapshot-inline.h index 5bf344427..7c15b81a7 100644 --- a/src/cairo-surface-snapshot-inline.h +++ b/src/cairo-surface-snapshot-inline.h @@ -47,7 +47,14 @@ _cairo_surface_snapshot_is_reused (cairo_surface_t *surface) static inline cairo_surface_t * _cairo_surface_snapshot_get_target (cairo_surface_t *surface) { - return ((cairo_surface_snapshot_t *) surface)->target; + cairo_surface_snapshot_t *snapshot = (cairo_surface_snapshot_t *) surface; + cairo_surface_t *target; + + CAIRO_MUTEX_LOCK (snapshot->mutex); + target = cairo_surface_reference (snapshot->target); + CAIRO_MUTEX_UNLOCK (snapshot->mutex); + + return target; } static inline cairo_bool_t diff --git a/src/cairo-surface-snapshot-private.h b/src/cairo-surface-snapshot-private.h index 4c3369ba9..58bee7b49 100644 --- a/src/cairo-surface-snapshot-private.h +++ b/src/cairo-surface-snapshot-private.h @@ -36,12 +36,14 @@ #ifndef CAIRO_SURFACE_SNAPSHOT_PRIVATE_H #define CAIRO_SURFACE_SNAPSHOT_PRIVATE_H +#include "cairo-mutex-private.h" #include "cairo-surface-private.h" #include "cairo-surface-backend-private.h" struct _cairo_surface_snapshot { cairo_surface_t base; + cairo_mutex_t mutex; cairo_surface_t *target; cairo_surface_t *clone; }; diff --git a/src/cairo-surface-snapshot.c b/src/cairo-surface-snapshot.c index 2562d1244..7b8a9a10c 100644 --- a/src/cairo-surface-snapshot.c +++ b/src/cairo-surface-snapshot.c @@ -149,6 +149,8 @@ _cairo_surface_snapshot_copy_on_write (cairo_surface_t *surface) * been lost. */ + CAIRO_MUTEX_LOCK (snapshot->mutex); + if (snapshot->target->backend->snapshot != NULL) { clone = snapshot->target->backend->snapshot (snapshot->target); if (clone != NULL) { @@ -165,7 +167,7 @@ _cairo_surface_snapshot_copy_on_write (cairo_surface_t *surface) if (unlikely (status)) { snapshot->target = _cairo_surface_create_in_error (status); status = _cairo_surface_set_error (surface, status); - return; + goto unlock; } clone = image->base.backend->snapshot (&image->base); _cairo_surface_release_source_image (snapshot->target, image, extra); @@ -174,6 +176,8 @@ done: status = _cairo_surface_set_error (surface, clone->status); snapshot->target = snapshot->clone = clone; snapshot->base.type = clone->type; +unlock: + CAIRO_MUTEX_UNLOCK (snapshot->mutex); } /** @@ -230,6 +234,7 @@ _cairo_surface_snapshot (cairo_surface_t *surface) surface->content); snapshot->base.type = surface->type; + CAIRO_MUTEX_INIT (snapshot->mutex); snapshot->target = surface; snapshot->clone = NULL; diff --git a/src/cairo-surface-subsurface.c b/src/cairo-surface-subsurface.c index 97475f2b4..dfea05c4a 100644 --- a/src/cairo-surface-subsurface.c +++ b/src/cairo-surface-subsurface.c @@ -302,7 +302,8 @@ _cairo_surface_subsurface_source (void *abstract_surface, cairo_surface_t *source; source = _cairo_surface_get_source (surface->target, extents); - *extents = surface->extents; + if (extents) + *extents = surface->extents; return source; } diff --git a/src/cairo-surface.c b/src/cairo-surface.c index 58db7be75..46ba2564e 100644 --- a/src/cairo-surface.c +++ b/src/cairo-surface.c @@ -961,6 +961,8 @@ cairo_surface_finish (cairo_surface_t *surface) /* We have to becareful when decoupling potential reference cycles */ cairo_surface_reference (surface); _cairo_surface_finish (surface); + + /* XXX need to block and wait for snapshot references */ cairo_surface_destroy (surface); } slim_hidden_def (cairo_surface_finish); @@ -1799,7 +1801,8 @@ cairo_surface_t * _cairo_surface_default_source (void *surface, cairo_rectangle_int_t *extents) { - _cairo_surface_get_extents(surface, extents); + if (extents) + _cairo_surface_get_extents(surface, extents); return surface; } diff --git a/src/cairo-traps-compositor.c b/src/cairo-traps-compositor.c index 519727bff..f8308ad44 100644 --- a/src/cairo-traps-compositor.c +++ b/src/cairo-traps-compositor.c @@ -1127,10 +1127,7 @@ is_recording_pattern (const cairo_pattern_t *pattern) return FALSE; surface = ((const cairo_surface_pattern_t *) pattern)->surface; - if (_cairo_surface_is_paginated (surface)) - surface = _cairo_paginated_surface_get_recording (surface); - if (_cairo_surface_is_snapshot (surface)) - surface = _cairo_surface_snapshot_get_target (surface); + surface = _cairo_surface_get_source (surface, NULL); return _cairo_surface_is_recording (surface); } @@ -1140,11 +1137,7 @@ recording_pattern_get_surface (const cairo_pattern_t *pattern) cairo_surface_t *surface; surface = ((const cairo_surface_pattern_t *) pattern)->surface; - if (_cairo_surface_is_paginated (surface)) - surface = _cairo_paginated_surface_get_recording (surface); - if (_cairo_surface_is_snapshot (surface)) - surface = _cairo_surface_snapshot_get_target (surface); - return surface; + return _cairo_surface_get_source (surface, NULL); } static cairo_bool_t