QString -> QStringLiteral conversions might make applications crash on exit

[Frank Reininghaus]

Hi Albert,

2016-02-28 11:54 GMT+01:00 Albert Astals Cid:
> El Friday 26 February 2016, a les 01:37:57, Frank Reininghaus va escriure:
>> 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.
>>
>> QStringLiteral has the nice property that it stores the string data in
>> read-only memory and avoids heap allocations when it is used to
>> construct a QString. The QString, and any copies that are made, just
>> contain a pointer to read-only data. There is a very nice overview by
>> Olivier at
>>
>> https://woboq.com/blog/qstringliteral.html
>>
>> However, QString is still a non-POD type, even if it has been
>> constructed with QStringLiteral (or copied from such a QString), so
>> its destructor is being run, which accesses the read-only data, then
>> finds out that it's been made with QStringLiteral, such that no
>> refcount updates and deallocations are needed.
>>
>> 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!
>
> If you know what commit causes it I'd say you have two options:
>  * Find exactly which of the changes in the patch is causing the problem, add
> a test case that crashes and then commit the smallest change that fixes the
> crash
>  * Revert the commit and CC the person that did the commit asking him to be
> more careful.

I could go for option 1 because I already found which QStringLiteral
caused the problem, but I'll try to find out in the next few days if
the workaround that Jan showed can also be used in KIconLoader. This
would prevent that the same problem happens again if QStringLiterals
are also used elsewhere to load icons.

But I'm not 100% sure if there aren't more places in KF5 and KDE
Applications where using QStringLiteral could cause similar problems.

Best regards,
Frank