QString -> QStringLiteral conversions might make applications crash on exit

[Frank Reininghaus]

Hi,

2016-02-29 19:04 GMT+01:00 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.

Yes, I will, thanks!

>> 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.

Maybe it should always crash, but it doesn't ;-) The reporter of
https://bugs.kde.org/show_bug.cgi?id=359758 has found, and I have
confirmed, that this only happens if the "--icon <something>" argument
is used, and only since the QStringLiteral change was made in
frameworkintergration.

The "<something>" is never put into a QStringLiteral, of course. As I
wrote in the bug report last week, the QStringLiteral that causes the
crash is "dialog-close" in frameworkintegration
(src/kstyle/kstyle.cpp). This patch in frameworkintegration fixes the
problem for me:

diff --git a/src/kstyle/kstyle.cpp b/src/kstyle/kstyle.cpp
index 6ba5d51..429ede9 100644
--- a/src/kstyle/kstyle.cpp
+++ b/src/kstyle/kstyle.cpp
@@ -362,7 +362,7 @@ QIcon KStyle::standardIcon(StandardPixmap
standardIcon, const QStyleOption */*op
     case QStyle::SP_DialogSaveButton:
         return QIcon::fromTheme(QStringLiteral("document-save"));
     case QStyle::SP_DialogCloseButton:
-        return QIcon::fromTheme(QStringLiteral("dialog-close"));
+        return QIcon::fromTheme(QString("dialog-close"));
     case QStyle::SP_DialogApplyButton:
         return QIcon::fromTheme(QStringLiteral("dialog-ok-apply"));
     case QStyle::SP_DialogResetButton:

I have no idea why "dialog-close" is the problem. I just found it with
a rather primitive debugging approach, see the bug report.

I also don't know why the --icon argument triggers the problem. Maybe
it affects how the kstyle plugin is loaded and unloaded.

>> 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.

I had already tried Valgrind last week (same backtrace as with gdb and
an "Access not within mapped region" error).

Thanks for the "info shared" hint! The address is close to the memory
region of libKF5Style.so.5, but not quite inside if I'm reading the
output correctly. This is the part of the "info shared" output before
the crash that looks close to the address which caused the crash
(0x7fffdd778a80):

0x00007fffddc44b50  0x00007fffddc4bddb  Yes
/home/kde-frameworks/qt5/qtbase/plugins/platforminputcontexts/libcomposeplatforminputcontextplugin.so
0x00007fffdd9989a0  0x00007fffdd9f93c5  Yes
/home/kde-frameworks/kf5/lib64/plugins/styles/breeze.so
0x00007fffdd7730d0  0x00007fffdd777d9f  Yes
/home/kde-frameworks/kf5/lib64/libKF5Style.so.5
0x00007fffdd5686a0  0x00007fffdd56b144  Yes
/home/kde-frameworks/kf5/lib64/plugins/kf5/FrameworkIntegrationPlugin.so

<snip>

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff4b3bbb0 in load<int> (_q_value=@0x7fffdd778a80: <error
reading variable>)
    at /home/kde-frameworks/qt5/qtbase/src/corelib/thread/qgenericatomic.h:90
90              return _q_value;

If I then run "info shared" again, libKF5Style.so.5 is gone:

<snip>

0x00007fffde875650  0x00007fffde88dbea  Yes
/home/kde-frameworks/kf5/lib64/plugins/platformthemes/KDEPlatformTheme.so
0x00007fffdd5686a0  0x00007fffdd56b144  Yes
/home/kde-frameworks/kf5/lib64/plugins/kf5/FrameworkIntegrationPlugin.so
0x00007fffce1a4d10  0x00007fffce1d7948  Yes (*)     /usr/lib64/libtxc_dxtn.so

If libKF5Style.so.5 is unloaded before the global static KIconLoader,
which has a set containing a QString that was constructed from a
QStringLiteral in kstyle.cpp from frameworkintegration, is destroyed,
then it's probably not surprising that we get a crash.

>> 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.

Well, I am convinced :-) I really cannot see how the QStringLiteral
commit in frameworkintegration cannot be related to this crash if
reverting either the full commit or a single QString -> QStringLiteral
change from the commit makes the crash go away.

I'm not really familiar with the details of how library/plugin loading
and unloading work. If anyone who is can answer the question why using
the "--icon" parameter causes this crash on exit, I would appreciate
that. Then we we would probably know if not using QStringLiteral in
kstyle.cpp is the correct fix, or if there is a better solution.

If something like "do not use QStringLiteral in plugins" will be the
conclusion from this issue, then I'm all for continuing to use
QStringLiteral everywhere else in order to avoid heap allocations.

Thanks and best regards,
Frank