Bug 71060 - xcb-util-cursor trying to create Pixmap of size 0
Summary: xcb-util-cursor trying to create Pixmap of size 0
Status: RESOLVED FIXED
Alias: None
Product: XCB
Classification: Unclassified
Component: Utils (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: medium normal
Assignee: xcb mailing list dummy
QA Contact: xcb mailing list dummy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-30 16:55 UTC by ludovic coues
Modified: 2013-11-09 14:06 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Sanity check for image too small (936 bytes, patch)
2013-10-30 17:35 UTC, ludovic coues
Details | Splinter Review
Check that read() succeeds (3.88 KB, patch)
2013-11-06 21:50 UTC, Uli Schlachter
Details | Splinter Review
Check that malloc() succeeds (1.09 KB, patch)
2013-11-06 21:58 UTC, Uli Schlachter
Details | Splinter Review
fix for loop (863 bytes, text/plain)
2013-11-08 21:42 UTC, ludovic coues
Details
strace -e trace=file ./cursor left_ptr (2.34 KB, text/plain)
2013-11-09 12:49 UTC, ludovic coues
Details

Description ludovic coues 2013-10-30 16:55:34 UTC
awesome 3.5.2, xcb-util-cursor 0.1.0-5-g0bde33d
xtrace says http://sprunge.us/dhOB

psychon says the bug is in xcb-util-cursor, but doesn't know where the check is needed nor why such "evil" cursor file could exit at all.
libXcursor:src/file.c seems to be doing sanity check.
Comment 1 Uli Schlachter 2013-10-30 17:09:53 UTC
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().
Comment 2 ludovic coues 2013-10-30 17:35:19 UTC
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.
Comment 3 Michael Stapelberg 2013-11-04 22:04:19 UTC
(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 :).
Comment 4 ludovic coues 2013-11-05 19:16:55 UTC
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.
Comment 5 Michael Stapelberg 2013-11-05 23:05:28 UTC
(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.
Comment 6 ludovic coues 2013-11-06 20:12:15 UTC
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.
Comment 7 Uli Schlachter 2013-11-06 21:50:55 UTC
Created attachment 88787 [details] [review]
Check that read() succeeds
Comment 8 Uli Schlachter 2013-11-06 21:58:07 UTC
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.
Comment 9 ludovic coues 2013-11-08 21:42:31 UTC
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
Comment 10 Michael Stapelberg 2013-11-09 10:16:12 UTC
(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?
Comment 11 ludovic coues 2013-11-09 12:49:29 UTC
Created attachment 88928 [details]
strace -e trace=file ./cursor left_ptr

hey, it's working fine with &((*images)[cnt]); :)
Comment 12 Michael Stapelberg 2013-11-09 14:06:19 UTC
(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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.