Bug 71507

Summary:	ABI breakage: Qt segfaults when run against libxcb 1.9.3
Product:	XCB	Reporter:	Uli Schlachter <psychon>
Component:	Library	Assignee:	xcb mailing list dummy <xcb>
Status:	RESOLVED FIXED	QA Contact:	xcb mailing list dummy <xcb>
Severity:	blocker
Priority:	medium	CC:	devurandom, kensington, me, nikoamia
Version:	1.9.3
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Bug Depends on:
Bug Blocks:	71752

Description Uli Schlachter 2013-11-11 18:42:36 UTC

As seen elsewhere[0], Qt5 segfaults when run against the latest xcb release.

Thanks to Tommalla from #qt-labs, I got the following backtrace for this crash:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff0ae2120 in QXcbKeyboard::updateVModMapping() () from /usr/lib64/qt5/plugins/platforms/libqxcb.so
(gdb) bt
#0  0x00007ffff0ae2120 in QXcbKeyboard::updateVModMapping() () from /usr/lib64/qt5/plugins/platforms/libqxcb.so
#1  0x00007ffff0ae25e8 in QXcbKeyboard::QXcbKeyboard(QXcbConnection*) () from /usr/lib64/qt5/plugins/platforms/libqxcb.so
#2  0x00007ffff0adef90 in QXcbConnection::QXcbConnection(QXcbNativeInterface*, bool, char const*) () from /usr/lib64/qt5/plugins/platforms/libqxcb.so
#3  0x00007ffff0ae11ba in QXcbIntegration::QXcbIntegration(QStringList const&, int&, char**) () from /usr/lib64/qt5/plugins/platforms/libqxcb.so
#4  0x00007ffff0af1790 in QXcbIntegrationPlugin::create(QString const&, QStringList const&, int&, char**) () from /usr/lib64/qt5/plugins/platforms/libqxcb.so
[...]

I asked him to tell gdb to "disassemble" and got the following snippet:

   0x00007ffff0ae2118 <+232>:   and    %eax,%r15d
   0x00007ffff0ae211b <+235>:   mov    0x78(%rsp),%rax
=> 0x00007ffff0ae2120 <+240>:   mov    (%rax,%rdx,4),%edx
   0x00007ffff0ae2123 <+243>:   callq  0x7ffff0ade5c0 <_ZN14QXcbConnection8atomNameEj>

So this segfaults right before calling QXcbConnection::atomName(unsigned int)[1].

The relevant code is here: [2]

Because it segfaults right before calling atomName(), it must be dying in line 1064 where it just indexes into the result of xcb_xkb_get_names_value_list_unpack().

If you want me to guess, then the size of this struct changed because of commit 37d0f55392d68d0a05dcf5d793d729e49108f1b7. This introduced a new alignment_pad field and thus shifted all the following members around by four byte. (Having just written this sentence, I am pretty sure that this is the reason for the crash).

[0]:
https://bugreports.qt-project.org/browse/QTBUG-34748
https://bbs.archlinux.org/viewtopic.php?id=172746
https://bugs.freedesktop.org/show_bug.cgi?id=71502

[1]:
$ c++filt _ZN14QXcbConnection8atomNameEj
QXcbConnection::atomName(unsigned int)

[2]: https://qt.gitorious.org/qt/qtbase/source/fd619946be51784dc709363324897be6af144c52:src/plugins/platforms/xcb/qxcbkeyboard.cpp#L1019

Comment 1 Uli Schlachter 2013-11-11 19:43:44 UTC

#qt-labs:

<+peppe> psychon: I think you've nailed it, abicc complains about new fields in struct xcb_xkb_get_map_map_t and struct xcb_xkb_get_names_value_list_t

Comment 2 Ran Benita 2013-11-11 21:16:44 UTC

Hmm clearly I did not anticipate this. I've just retested with xcb-proto and libxcb master, and the GetNames request is definitely not usable without the alignment_pad, so the patch does work as intended. However, if it breaks existing users (especially something like Qt) it should clearly be reverted. So FWIW, have my ack on reverting, as the patch author.

Comment 3 Julien Cristau 2013-11-12 08:24:50 UTC

I think if those requests weren't usable before the change, then bumping libxcb-xkb's SONAME would be a better way out than reverting.

Comment 4 Ran Benita 2013-11-12 21:44:19 UTC

(In reply to comment #3)
> I think if those requests weren't usable before the change, then bumping
> libxcb-xkb's SONAME would be a better way out than reverting.

I was thinking to suggest this after the dust settles. I would personally prefer it of course, as I do need the info in these requests (though the prospect of using them seemed a bit brighter 6 months ago).

Another option might be to deprecate the old symbols and add new ones. But that's a whole can of worms no one wants to open.

So hopefully one fix or the other can go in a short-term release, along with the python3 tab fix on the list?

Comment 5 Gatis Paeglis 2013-11-18 11:19:06 UTC

I added a commend regarding this in https://bugreports.qt-project.org/browse/QTBUG-34748

Comment 6 Uli Schlachter 2013-12-21 13:57:48 UTC

commit b30b11ac49d934541312b03c41d1ab83047a59f4
Author: Uli Schlachter <psychon@znc.in>
Date:   Mon Nov 18 20:28:08 2013 +0100

    Increment the "current" version info for sync, xinput and xkb
    
    Sync: Due to commit e6a246e50e62cbcba3 "sync: Change value list param of
    CreateAlarm and ChangeAlarm into switch", various symbols disappeared,
    for example xcb_sync_{change,create}_alarm_sizeof.
    
    xinput: This extension was updated from version 1.4 to 2.3. This means
    that lots of new things are generated. However, this change is
    backwards-compatible and thus age gets set to 1.
    
    xkb: In commit 37d0f55392d6 "xkb: Work around alignment problems in
    GetNames and GetMap replies", some padding fields were introduced into
    structures for which an _unpack() function is generated. This changed
    the size of the struct and caused offsets into this struct to change.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=71507
    
    Signed-off-by: Uli Schlachter <psychon@znc.in>
    Signed-off-by: Julien Cristau <jcristau@debian.org>

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.