QString -> QStringLiteral conversions might make applications crash on exit

[Milian Wolff]

On Friday, February 26, 2016 1:37:57 AM CET Frank Reininghaus wrote:
> Hi everyone,
> 
> sorry if most of you know about this already, but since it seems that
> QStringLiterals are being introduced everywhere right now, I think
> that it is important to raise awareness about the fact that this might
> be more dangerous that it seems at first sight.

make sure you read up on that topic on the Qt devel mailing list - the issue 
you describe here has been spotted with Qt DBUS as well afaik.

<snip>

> This becomes a problem if the read-only data that the QString refers
> to are not there any more, which can happen if the QString was stored
> in a global static object from one library, and the QStringLiteral is
> from another library, which might have been unloaded before the global
> static object was destroyed.
> 
> I believe that this is just what happens right now with a global
> static KIconLoader from kicontnkhemes and a QStringLiteral from
> frameworkintegration:
> 
> https://bugs.kde.org/show_bug.cgi?id=359758
> 
> If my analysis is wrong, please let me know!

Considering that the above bug says it crashes when you do

dolphin --icon <something>

when do you put "<something>" into a QStringLiteral? Considering that this is 
UTF8 data you get at runtime via argv, I doubt that this is a related issue. 
If at all, dolphin should crash always on exit, independent of an argv 
parameter.

> If this crash is really caused by the QStringLiteral, then we should
> think about how we want to treat QStringLiteral in the future. The
> current approach seems to be "use it everywhere", but this might cause
> more crashes in the future.

To debug this issue, use valgrind (sadly Thiago's suggestion on an improved 
diagnostic for the issue hasn't been implemented yet), which should at least 
give a somewhat more explicit backtrace than GDB afaik. Furthermore, in GDB. 
do a "info shared" before shutting down GDB, and once when you hit the crash. 
Then look at where the data that is being deleted (i.e. the QString) is 
stored, and map it to one of the *.so's. If it was a QStringLiteral you will 
find out which lib it was that way.
 
> Possible solutions could be
> 
> a) Everyone who makes QString -> QStringLiteral replacements should be
> extremely careful (which is very difficult, since it is not always
> obvious if passing a QString to a function will result in the string
> being stored in a global static object). Automated tools like clazy
> should then not recommend to use QStringLiteral any more.

This only happens when libraries are unloaded afaik, which should not happen 
afaik during __run_exit_handlers - but I may be wrong in that regard. Can you 
check who's triggering the dlcose?

> b) Classes like KIconLoader, which are used as global static objects,
> should copy all strings that they get to the heap in order to prevent
> such crashes (which might also be quite difficult to do consistently).


> Accidental mistakes are very easy to make either way, so I'm wondering
> if the use of QStringLiteral should be restricted to situations where
> it's 100% clear that there will be no risk (this will often not be the
> case, I think), and it also seems likely that using it will have an
> impact on the performance and/or memory usage (because it's a string
> that will be copied a lot, or because it's in a hot code path).
> 
> Opinions?

In KDevelop we started using QStringLiteral in many places and haven't seen 
any crashes. We definitely use that for QIcon::fromTheme(). So while I agree 
that using QSL may introduce bugs, they are rare. I rather we make the code 
fast by default and fix the very few issues that arise. I.e.: I'm not yet 
convinced that the bug you are seeing is related.

Bye
-- 
Milian Wolff
mail at milianw.de
http://milianw.de