avoid crash-on-exit issues during global destruction in stock Qt
ClosedPublic

Authored by rjvbb on May 3 2017, 7:04 PM.

Details

Reviewers
yuyichao
Summary

As discussed in https://bugs.kde.org/show_bug.cgi?id=363753 :

Qt has a long-standing issue where attempts to disconnect from DBus during global destruction can happen when the QtDBus internals have already been destroyed. A Qt patch exists that protects against this situation but until further notice it has to be applied by users and distribution maintainers.

There is another potential issue during global destruction if no disconnection from DBus is performed (a usual approach): DBus signals can arrive and be processed when the style plugin has already been dlclose'd. This jumps into unmapped memory space, leading to a crash.

This revision proposes a patch based on Debian's workaround. It attempts to prevent the DBus issues on exit in 2 ways:

  • disconnect in reaction to QCoreApplication::aboutToQuit()
  • delete all Style instances before unloading the plugin to avoid dangling references to unmapped memory.

The 2nd version of the patch includes a fix for a regression in Qt 5.9.0 (possibly a latent issue already present in earlier versions) that will be fixed only in Qt 5.9.1 . This is not related to DBus but is also caused by insufficient checking to avoid accessing (global) structures after they were deleted.

Test Plan

Testing has been done by me on Mac and Linux with Qt 5.5.x through 5.8.0; the Debian patch has evidently been tested in the wild on Debian systems and a more recent version also in KaOS.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.May 3 2017, 7:04 PM
rjvbb updated this revision to Diff 14187.May 6 2017, 9:29 AM
rjvbb edited the summary of this revision. (Show Details)

This version introduces another fix, making this patch a combined fix for 2 possible crashes on exit that are both caused by invoking Qt functionality after its internal structure has already been deleted in the global destruction phase.

This 2nd issue was discovered in Qt 5.9.0 by Eugene Shalygin and will be fixed only in Qt 5.9.1 . The fix proposed here follows the same principle as also used for DBus disconnection: the event notify callback is no longer required when the application is about to quit so the unregisterCallback() call can be made at that time.

rjvbb retitled this revision from avoid a DBus disconnect related crash at exit in stock Qt to avoid crash-on-exit issues during global destruction in stock Qt.May 6 2017, 9:34 AM
rjvbb edited the summary of this revision. (Show Details)
rjvbb set the repository for this revision to R626 QtCurve.
yuyichao accepted this revision.May 6 2017, 1:14 PM

Only very minor comments.

qt5/style/qtcurve.h
362

BTW, is it still necessary to declare this a Q_SLOTS when the new syntax is used?

qt5/style/qtcurve_plugin.cpp
196

Does int atLibClose() __attribute__((destructor)) { ...; } not work? (I think that's how I've been using it).

This revision is now accepted and ready to land.May 6 2017, 1:14 PM
rjvbb added inline comments.May 6 2017, 1:42 PM
qt5/style/qtcurve.h
362

slots works too instead - it is replaced with Q_SLOTS *if* QT_NO_SIGNALS_SLOTS_KEYWORDS is not defined.

Apparently Q_SLOTS is only when you want to use a 3rd-party slot/signal mechanism so we should be able to use slots.

qt5/style/qtcurve_plugin.cpp
196

The attribute would have to be before the function declaration, as with the hot attribute used elsewhere in the file. I had been meaning to align to that syntax.

yuyichao added inline comments.May 6 2017, 1:50 PM
qt5/style/qtcurve.h
362

I mean is slot even necessary? I think it's only when you want to use SLOT(disconnectDBus()) to connect slot since it need to do a string lookup. Connecting using function pointer shouldn't need this.

qt5/style/qtcurve_plugin.cpp
196

Yeah, don't think I actually tried putting it after. Just that repeating this twice seems a little strange......

rjvbb closed this revision.May 6 2017, 1:55 PM
rjvbb marked 3 inline comments as done.
qt5/style/qtcurve.h
362

Maybe not in the connect() statement but it could still be required for the moc.

I don't think I've ever seen slots or signals that were NOT declared as such in the class definition, not even in private code. We could investigate this separately, it's a different (cleanup) issue anyway.

yuyichao added inline comments.May 6 2017, 2:35 PM
qt5/style/qtcurve.h
362

it could still be required for the moc.

moc is only needed if you use SLOT on it.

I don't think I've ever seen slots or signals that were NOT declared as such in the class definition

It's used a lot here. The only Q_SLOTS in the qt5 version are ones that does not have a equivalent connect function that takes a function pointer.