Re: [PATCH 5/5] GetCrtcGamma: Fix error handling.

[Jamey Sharp]

On Mon, Nov 9, 2009 at 2:56 PM, Adam Jackson <ajax@...> wrote:

> We didn't treat _XReply failure as fatal.  Parsing an xError as a gamma
> ramp reply doesn't work that often.
>
> Signed-off-by: Adam Jackson <ajax@...>
> ---
>  src/XrrCrtc.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/src/XrrCrtc.c b/src/XrrCrtc.c
> index 3f048e2..697987a 100644
> --- a/src/XrrCrtc.c
> +++ b/src/XrrCrtc.c
> @@ -179,7 +179,7 @@ XRRGetCrtcGamma (Display *dpy, RRCrtc crtc)
>     XExtDisplayInfo        *info = XRRFindDisplay(dpy);
>     xRRGetCrtcGammaReply    rep;
>     xRRGetCrtcGammaReq     *req;
> -    XRRCrtcGamma           *crtc_gamma;
> +    XRRCrtcGamma           *crtc_gamma = NULL;
>     long                   nbytes;
>     long                   nbytesRead;
>
> @@ -192,7 +192,7 @@ XRRGetCrtcGamma (Display *dpy, RRCrtc crtc)
>     req->crtc = crtc;
>
>     if (!_XReply (dpy, (xReply *) &rep, 0, xFalse))
> -       rep.status = RRSetConfigFailed;
> +       goto out;
>
>     nbytes = (long) rep.length << 2;
>
> @@ -204,9 +204,7 @@ XRRGetCrtcGamma (Display *dpy, RRCrtc crtc)
>     if (!crtc_gamma)
>     {
>        _XEatData (dpy, (unsigned long) nbytes);
> -       UnlockDisplay (dpy);
> -       SyncHandle ();
> -       return NULL;
> +       goto out;
>     }
>     _XRead16 (dpy, crtc_gamma->red, rep.size * 2);
>     _XRead16 (dpy, crtc_gamma->green, rep.size * 2);
> @@ -214,7 +212,8 @@ XRRGetCrtcGamma (Display *dpy, RRCrtc crtc)
>
>     if (nbytes > nbytesRead)
>        _XEatData (dpy, (unsigned long) (nbytes - nbytesRead));
> -
> +
> +out:
>     UnlockDisplay (dpy);
>     SyncHandle ();
>     return crtc_gamma;
> --
> 1.6.5.2

Reviewed-by: Jamey Sharp <jamey@...>

I pointed this bug out to Keith last week. XCB-based Xlib assert-fails
if you try to read past the end of your reply, as this function does
in the error case, and we got a bug report on the xcb list. I don't
have a test case for your patch but it looks correct to me.

I also reviewed the other four patches in this series. Feel free to
include my Reviewed-by in those as well.

Jamey
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Adam Jackson]

On Mon, 2009-11-09 at 17:01 -0800, Jamey Sharp wrote:

> Reviewed-by: Jamey Sharp <jamey@...>
>
> I pointed this bug out to Keith last week. XCB-based Xlib assert-fails
> if you try to read past the end of your reply, as this function does
> in the error case, and we got a bug report on the xcb list. I don't
> have a test case for your patch but it looks correct to me.

dispwin(1) from argyllcms would trigger this.  If you had a connected or
unknown-connection output with no associated CRTC, it would attempt to
GetCrtcGamma on the associated CRTC (ie, None).  X would rightly throw
an error, we'd parse it as a reply, assert.  

See also https://bugzilla.redhat.com/show_bug.cgi?id=498931

> I also reviewed the other four patches in this series. Feel free to
> include my Reviewed-by in those as well.

Thanks!

- ajax


_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb