[vlc-devel] [PATCH] gui/qt: bookmarks: fix psz_name lifetime
filip at atch.se
Fri Nov 10 17:04:09 CET 2017
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
> 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]
> 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
> 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 );
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the vlc-devel