Sanitize signal handling in ksmserver
ClosedPublic

Authored by jpalecek on Nov 6 2017, 12:22 AM.

Details

Summary

The TERM signal handling in ksmserver invokes functions which are not async-signal safe, like Qt functions and C++ destructors. Moreover, the signal handling can occur in other than the main thread, which leads to Qt complaining about functions being invoked from the wrong thread. Such a crash can be seen in a report of https://bugs.kde.org/show_bug.cgi?id=384316.

To fix both of these issues, this change makes the signal handling use the self-pipe trick, which signals the need for termination to the main thread by writing to a special-purpose file descriptor. The main loop then takes care of the termination. This is mostly inspired by http://doc.qt.io/qt-5/unix-signals.html.

Note that QApplication::quit already does what we need when destroying the server, particularly deleting the_server/calling cleanUp().

BTW I noticed the code (here and in XIO error handler) makes quite sure the_server is zeroed. However, other code in ksmserver is not so keen on checking the_server is !0 when using it.

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
jpalecek created this revision.Nov 6 2017, 12:22 AM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 6 2017, 12:22 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
jpalecek edited the summary of this revision. (Show Details)Nov 6 2017, 12:24 AM
jpalecek added a reviewer: Plasma.
jpalecek edited the summary of this revision. (Show Details)

Makes sense.

Do the sockets need closing in the destructor?

Do the sockets need closing in the destructor?

Good point. I assumed the KSMServer instance is practically a singleton (being assigned to global the_server etc.) which is created only once during running of the process. In that case, the sockets would be destroyed anyway so I kind of cut that corner and left it be. It would certainly be cleaner to close them in the destructor.

It would certainly be cleaner to close them in the destructor.

Ping.

graesslin added inline comments.
ksmserver/server.cpp
615 ↗(On Diff #21942)

why the old connect syntax?

jpalecek updated this revision to Diff 32067.Apr 13 2018, 3:28 PM

I finally got round to this. So:

  • this revision stores the socketpair fds in KSMServer instance, so they can be closed later.
  • also, it uses the modern QObject::connect syntax with pmfs instead of strings
  • the patch is refreshed so it applies on current master
davidedmundson accepted this revision.Apr 13 2018, 3:33 PM

Thanks, do you have commit access?

This revision is now accepted and ready to land.Apr 13 2018, 3:33 PM

Thanks, do you have commit access?

Sorry, no.

This revision was automatically updated to reflect the committed changes.