Re: [PATCH] xcb: Fix error reporting if fallback fails

On Tue, May 30, 2017 at 06:24:22PM +0200, Uli Schlachter wrote:
> If we cannot let the X11 server do some operation (for example: the
> RENDER extension is not available), then we fall back to an image
> surface and do the operation locally instead. This fallback requires the
> current content of the surface to be downloaded from the X11 server.
> This fallback logic had an error.
> 
> The fallback is implemented with _get_image() in the function
> _cairo_xcb_surface_fallback(). _get_image() is only called if we do not
> yet have a fallback available, so when we call _get_image we have
> surface->fallback == NULL. Then, if _get_image() fails, it returns a
> surface in an error state.
> 
> Before this patch, the code would then just ignore this error surface
> and return &surface->fallback->base, a NULL pointer. This would then
> quickly cause a crash when e.g. the surface's ->status member is
> accessed.
> 
> Fix this by returning the error surface instead as the fallback.
> 
> The end result of this patch will be that the XCB surface that is
> currently drawn to ends up in an error state which is a lot better than
> a NULL pointer dereference and actually correct in this case. The error
> state is reached because the current drawing operation will fail and
> this error is reported up the call stack and eventually "taints" the
> surface.
> 
> (However, the error code could be better: _get_image() too often fails
> with a generic CAIRO_STATUS_NO_MEMORY error, but that's left as future
> work)
> 
> Signed-off-by: Uli Schlachter <[email protected]>
> ---
> 
> [No test for this since in the case were I ran into this, _get_image()
> failed because the X11 server closed the connection. No idea why it
> did that.]

Steps to repro the issue (or a bug # if it's been reported) might be
nice, but the change looks sensible enough so:

Reviewed-by: Bryce Harrington <[email protected]>

>  src/cairo-xcb-surface.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/cairo-xcb-surface.c b/src/cairo-xcb-surface.c
> index b319b7f24..39127c148 100644
> --- a/src/cairo-xcb-surface.c
> +++ b/src/cairo-xcb-surface.c
> @@ -833,12 +833,13 @@ _cairo_xcb_surface_fallback (cairo_xcb_surface_t *surface,
>      image = (cairo_image_surface_t *)
>  	    _get_image (surface, TRUE, 0, 0, surface->width, surface->height);
>  
> +    if (image->base.status != CAIRO_STATUS_SUCCESS)
> +	return &image->base;
> +
>      /* If there was a deferred clear, _get_image applied it */
> -    if (image->base.status == CAIRO_STATUS_SUCCESS) {
> -	surface->deferred_clear = FALSE;
> +    surface->deferred_clear = FALSE;
>  
> -	surface->fallback = image;
> -    }
> +    surface->fallback = image;
>  
>      return &surface->fallback->base;
>  }
> -- 
> 2.11.0
> -- 
> cairo mailing list
> [email protected]
> https://lists.cairographics.org/mailman/listinfo/cairo
-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo