[Xcb] [PATCH] Perform safety check before trying to load glyph cursor

[Christian Linhart]

Hi Martin,

Thank you for your patch.

One thing:
You return XCB_CURSOR_NONE, but at some other places in this function, XCB_NONE is returned or used for a cursor:
Such as line 195:
    xcb_cursor_t cid = XCB_NONE;
or line 218:
        return XCB_NONE;

I think that this should be consistent.
I also think that your choice, i.e., "XCB_CURSOR_NONE", is the better choice.

Can you please revise your patch, or add a second patch,
so that "XCB_CURSOR_NONE" is used for all cursors throughout this function?
( Of course  the XCB_NONE constants that refer to Pixmaps and GCs should stay XCB_NONE )

Thank you,

Chris

P.S.: The patch for the library in repository "xcb/util-cursor" as far as I can tell.
I think it'd be good to include the name of the library after PATCH in the subject-line.
So the subject-line would look like this:
" [PATCH util-cursor] Perform safety check before trying to load glyph cursor"
This convention is used most of the time on the xcb mailinglist.


On 03/17/15 17:22, mgraesslin at kde.org wrote:
> From: Martin Gräßlin <mgraesslin at kde.org>
>
> The passed in cursor name to xcb_cursor_load_cursor might not match
> one of the predefined font cursor values. Without the check the
> call to create glyph cursor will fail with a BadValue error, but
> the library returns the id allocated for the xcb_cursor_t. A user
> of the library gets a value which looks like a valid cursor, but
> when using it for e.g. a cursor value in xcb_create_window it raises
> a BadCursor error.
> ---
>  cursor/load_cursor.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/cursor/load_cursor.c b/cursor/load_cursor.c
> index 914950a..9edd061 100644
> --- a/cursor/load_cursor.c
> +++ b/cursor/load_cursor.c
> @@ -207,6 +207,8 @@ xcb_cursor_t xcb_cursor_load_cursor(xcb_cursor_context_t *c, const char *name) {
>      if (fd == -1 || core_char > -1) {
>          if (core_char == -1)
>              core_char = cursor_shape_to_id(name);
> +        if (core_char == -1)
> +            return XCB_CURSOR_NONE;
>  
>          cid = xcb_generate_id(c->conn);
>          xcb_create_glyph_cursor(c->conn, cid, c->cursor_font, c->cursor_font, core_char, core_char + 1, 0, 0, 0, 65535, 65535, 65535);