xcb: Fix error reporting if fallback fails

Submitted by Uli Schlachter on May 30, 2017, 4:24 p.m.

Details

Message ID 4c5654ac-be75-efe4-4c44-3138fdd355df@znc.in
State New
Headers show
Series "xcb: Fix error reporting if fallback fails" ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Uli Schlachter May 30, 2017, 4:24 p.m.
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 <psychon@znc.in>
---

[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.]

 src/cairo-xcb-surface.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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;
 }

Comments

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 <psychon@znc.in>
> ---
> 
> [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 <bryce@osg.samsung.com>

>  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
> cairo@cairographics.org
> https://lists.cairographics.org/mailman/listinfo/cairo
On 07.06.2017 02:51, Bryce Harrington wrote:
[...]
>> [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:

By now I figured out:

At my university, there was an ancient version of x11vnc which
mis-applied the maximum request length for X11 requests. Also, instead
of the required BadLength error, it just closed the client's connection.
(I tried to find the fixes for this in some Git repositories, but I
failed. But I did check that this does not happen with newer versions of
x11vnc.)

Thus, when the window manager called awesome tried to set a wallpaper,
the server closed the X11 connection. This caused some following
wallpaper-related things to fail and then cairo dereferenced a NULL pointer.

Since the above was never reported through cairo's bugzilla, there is no
bug number for it. Also, due to "old x11vnc version" I do not have any
"good" reproduction steps.

The best I could offer: Hack the source code so that the fallback fails
and returns a surface in an error state.

> Reviewed-by: Bryce Harrington <bryce@osg.samsung.com>

Sorry, I didn't see this before pushing the patch.

Cheers,
Uli