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

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

On 11.03.19 г. 15:44 ч., Slavomir Kaslev wrote:
> On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware)
> <y.karadz@xxxxxxxxx> wrote:
> > On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote:
> > > On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >    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