Avoid yet another crash on exit
ClosedPublic

Authored by fvogt on Jul 24 2019, 3:21 PM.

Details

Reviewers
yuyichao
rjvbb
Summary

Apparently it's possible that there are no arguments, especially
if called during teardown from ~QApplication.

Test Plan

Ran pinentry-qt, now does not crash on exit.

Diff Detail

Repository
R626 QtCurve
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14374
Build 14392: arc lint + arc unit
fvogt requested review of this revision.Jul 24 2019, 3:21 PM
fvogt created this revision.
yuyichao accepted this revision.Jul 24 2019, 3:42 PM

Didn't know this can happen and lgtm.

This revision is now accepted and ready to land.Jul 24 2019, 3:42 PM
rjvbb added a comment.Jul 24 2019, 3:55 PM
Didn't know this can happen and lgtm.

Idem (2x).

Out of curiosity: how do you invoke pinentry-qt to trigger the crash, where does the it occur, and what does qApp->arguments()[0] return if there are no arguments? FWIW my own copy of pinentry-qt starts and exits fine, but I don't know how to make it pop up a GUI.

Before I give the green light too, do you also get a crash when you use qApp->arguments().at(0)? This method doesn't work exactly the same way as the [] operator does.

Of course we could also just query qApp->applicationName() ...

fvogt added a comment.Jul 24 2019, 4:52 PM
Didn't know this can happen and lgtm.

Idem (2x).

Out of curiosity: how do you invoke pinentry-qt to trigger the crash, where does the it occur, and what does qApp->arguments()[0] return if there are no arguments? FWIW my own copy of pinentry-qt starts and exits fine, but I don't know how to make it pop up a GUI.

Just pinentry-qt and then BYE. Backtrace and more info on https://bugzilla.opensuse.org/show_bug.cgi?id=1141883

The invalid read "0 bytes after a block of 16 bytes" is visible with valgrind.

Before I give the green light too, do you also get a crash when you use qApp->arguments().at(0)? This method doesn't work exactly the same way as the [] operator does.

I assume so, as arguments is empty both would be invalid.

Of course we could also just query qApp->applicationName() ...

Just `pinentry-qt` and then `BYE`. Backtrace and more info on https://bugzilla.opensuse.org/show_bug.cgi?id=1141883

That backtrace isn't very helpful, since missing most line numbers.

> pinentry-qt --debug 
org.kde.kwindowsystem: Loaded plugin "/opt/local/share/qt5/plugins/kf5/org.kde.kwindowsystem.platforms/KF5WindowSystemX11Plugin.so" for platform "xcb"
OK Pleased to meet you
BYE
OK closing connection
I assume so, as arguments is empty both would be invalid.

Not necessarily, QStringList::at(0) could return an empty QString instance if the list is empty.

Your comment https://bugzilla.opensuse.org/show_bug.cgi?id=1141883#c17 does make wonder if OpenSuse has the latest QtCurve version (or more specifically, if the proposed change is the only fix/workaround for the issue in the latest QtCurve version from git?

Not necessarily, QStringList::at(0) could return an empty QString instance if the list is empty.

No. Also that's basically impossible from the return type (a reference).

fvogt added a comment.Jul 25 2019, 9:43 AM

Your comment https://bugzilla.opensuse.org/show_bug.cgi?id=1141883#c17 does make wonder if OpenSuse has the latest QtCurve version (or more specifically, if the proposed change is the only fix/workaround for the issue in the latest QtCurve version from git?

Tumbleweed has the latest released version and the crash happens also with git master.

While I narrowed down the root cause of argc suddenly being empty to a dangling pointer in pinentry, I'd still say that this patch is correct (or might be extended to connectDBus() as well).

OK.

Would you mind checking if using QCoreApplication::applicationName() would be an alternative? I'd do it myself but since I cannot reproduce your issue I can't answer the question fully.

There's another alternative: caching the information ourselves. That would remove all possibilities of race conditions during global destruction when we can no longer rely on Qt's own data.

or might be extended to connectDBus() as well

Do you mean (or have any indication that) arguments() can be empty when the application is starting up?

fvogt added a comment.Jan 30 2020, 8:06 AM

Hm, totally missed the notification for this...

OK.

Would you mind checking if using QCoreApplication::applicationName() would be an alternative? I'd do it myself but since I cannot reproduce your issue I can't answer the question fully.

At worst it would return a null QString, so should work fine, yes.

There's another alternative: caching the information ourselves. That would remove all possibilities of race conditions during global destruction when we can no longer rely on Qt's own data.

or might be extended to connectDBus() as well

Do you mean (or have any indication that) arguments() can be empty when the application is starting up?

Unless argc is 0 when constructing the QCoreApplication, I don't think so.

rjvbb closed this revision.EditedJan 30 2020, 9:33 AM

Go figure... I had exactly this kind of crash the other day, despite your proposed change.

Turns out this can still happen in applications that do not destroy the Q*Application instance before exitting or returning from main(). Which BTW is the official way to do things, even if it isn't documented explicitly.

Since that is something we cannot control I have indeed decided to use cached information. The application name was already being cached in a global variable that's used for application-specific behaviour, I guess you could say it should have been used for that throughout QtCurve.

Fixed in e996040b6972d4da5d743761ec8cb3b71dff9925