Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

On 11.03.19 г. 15:44 ч., Slavomir Kaslev wrote:
> On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware)
> <y.karadz@gmail.com> wrote:
>>
>>
>>
>> On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote:
>>> On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>>>>
>>>> const char *lib = plugin.toStdString().c_str();
>>>>
>>>> This line is a bad idea because the returned array may (will) be
>>>> invalidated when the destructor of std::string is called.
>>>>
>>>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>>>> ---
>>>>    kernel-shark/src/KsUtils.cpp | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
>>>> index 34b2e2d..d7b1753 100644
>>>> --- a/kernel-shark/src/KsUtils.cpp
>>>> +++ b/kernel-shark/src/KsUtils.cpp
>>>> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
>>>>
>>>>           auto lamRegUser = [&kshark_ctx](const QString &plugin)
>>>>           {
>>>> -               const char *lib = plugin.toStdString().c_str();
>>>> +               const char *lib = plugin.toLocal8Bit().data();
>>>>                   kshark_register_plugin(kshark_ctx, lib);
>>>>           };
>>>>
>>>> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
>>>>
>>>>           auto lamUregUser = [&kshark_ctx](const QString &plugin)
>>>>           {
>>>> -               const char *lib = plugin.toStdString().c_str();
>>>> +               const char *lib = plugin.toLocal8Bit().data();
>>>>                   kshark_unregister_plugin(kshark_ctx, lib);
>>>>           };
>>>
>>> Doesn't the new version have the same problem with the temporary QByteArray?
>>>
>>> How about saving the data in a local std::string/QByteArray variable?
>>>
>>>                  std::string lib = plugin.toStdString();
>>>                  kshark_register_plugin(kshark_ctx, lib.c_str());
>>>
>>
>> Hi Slavi,
>>
>> As far I can understand toStdString() will create a fresh std::string
>> and this string will has its own copy of the characters. However, this
>> copy gets out-of-scope as soon as we hit the semicolon of the line.
>>
>> My understanding was that toLocal8Bit().data() makes no copy so the
>> array will go out-of-scope only when the QString is destroyed.
> 
> I did some digging into QString's implementation. From my reading of
> the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which
> does allocation, copying/transform-to-latin1 and returns the
> QByteArray. So it seems that toLocal8Bit() is still making a copy.
> 
> [1] https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275
> 
> Cheers,
> 

Thanks a lot for investigating this!
I think in this case it will be more appropriate if you sign the patch.
cheers,
Y.

> -- Slavi
> 
>>
>> But anyway, your solution looks cleaner and safer.
>> I will send another version of the patch.
>> Thanks a lot!
>> Y.
>>
>>
>>> Thanks!
>>>
>>>
>>> --
>>> Slavomir Kaslev
>>>
> 
> 
>