Bug 39383 - X server crashes when restarting KDE from Alt+F2
Summary: X server crashes when restarting KDE from Alt+F2
Status: NEW
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: medium critical
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard: 2012BRB_Reviewed
Keywords: love
Depends on:
Blocks: xserver-1.12 xserver-1.13
  Show dependency treegraph
 
Reported: 2011-07-19 11:15 UTC by Kirill Elagin
Modified: 2012-03-24 11:44 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
backtrace full (2.92 KB, text/plain)
2011-07-19 11:15 UTC, Kirill Elagin
no flags Details
Fix server crash due to invalid images (678 bytes, patch)
2011-10-04 12:10 UTC, Kirill Elagin
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kirill Elagin 2011-07-19 11:15:04 UTC
Created attachment 49306 [details]
backtrace full

X server receives SIGSEGV due to null-pointer dereference. The problem is that pixman_image_create_bits returns NULL and the code does not expect this to happen. It returns NULL because picture's BPP (24) is less than DEPTH (32).

================================
(gdb) b render.c:721
Breakpoint 1 at 0xb775428e: file /var/tmp/portage/x11-base/xorg-server-1.10.3/work/xorg-server-1.10.3/render/render.c, line 721.
(gdb) cond 1 ((*(PicturePtr)pDst)->format >> 24) < (((*(PicturePtr)pDst)->format >> 12) & 0x0f) + (((*(PicturePtr)pDst)->format >> 8) & 0x0f) + (((*(PicturePtr)pDst)->format >> 4) & 0x0f) + (((*(PicturePtr)pDst)->format) & 0x0f)
(gdb) c
Continuing.

/****************
I couldn't trace the problem further. In render.c on line 720 VERIFY_PICTURE returns a picture in pDst. The condition is the inverted one from pixman-bits-image.c on line 1564 (PIXMAN_FORMAT_BPP < PIXMAN_FORMAT_DEPTH). If execution breaks it means that this check will fail later.
****************/

/****************
I press Alt+F2, type “restart” and hit Enter.
****************/

Breakpoint 1, ProcRenderComposite (client=0xb7d6a288) at /var/tmp/portage/x11-base/xorg-server-1.10.3/work/xorg-server-1.10.3/render/render.c:721

(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0xb74d7eae in pixman_image_set_has_client_clip (image=0x0, client_clip=1)
    at /var/tmp/portage/x11-libs/pixman-0.22.2/work/pixman-0.22.2/pixman/pixman-image.c:484
================================

I didn't manage to find out where did this invalid picture come from.
See complete backtrace in the attachment.

xorg-server-1.10.3
KDE 4.6.5
Under Gentoo Linux in qemu-kvm (git)

terminal ~ # uname -a
Linux terminal 2.6.39-hardened #4 SMP PREEMPT Tue Jul 19 13:38:28 MSD 2011 i686 QEMU Virtual CPU version 0.14.50 GenuineIntel GNU/Linux
Comment 1 Jeremy Huddleston Sequoia 2011-10-03 17:29:34 UTC
Is this a regression?  If so, what was the last version you tried that did not 
exhibit this problem?
Comment 2 Kirill Elagin 2011-10-04 00:04:50 UTC
This is a synthetic environment, so there was no previous version. But we'll try to see if this is a regression. The main question is who to blame: KDE or X?

But, anyway, this part of code shouldn't crash.
Comment 3 Jeremy Huddleston Sequoia 2011-10-04 11:37:40 UTC
Please do not change the component and severity.  This is a server crash which warrants P2.  Not blocking 1.12 as this is not known to be a regression.

You seem to have diagnosed the issue a tad.  Would you mind sending a patch for the NULL return check sub-issue and sending to xorg-devel for review?

"""
The problem is that
pixman_image_create_bits returns NULL and the code does not expect this to
happen. It returns NULL because picture's BPP (24) is less than DEPTH (32).
"""

If not, I'll get to it eventually, but I've still got another 1500 bug reports to screen before I start fixing any without patches.

I understand that the underlying cause still needs to be found, but rendering bad or sending an error to the client is better than crashing.
Comment 4 Kirill Elagin 2011-10-04 12:10:56 UTC
Created attachment 51972 [details] [review]
Fix server crash due to invalid images

Sorry I didn't want to change anything, this was not intentionally.

I'm not familiar with X server code at all, so the best thing I can suggest is simply returning NULL. As I can see, this case is handled correctly in top-level functions (e.g. see fb/fbpict.c:65).
Comment 5 Jeremy Huddleston Sequoia 2011-10-04 12:33:41 UTC
Thanks.
Comment 6 Jeremy Huddleston Sequoia 2011-10-04 12:34:40 UTC
I want to bring the NULL check into 1.11, so adding to that tracker for that quick fix.
Comment 7 Jeremy Huddleston Sequoia 2011-10-04 19:47:15 UTC
Thanks, the NULL check is in my for-keith branch and will be pulled to master soon (and then 1.11)
Comment 8 Jeremy Huddleston Sequoia 2011-10-04 23:09:01 UTC
Søren, can you take a look at this one?  

In create_bits_picture in fbpict.c, the call to pixman_image_create_bits looks 
wrong.  What's with that " * sizeof(stride)"?

It was added in:

commit 54e023cec07aa7e392da36e11d0a4667b8341370
Author: Søren Sandmann Pedersen <sandmann@redhat.com>
Date:   Mon Jun 11 09:16:46 2007 -0400

    Don't pass regions to pixman_image_composite() anymore.
Comment 9 Jeremy Huddleston Sequoia 2011-10-14 16:50:55 UTC
The NULL check is in 1.11.  Moving this to 1.12 for the underlying fix.
Comment 10 Søren Sandmann Pedersen 2011-10-16 15:09:37 UTC
The "* sizeof (FbStride)" is correct because the

    fbGetPixmapBitsData(pixmap, bits, stride, bpp);

macro computes stride as "number of FbStride", and pixman expects the stride to be given in number of bytes.

The problem is that something in the X server generates a bogus format with 24 bits per pixel, but 32 bits of channel data (a, r, g, and b).

It is likely that the problem is to be found in PictureCreateDefaultFormats() where the default formats are created. Something there is creating a bogus format which is later used for pictures.
Comment 11 Søren Sandmann Pedersen 2011-10-16 15:14:53 UTC
Alternatively, it may be that some variant of 

    http://patchwork.freedesktop.org/patch/1327/

is what's needed.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.