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

[Filip Roséen]

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

>  
>          input_Control( p_input, INPUT_ADD_BOOKMARK, &bookmark );
>      }

Best Regards,
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20171110/7c90611a/attachment.html>