[vlc-devel] [PATCH] gui/qt: bookmarks: fix psz_name lifetime

[Romain Vimont]

Le vendredi 10 novembre 2017 à 17:04 +0100, Filip Roséen a écrit :
> Hi Romain,
> 
> I agree with what you write (in terms of behavior), but the relevant
> piece of implementation should go through a more robust clean-up than
> what you propose (especially given that your fix is incorrect).
> 
> On 2017-11-10 16:55, Romain Vimont wrote:
> 
> > The macro qtu() is defined as follows:
> > 
> >     #define qtu( i ) ((i).toUtf8().constData())
> > 
> > "i" is a QString, .toUtf8() returns a QByteArray, .constData() returns a
> > pointer to the data inside the QByteArray.
> > 
> > This macro is used for the initialization of bookmark.psz_name, which
> > suffered from two problems.
> > 
> > Firstly, the pointer returned by .constData() is owned by the
> > QByteArray, so it must not be freed explicitly. This is not compatible
> > with how the bookmark is used (e.g. psz_name may be freed by
> > BooksmarksDialog::edit()).
> > 
> > Secondly, in the definition of qtu(), the QByteArray is a temporary
> > value. Therefore, it is "destroyed as the last step in evaluating the
> > full-expression that (lexically) contains the point where [it was]
> > created".
> > 
> > Concretely, this means that this call is correct:
> > 
> >     do_something( qtu( string ) );
> > 
> > But this one is undefined behavior:
> > 
> >     const char *s = qtu( string );
> >     do_something( s );
> > 
> > Thus, here, bookmark.psz_name was initialized with a pointer to garbage
> > data.
> > 
> > To fix both problems, just strdup() the string.
> > 
> > (Fixes invalid reads reported by valgrind)
> > 
> > Signed-off-by: Romain Vimont <rom at rom1v.com>
> > ---
> > Note: without looking at the definition of qtu(), it is not obvious that the
> > result of qtu(str) has a shorter lifetime than str. Maybe its name should be
> > more explicit about this (qtu_tmp() or something).
> > 
> >  modules/gui/qt/dialogs/bookmarks.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/modules/gui/qt/dialogs/bookmarks.cpp b/modules/gui/qt/dialogs/bookmarks.cpp
> > index c3c29a02fe..fd5fca147a 100644
> > --- a/modules/gui/qt/dialogs/bookmarks.cpp
> > +++ b/modules/gui/qt/dialogs/bookmarks.cpp
> > @@ -168,7 +168,7 @@ void BookmarksDialog::add()
> >      {
> >          QString name = THEMIM->getIM()->getName() + " #"
> >                       + QString::number( bookmarksList->topLevelItemCount() );
> > -        bookmark.psz_name = const_cast<char *>qtu( name );
> > +        bookmark.psz_name = strdup( qtu( name ) );
> 
> You are now leaking the result of `strdup`, `INPUT_ADD_BOOKMARK` does
> not take ownership of the passed indirect values of `seekpoint_t`.

You're right, I don't know why I assumed that it took ownership
(probably because I saw other parts of code freing the bookmark
psz_name, but this was in other contexts). Then my first point is also
wrong.

So, the "quick fix" consists in not using qtu() there, and create a
local variable for the QByteArray.

> 
> >  
> >          input_Control( p_input, INPUT_ADD_BOOKMARK, &bookmark );
> >      }
> 
> Best Regards,
> Filip

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel