Ubuntu
qtbase-opensource-src package

Qt5 windows may be randomly unmapped due to assumption sizeof(long)==4

Bug #1251262 reported by Bruce Merry
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
qtbase-opensource-src (Ubuntu)
Fix Released
Undecided
Alberto Mardegan

Bug Description

The patch debian/patches/0001-Implement-XEmbed-protocol.patch adds XEmbed protocol support, but it has some undefined behaviour bugs on LP64 systems like x86-64. In particular, the _XEMBED_INFO property is defined as two CARD32 values (http://standards.freedesktop.org/xembed-spec/xembed-spec-latest.html#id2877439), but it is cast as "long" in some places e.g.

+ /* Add XEMBED info; this operation doesn't initiate the embedding. */
+ long data[] = { XEMBED_VERSION, XEMBED_MAPPED };
+ Q_XCB_CALL(xcb_change_property(xcb_connection(), XCB_PROP_MODE_REPLACE, m_window,
+ atom(QXcbAtom::_XEMBED_INFO),
+ atom(QXcbAtom::_XEMBED_INFO),
+ 32, 2, (void *)data));

...

+ const xcb_get_property_cookie_t get_cookie =
+ xcb_get_property(xcb_connection(), 0, m_window, xEmbedInfoAtom,
+ XCB_ATOM_ANY, 0, 3);
+
+ xcb_get_property_reply_t *reply =
+ xcb_get_property_reply(xcb_connection(), get_cookie, NULL);
+ if (reply && reply->length >= 2) {
+ const long *data = (const long *)xcb_get_property_value(reply);
+ if (data[1] & XEMBED_MAPPED)
+ Q_XCB_CALL(xcb_map_window(xcb_connection(), m_window));
+ else
+ Q_XCB_CALL(xcb_unmap_window(xcb_connection(), m_window));
+ }

I discovered this when some code I compiled with -fsanitize=address would pop up a window for an instant before it was unmapped again - since the reply only contains two 32-bit words (I confirmed with a debugger than reply->length == 2 and reply->format == 32), data[1] has undefined contents. On the sending side, it is actually sending {0, 0} rather than the intended {0, 1}. Changing "long" to "quint32" made the problem go away.

I don't know if this is a complete fix - there are other places where 'long' is used and I don't know enough about XCB to know whether they're broken or not (and I'd never heard of XEmbed until a few hours ago... I'm definitely not an expert on this stuff).

Incidentally, I also have no idea why the call to xcb_get_property passes 3 as the length, when only 2 words are expected or examined.

I used ubuntu-bug so hopefully it will pick up all the relevant information about my system, but just in case: I'm running 13.10 on x86-64, and I'm building from qtbase-opensource-src_5.0.2+dfsg1-7ubuntu11.

ProblemType: Bug
DistroRelease: Ubuntu 13.10
Package: libqt5gui5 5.0.2+dfsg1-7ubuntu11
ProcVersionSignature: Ubuntu 3.11.0-13.20-generic 3.11.6
Uname: Linux 3.11.0-13-generic x86_64
NonfreeKernelModules: nvidia
ApportVersion: 2.12.5-0ubuntu2.1
Architecture: amd64
Date: Thu Nov 14 16:13:16 2013
InstallationDate: Installed on 2011-05-25 (904 days ago)
InstallationMedia: Ubuntu 11.04 "Natty Narwhal" - Release amd64 (20110426)
MarkForUpload: True
SourcePackage: qtbase-opensource-src
UpgradeStatus: Upgraded to saucy on 2013-10-25 (19 days ago)

Revision history for this message
Bruce Merry (bmerry) wrote :
Revision history for this message
Dmitry Shachnev (mitya57) wrote :

Hey Alberto, can you please confirm that "const long *data" should be changed to "const quint32 *data"? Are there other places where a similar change should be done?

Changed in qtbase-opensource-src (Ubuntu):
assignee: nobody → Alberto Mardegan (mardy)
Revision history for this message
Alberto Mardegan (mardy) wrote :

Yes, indeed it should be quint32. Note that I mostly ported the code from Qt4, and it was "long" there.
Also, notice that this patch went upstream since Qt 5.1, so the code should be fixed upstream too.

I filed https://bugreports.qt-project.org/browse/QTBUG-34861, I'll try to get some time to work on that.

Changed in qtbase-opensource-src (Ubuntu):
status: New → Confirmed
Revision history for this message
Launchpad Janitor (janitor) wrote :
Download full text (13.3 KiB)

This bug was fixed in the package qtbase-opensource-src - 5.2.1+dfsg-1ubuntu7

---------------
qtbase-opensource-src (5.2.1+dfsg-1ubuntu7) trusty; urgency=medium

  [ Colin Watson ]
  * Add arm64 to archs that don't use -m64

qtbase-opensource-src (5.2.1+dfsg-1ubuntu6) trusty; urgency=medium

  * Add Use-None-instead-of-GLX_NONE.patch:
    - Cherry-pick upstream patch (LP: #1288278)

qtbase-opensource-src (5.2.1+dfsg-1ubuntu5) trusty; urgency=medium

  * Only run tests on armhf, amd64 and i386.

qtbase-opensource-src (5.2.1+dfsg-1ubuntu4) trusty; urgency=medium

  [ Chris Gagnon ]
  * Enable unit tests

qtbase-opensource-src (5.2.1+dfsg-1ubuntu3) trusty; urgency=medium

  * Revert the transitional package change final landing.

qtbase-opensource-src (5.2.1+dfsg-1ubuntu2) trusty; urgency=medium

  * libqt5core5 transitional package to be able to run ABI related tests

qtbase-opensource-src (5.2.1+dfsg-1ubuntu1) trusty; urgency=low

  [ Dmitry Shachnev ]
  * Update watch file (taken from Debian).
  * Fix generating documentation by building qdoc before using it.
  * Remove qtcreator.qdoc from qtbase5-doc.install, as it is already in
    qtbase5-dev.install.
  * Merge with Debian up to 5.2.0~beta1+dfsg-3.
    - Fixes build failures on powerpc and armel.
  * Add debian/patches/fix_cppcodemarker_crash.patch to fix qdoc
    crash that caused ubuntu-ui-toolkit to FTBFS (LP: #1217331).

  [ Łukasz 'sil2100' Zemczak ]
  * Cherry-pick two submitted patches to support appmenu-qt: (LP: #1157213)
    - make_qkdetheme_constructor_public.diff
    - platformtheme_env.diff

  [ Timo Jyrinki ]
  * New upstream release 5.2.1 (LP: #1256341) (LP: #1223032) (LP: #1222988)
    (LP: #1223042) (LP: #1253120) (LP: #1251262)
  * Sync with Debian 5.2.0+dfsg-7, remaining changes:
    - Remove firebird and ibase dependencies
    - Maintainer fields and Vcs-Bzr
    - No gdb required on ppc64el
    - Provides: qt-default to qt5-default
    - Define explicit list on which archs openvg required
    - Additional patches:
      + disable_overlay_scrollbars.diff
      + load_testability_from_env_var.patch
      + make_qkdetheme_constructor_public.diff
      + platformtheme_env.diff
      + qdoc-Fix-crash-in-Generator-generateInnerNode.patch
      + 0001-Do-not-overwrite-basePixmap-of-QIconLoader-PixmapEnt.patch
    - Use our symbols files
    - Additional multi-arch packages (not correct policy-wise)
  * Drop upstream patches:
    - add_since_52_to_new_QColor_features.patch
    - fix_cppcodemarker_crash.patch
    - fix_usr-move_workaround_in_the_presence_of_multi-arch.patch
    - make_QColor_understand_AARRGGBB.patch
    - Add-workaround-for-GL-on-Android-emulator.patch
    - 0001-Do-not-overwrite-basePixmap-of-QIconLoader-PixmapEnt.patch
    - fix_destroy_qapp_segfault.diff
  * Remove Ubuntu patches:
    - enable_appmenu_support.diff (obsolete)
    - 0001-Implement-XEmbed-protocol.patch (submitted and merged upstream)
    - fix_maliit_activation.patch (not used anymore)
    - inputmethod_fix_focusout.patch (not used anymore)
    - fix_number_precision_qjsondocument.patch_8e8becdc.patch (upstream)
    - bug1227629.patch (merged upstream)
    - fix_rowinserted.patch (LP: #1242630...

Changed in qtbase-opensource-src (Ubuntu):
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.