Bug 71060

So the error is xcb-util-cursor creating a pixmap of size 0x0. This must be coming from xcb_cursor_load_cursor():

 http://cgit.freedesktop.org/xcb/util-cursor/tree/cursor/load_cursor.c#n227

So something is missing a 0-check somewhere.


Thinking "how does libXcursor handle this", I found the following. It's the only 0-check I saw and no idea if this is the right one:

 http://cgit.freedesktop.org/xorg/lib/libXcursor/tree/src/file.c#n425

while xcb-util-cursor doesn't do anything equivalent to this:

 http://cgit.freedesktop.org/xcb/util-cursor/tree/cursor/parse_cursor_file.c#n148

Since I have a hard time imagining that such a broken cursor file could be laying around anywhere, I hope someone can figure out where this broken data is coming from.

Perhaps the fix is as easy as checking for too small (and also for too large? libXcursor does that) width/height in parse_cursor_file().

Created attachment 88374 [details] [review]
Sanity check for image too small

I believe the patch check at the right place that the image can have at least one pixel.

(In reply to comment #2)
> Created attachment 88374 [details] [review] [review]
> Sanity check for image too small
> 
> I believe the patch check at the right place that the image can have at
> least one pixel.
Thanks for providing that patch. Did you verify that it makes things work on your system, i.e. with the bad cursor file? If so, I’ll be happy to merge it. Please let me know :).

Nope, the patch didn't help. 
I still don't know exactly what the problem is. Thing work fine if I force xcb_cursor_load to return XCB_NONE, but it's not a proper fix :)

I have some trouble to reproduce too. awesome can either crash fast with a lua backtrace, crash slowly with lot of X error or simply freeze.

(In reply to comment #4)
> Nope, the patch didn't help. 
> I still don't know exactly what the problem is. Thing work fine if I force
> xcb_cursor_load to return XCB_NONE, but it's not a proper fix :)
> 
> I have some trouble to reproduce too. awesome can either crash fast with a
> lua backtrace, crash slowly with lot of X error or simply freeze.
I’ll not apply the patch then, for now.

Here are a few lines of C code to use xcb-util-cursor to set the X root window cursor. Maybe that helps to reproduce the problem: http://sprunge.us/AjSA

You could also test whether you can reproduce the problem with i3 instead of awesome; both use xcb-util-cursor.

Thank, the lines from sprunge.us help, I can now reproduce every time.

With version 0.1.0, both freshly compiled and from arch repo, I get a segmentation fault on xcb_disconnect just after an error "free(): invalid next size (fast): 0x0847d090".
With the git version, freshly pulled, it's a segfault on the malloc at parse_cursor_file.c:168. numpixels, at line 167, have a value of 576 (24*24).

valgrind give more interesting data. He complain about "Syscall param read(buf) point to unaddressable byte(s)". Following trace lead me to parse_cursor_file.c:92, a call to read. When I try to print cf.header value, I get coherent data, like 0x902a038, same as cf. But both &cf and &(cf.header) give 0xffffffff.

Created attachment 88787 [details] [review]
Check that read() succeeds

Created attachment 88788 [details] [review]
Check that malloc() succeeds

The error messages from valgrind don't make much sense to me. That sounds like a broken stack pointer which shouldn't really be possible. However, the other error messages sound like the stack was smashed and everything went downhill. Perhaps compiling with -fstack-protector-all can help narrowing this down...

Anyway, the message about read() made me wonder about short reads, thus my first patch adds error checking to all calls to read(). This second patch now also checks the result of malloc() (all calloc() calls are already checked).

Also, I kept a tradition alive: Every time I look at xcb-util-cursor, I find memory leaks. See the comments in the patches.

@ludovic: Could you test if one of these two patches helps your problem or if I am just finding random issues?

Also, perhaps you could run the test program under strace -e trace=file so that we can finally figure out which file contains the broken cursor images.

Created attachment 88910 [details]
fix for loop

Sorry Uli, I couldn't apply the patch, but I think the first one is obsolete.
This patch fix the segfault but the code from sprunge.us seems to do nothing. The cursor is a black cross and doesn't change. 
 
With this patch, error from awesome are more reliable. Always X error with bad drawable. 

I have no strace on my computer, but I know that xcb-util-cursor try to open $HOME/.icons/default/cursors/+argv[1]. I do most of the test with this file > http://dl.dropboxusercontent.com/u/39506370/left_ptr

(In reply to comment #9)
> Created attachment 88910 [details]
> fix for loop
> 
> Sorry Uli, I couldn't apply the patch, but I think the first one is obsolete.
> This patch fix the segfault but the code from sprunge.us seems to do
> nothing. The cursor is a black cross and doesn't change. 
>  
> With this patch, error from awesome are more reliable. Always X error with
> bad drawable. 
I don’t think that patch is correct. Instead, can you try the following (on top of latest git) please?

--- i/cursor/parse_cursor_file.c
+++ w/cursor/parse_cursor_file.c
@@ -138,7 +138,7 @@ int parse_cursor_file(xcb_cursor_context_t *c, const int fd, xcint_image_t **ima
     for (int n = 0; n < cf.header.ntoc; n++) {
         xcint_chunk_header_t chunk;
         /* for convenience */
-        xcint_image_t *i = &((*images)[n]);
+        xcint_image_t *i = &((*images)[cnt]);
         uint32_t numpixels = 0;
         uint32_t *p = NULL;
 
> I have no strace on my computer, but I know that xcb-util-cursor try to open
> $HOME/.icons/default/cursors/+argv[1]. I do most of the test with this file
> > http://dl.dropboxusercontent.com/u/39506370/left_ptr
Then install strace please?

Created attachment 88928 [details]
strace -e trace=file ./cursor left_ptr

hey, it's working fine with &((*images)[cnt]); :)

(In reply to comment #11)
> Created attachment 88928 [details]
> strace -e trace=file ./cursor left_ptr
> 
> hey, it's working fine with &((*images)[cnt]); :)
Thanks for confirming!

This is pushed with http://cgit.freedesktop.org/xcb/util-cursor/commit/?id=c6c04f73ebc307f04326de042e496bca84682f63

I’ll do a new release soonish.