[cairo] [PATCH] Improve handling of contexts with finished surface
Chris Wilson
chris at chris-wilson.co.uk
Mon Aug 12 08:43:19 PDT 2013
On Mon, Aug 12, 2013 at 04:51:11PM +0200, Uli Schlachter wrote:
> Hi everyone,
>
> in response to Bug 68014 "Writing on a finished surface." [0], I came up with
> the attached patches.
>
> The first patch enhances the api-special-cases test so that it also tests the
> behavior of functions receiving a cairo_t* and the other patches fix various
> problems that these new tests uncovered.
>
> I would be happy about comments on these patches.
>
> Cheers,
> Uli
>
> [0]: https://bugs.freedesktop.org/show_bug.cgi?id=68014
> --
> "Do you know that books smell like nutmeg or some spice from a foreign land?"
> -- Faber in Fahrenheit 451
> From ec399258c888befec1b174da85e6853250b2c7af Mon Sep 17 00:00:00 2001
> From: Uli Schlachter <psychon at znc.in>
> Date: Mon, 12 Aug 2013 14:30:59 +0200
> Subject: [PATCH 1/4] api-special-cases: Also test contexts
>
> This adds code to the api-special-cases test which also tests the behavior of
> cairo when the cairo context or the surface that is target is in an error state
> or finished. These new tests call into all public entry points defined in
> cairo.h which receive a cairo_t * as their first argument.
>
> Currently this causes a new crash in the testsuite:
>
> cairo-surface.c:394:
> _cairo_surface_begin_modification: Assertion `! surface->finished' failed.
>
Reported-by: christophe.troestler at umons.ac.be
References: https://bugs.freedesktop.org/show_bug.cgi?id=68014
> Signed-off-by: Uli Schlachter <psychon at znc.in>
The test cases look good to me.
> From 603b5a8fcdb20d833dda21ecd56e0e4544599899 Mon Sep 17 00:00:00 2001
> From: Uli Schlachter <psychon at znc.in>
> Date: Mon, 12 Aug 2013 15:40:00 +0200
> Subject: [PATCH 2/4] surface: Error out on finished surfaces
>
> Finished surfaces and surfaces with an error status must not be usable anymore,
> so refuse to work on them.
>
> This improves the result for api-special-cases.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68014
>
> Signed-off-by: Uli Schlachter <psychon at znc.in>
> ---
> src/cairo-surface.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/src/cairo-surface.c b/src/cairo-surface.c
> index 5c6969c..c695261 100644
> --- a/src/cairo-surface.c
> +++ b/src/cairo-surface.c
> @@ -1346,6 +1346,12 @@ cairo_surface_supports_mime_type (cairo_surface_t *surface,
> {
> const char **types;
>
> + if (unlikely (surface->status))
> + return surface->status;
return FALSE;
The rest looks reasonable.
> From 92aff06c0a933c77891f1619f7102eb3bd2bf99d Mon Sep 17 00:00:00 2001
> From: Uli Schlachter <psychon at znc.in>
> Date: Mon, 12 Aug 2013 15:59:18 +0200
> Subject: [PATCH 3/4] push_group: Refuse working with unusable surface
>
> Make cairo_push_group() fail when the context's target surface is finished.
>
> This fixes the api-special-cases for the xcb backend:
>
> Detected error during xcb run: error=9, seqno=0x13c, major=53, minor=0
>
> The problem was that the Pixmap for the cairo surface was already freed and
> cairo still tried to use it again as the drawable in a CreatePixmap request.
>
> Signed-off-by: Uli Schlachter <psychon at znc.in>
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> From 50ccf36e7c35a81ff89bda45aad2e0b9e2456694 Mon Sep 17 00:00:00 2001
> From: Uli Schlachter <psychon at znc.in>
> Date: Mon, 12 Aug 2013 16:33:19 +0200
> Subject: [PATCH 4/4] surface_get_extents: Reject finished or error surface
>
> This fixes a crash in the api-special-cases with xlib-xcb when calling
> cairo_clip_extents() on a context that refers to a finished surface.
>
> The crash was a simple NULL pointer dereference, because the underlying xcb
> surface that was used in xlib-xcb was gone and set to NULL already.
>
> Signed-off-by: Uli Schlachter <psychon at znc.in>
> ---
> src/cairo-surface.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/cairo-surface.c b/src/cairo-surface.c
> index c695261..1c2d39f 100644
> --- a/src/cairo-surface.c
> +++ b/src/cairo-surface.c
> @@ -2368,7 +2368,9 @@ _cairo_surface_get_extents (cairo_surface_t *surface,
> cairo_bool_t bounded;
>
> bounded = FALSE;
> - if (surface->backend->get_extents != NULL)
> + if (likely (surface->status == CAIRO_STATUS_SUCCESS) &&
> + likely (! surface->finished) &&
> + surface->backend->get_extents != NULL)
> bounded = surface->backend->get_extents (surface, extents);
I think I prefer
if (unlikely (surface->status))
goto zero_extents;
if (unlikely (surface->finished)) {
_cairo_surface_set_error(surface, CAIRO_STATUS_SURFACE_FINISHED);
goto zero_extents;
}
bounded = FALSE;
if (surface->backend->get_extents)
bounded = surface->backend->get_extents (surface, extents);
< blah >
return bounded;
zero_extents:
memset (extents, 0, sizeof(*extents));
return TRUE;
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
More information about the cairo
mailing list