Re: [PATCH] Improve handling of contexts with finished surface

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 <[email protected]>
> 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: [email protected]
References: https://bugs.freedesktop.org/show_bug.cgi?id=68014
> Signed-off-by: Uli Schlachter <[email protected]>

The test cases look good to me.

> From 603b5a8fcdb20d833dda21ecd56e0e4544599899 Mon Sep 17 00:00:00 2001
> From: Uli Schlachter <[email protected]>
> 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 <[email protected]>
> ---
>  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 <[email protected]>
> 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 <[email protected]>
Reviewed-by: Chris Wilson <[email protected]>

> From 50ccf36e7c35a81ff89bda45aad2e0b9e2456694 Mon Sep 17 00:00:00 2001
> From: Uli Schlachter <[email protected]>
> 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 <[email protected]>
> ---
>  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
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo