[vlc-commits] [Git][videolan/vlc][master] 3 commits: thumbnail: rework API to fix use-after-free

Steve Lhomme (@robUx4) gitlab at videolan.org
Fri Jan 27 08:32:41 UTC 2023



Steve Lhomme pushed to branch master at VideoLAN / VLC


Commits:
ca3b1315 by Thomas Guillem at 2023-01-27T08:18:54+00:00
thumbnail: rework API to fix use-after-free

Before 3b26eefc99ea6f0e0e7c268fcfe4d8ba0a788e33, the caller was not
responsible to destroy the request but he could cancel it via:

 vlc_thumbnailer_Cancel()
 WaitForTheCb()
 ReleaseResourceAssociatedWithTheCb()

This new commit, in addition with
3b26eefc99ea6f0e0e7c268fcfe4d8ba0a788e33 (that was not complete), allow
the user to cancel/destroy the request, without waiting for any
callback:

 vlc_thumbnailer_DestroyRequest()
 ReleaseResourceAssociatedWithTheCb()

vlc_thumbnailer_Cancel() has been renamed to
vlc_thumbnailer_DestroyRequest(), this new call must always be called to
release resources and can be called before receiving the callback in
order to cancel it.

Fixes #27766

- - - - -
5418c4a0 by Thomas Guillem at 2023-01-27T08:18:54+00:00
lib: remove libvlc_media_thumbnail_request_cancel

Use libvlc_media_thumbnail_request_destroy() to cancel *and* destroy the
thumbnail request.

- - - - -
8c31fe0d by Thomas Guillem at 2023-01-27T08:18:54+00:00
lib: update comment with last API changes

- - - - -


8 changed files:

- include/vlc/libvlc_media.h
- include/vlc_thumbnailer.h
- lib/libvlc.sym
- lib/media.c
- modules/misc/medialibrary/Thumbnailer.cpp
- src/input/thumbnailer.c
- src/libvlccore.sym
- test/src/input/thumbnail.c


Changes:

=====================================
include/vlc/libvlc_media.h
=====================================
@@ -704,8 +704,9 @@ typedef enum libvlc_thumbnailer_seek_speed_t
 /**
  * \brief libvlc_media_request_thumbnail_by_time Start an asynchronous thumbnail generation
  *
- * If the request is successfully queued, the libvlc_MediaThumbnailGenerated
- * is guaranteed to be emitted.
+ * If the request is successfully queued, the libvlc_MediaThumbnailGenerated is
+ * guaranteed to be emitted (except if the request is destroyed early by the
+ * user).
  * The resulting thumbnail size can either be:
  * - Hardcoded by providing both width & height. In which case, the image will
  *   be stretched to match the provided aspect ratio, or cropped if crop is true.
@@ -723,8 +724,8 @@ typedef enum libvlc_thumbnailer_seek_speed_t
  * \param timeout A timeout value in ms, or 0 to disable timeout
  *
  * \return A valid opaque request object, or NULL in case of failure.
- * It may be cancelled by libvlc_media_thumbnail_request_cancel().
- * It must be released by libvlc_media_thumbnail_request_destroy().
+ * It must be released by libvlc_media_thumbnail_request_destroy() and
+ * can be cancelled by calling it early.
  *
  * \version libvlc 4.0 or later
  *
@@ -742,8 +743,9 @@ libvlc_media_thumbnail_request_by_time( libvlc_instance_t *inst,
 /**
  * \brief libvlc_media_request_thumbnail_by_pos Start an asynchronous thumbnail generation
  *
- * If the request is successfully queued, the libvlc_MediaThumbnailGenerated
- * is guaranteed to be emitted.
+ * If the request is successfully queued, the libvlc_MediaThumbnailGenerated is
+ * guaranteed to be emitted (except if the request is destroyed early by the
+ * user).
  * The resulting thumbnail size can either be:
  * - Hardcoded by providing both width & height. In which case, the image will
  *   be stretched to match the provided aspect ratio, or cropped if crop is true.
@@ -761,7 +763,6 @@ libvlc_media_thumbnail_request_by_time( libvlc_instance_t *inst,
  * \param timeout A timeout value in ms, or 0 to disable timeout
  *
  * \return A valid opaque request object, or NULL in case of failure.
- * It may be cancelled by libvlc_media_thumbnail_request_cancel().
  * It must be released by libvlc_media_thumbnail_request_destroy().
  *
  * \version libvlc 4.0 or later
@@ -777,23 +778,12 @@ libvlc_media_thumbnail_request_by_pos( libvlc_instance_t *inst,
                                        bool crop, libvlc_picture_type_t picture_type,
                                        libvlc_time_t timeout );
 
-/**
- * @brief libvlc_media_thumbnail_cancel cancels a thumbnailing request
- * @param p_req An opaque thumbnail request object.
- *
- * Cancelling the request will still cause libvlc_MediaThumbnailGenerated event
- * to be emitted, with a NULL libvlc_picture_t
- * If the request is cancelled after its completion, the behavior is undefined.
- */
-LIBVLC_API void
-libvlc_media_thumbnail_request_cancel( libvlc_media_thumbnail_request_t *p_req );
-
 /**
  * @brief libvlc_media_thumbnail_destroy destroys a thumbnail request
  * @param p_req An opaque thumbnail request object.
  *
- * If the request has not completed or hasn't been cancelled yet, the behavior
- * is undefined
+ * This will also cancel the thumbnail request, no events will be emitted after
+ * this call.
  */
 LIBVLC_API void
 libvlc_media_thumbnail_request_destroy( libvlc_media_thumbnail_request_t *p_req );


=====================================
include/vlc_thumbnailer.h
=====================================
@@ -73,10 +73,10 @@ enum vlc_thumbnailer_seek_speed
  * \return An opaque request object, or NULL in case of failure
  *
  * If this function returns a valid request object, the callback is guaranteed
- * to be called, even in case of later failure.
- * The returned request object must not be used after the callback has been
- * invoked. That request object is owned by the thumbnailer, and must not be
- * released.
+ * to be called, even in case of later failure (except if destroyed early by
+ * the user).
+ * The returned request object must be freed with
+ * vlc_thumbnailer_DestroyRequest().
  * The provided input_item will be held by the thumbnailer and can safely be
  * released safely after calling this function.
  */
@@ -98,10 +98,10 @@ vlc_thumbnailer_RequestByTime( vlc_thumbnailer_t *thumbnailer,
  * \return An opaque request object, or NULL in case of failure
  *
  * If this function returns a valid request object, the callback is guaranteed
- * to be called, even in case of later failure.
- * The returned request object must not be used after the callback has been
- * invoked. That request object is owned by the thumbnailer, and must not be
- * released.
+ * to be called, even in case of later failure (except if destroyed early by
+ * the user).
+ * The returned request object must be freed with
+ * vlc_thumbnailer_DestroyRequest().
  * The provided input_item will be held by the thumbnailer and can safely be
  * released after calling this function.
  */
@@ -113,16 +113,16 @@ vlc_thumbnailer_RequestByPos( vlc_thumbnailer_t *thumbnailer,
                               vlc_thumbnailer_cb cb, void* user_data );
 
 /**
- * \brief vlc_thumbnailer_Cancel Cancel a thumbnail request
+ * \brief vlc_thumbnailer_DestroyRequest Destroy a thumbnail request
  * \param thumbnailer A thumbnailer object
  * \param request An opaque thumbnail request object
  *
- * Cancelling a request will invoke the completion callback with a NULL picture
- * The behavior is undefined if the request is cancelled after its completion.
+ * The request can be destroyed before receiving a callback (in that case, the
+ * callback won't be called) or after (to release resources).
  */
 VLC_API void
-vlc_thumbnailer_Cancel( vlc_thumbnailer_t* thumbnailer,
-                        vlc_thumbnailer_request_t* request );
+vlc_thumbnailer_DestroyRequest( vlc_thumbnailer_t* thumbnailer,
+                                vlc_thumbnailer_request_t* request );
 
 /**
  * \brief vlc_thumbnailer_Release releases a thumbnailer and cancel all pending requests


=====================================
lib/libvlc.sym
=====================================
@@ -83,7 +83,6 @@ libvlc_media_get_user_data
 libvlc_media_get_parsed_status
 libvlc_media_thumbnail_request_by_time
 libvlc_media_thumbnail_request_by_pos
-libvlc_media_thumbnail_request_cancel
 libvlc_media_thumbnail_request_destroy
 libvlc_media_track_hold
 libvlc_media_track_release


=====================================
lib/media.c
=====================================
@@ -1073,18 +1073,12 @@ libvlc_media_thumbnail_request_by_pos( libvlc_instance_t *inst,
     return req;
 }
 
-// Cancel a thumbnail request
-void libvlc_media_thumbnail_request_cancel( libvlc_media_thumbnail_request_t *req )
-{
-    libvlc_priv_t *p_priv = libvlc_priv(req->instance->p_libvlc_int);
-    assert( p_priv->p_thumbnailer != NULL );
-    vlc_thumbnailer_Cancel( p_priv->p_thumbnailer, req->req );
-}
-
 // Destroy a thumbnail request
 void libvlc_media_thumbnail_request_destroy( libvlc_media_thumbnail_request_t *req )
 {
-    libvlc_media_thumbnail_request_cancel( req );
+    libvlc_priv_t *p_priv = libvlc_priv(req->instance->p_libvlc_int);
+    assert( p_priv->p_thumbnailer != NULL );
+    vlc_thumbnailer_DestroyRequest( p_priv->p_thumbnailer, req->req );
     libvlc_media_release( req->md );
     libvlc_release(req->instance);
     free( req );


=====================================
modules/misc/medialibrary/Thumbnailer.cpp
=====================================
@@ -107,7 +107,8 @@ void Thumbnailer::stop()
     vlc::threads::mutex_locker lock{ m_mutex };
     if ( m_currentContext != nullptr )
     {
-        vlc_thumbnailer_Cancel( m_thumbnailer.get(), m_currentContext->request );
+        vlc_thumbnailer_DestroyRequest( m_thumbnailer.get(),
+                                        m_currentContext->request );
         m_currentContext->done = true;
         m_cond.signal();
     }


=====================================
src/input/thumbnailer.c
=====================================
@@ -32,9 +32,6 @@ struct vlc_thumbnailer_t
 {
     vlc_object_t* parent;
     vlc_executor_t *executor;
-
-    vlc_mutex_t lock;
-    struct vlc_list submitted_tasks; /**< list of struct thumbnailer_task */
 };
 
 struct seek_target
@@ -57,6 +54,7 @@ typedef struct vlc_thumbnailer_request_t task_t;
 
 struct vlc_thumbnailer_request_t
 {
+    vlc_atomic_rc_t rc;
     vlc_thumbnailer_t *thumbnailer;
 
     struct seek_target seek_target;
@@ -81,8 +79,6 @@ struct vlc_thumbnailer_request_t
     picture_t *pic;
 
     struct vlc_runnable runnable; /**< to be passed to the executor */
-
-    struct vlc_list node; /**< node of vlc_thumbnailer_t.submitted_tasks */
 };
 
 static void RunnableRun(void *);
@@ -96,6 +92,7 @@ TaskNew(vlc_thumbnailer_t *thumbnailer, input_item_t *item,
     if (!task)
         return NULL;
 
+    vlc_atomic_rc_init(&task->rc);
     task->thumbnailer = thumbnailer;
     task->item = item;
     task->seek_target = seek_target;
@@ -118,28 +115,14 @@ TaskNew(vlc_thumbnailer_t *thumbnailer, input_item_t *item,
 }
 
 static void
-TaskDelete(task_t *task)
+TaskRelease(task_t *task)
 {
+    if (!vlc_atomic_rc_dec(&task->rc))
+        return;
     input_item_Release(task->item);
     free(task);
 }
 
-static void
-ThumbnailerAddTask(vlc_thumbnailer_t *thumbnailer, task_t *task)
-{
-    vlc_mutex_lock(&thumbnailer->lock);
-    vlc_list_append(&task->node, &thumbnailer->submitted_tasks);
-    vlc_mutex_unlock(&thumbnailer->lock);
-}
-
-static void
-ThumbnailerRemoveTask(vlc_thumbnailer_t *thumbnailer, task_t *task)
-{
-    vlc_mutex_lock(&thumbnailer->lock);
-    vlc_list_remove(&task->node);
-    vlc_mutex_unlock(&thumbnailer->lock);
-}
-
 static void NotifyThumbnail(task_t *task, picture_t *pic)
 {
     assert(task->cb);
@@ -228,8 +211,6 @@ RunnableRun(void *userdata)
     bool notify = task->status != INTERRUPTED;
     vlc_mutex_unlock(&task->lock);
 
-    ThumbnailerRemoveTask(thumbnailer, task);
-
     if (notify)
         NotifyThumbnail(task, pic);
 
@@ -239,12 +220,8 @@ RunnableRun(void *userdata)
     input_Stop(input);
     input_Close(input);
 
-    TaskDelete(task);
-    return;
-
 error:
-    ThumbnailerRemoveTask(thumbnailer, task);
-    TaskDelete(task);
+    TaskRelease(task);
 }
 
 static void
@@ -268,14 +245,10 @@ RequestCommon(vlc_thumbnailer_t *thumbnailer, struct seek_target seek_target,
     if (!task)
         return NULL;
 
-    ThumbnailerAddTask(thumbnailer, task);
-
+    /* One ref for the executor */
+    vlc_atomic_rc_inc(&task->rc);
     vlc_executor_Submit(thumbnailer->executor, &task->runnable);
 
-    /* XXX In theory, "task" might already be invalid here (if it is already
-     * executed and deleted). This is consistent with the API documentation and
-     * the previous implementation, but it is not very convenient for the user.
-     */
     return task;
 }
 
@@ -308,11 +281,20 @@ vlc_thumbnailer_RequestByPos( vlc_thumbnailer_t *thumbnailer,
                          userdata);
 }
 
-void vlc_thumbnailer_Cancel( vlc_thumbnailer_t* thumbnailer, task_t* task )
+void vlc_thumbnailer_DestroyRequest( vlc_thumbnailer_t* thumbnailer, task_t* task )
 {
-    (void) thumbnailer;
-    /* The API documentation requires that task is valid */
-    Interrupt(task);
+    bool canceled = vlc_executor_Cancel(thumbnailer->executor, &task->runnable);
+    if (canceled)
+    {
+        /* Release the executor reference (since it won't run) */
+        bool ret = vlc_atomic_rc_dec(&task->rc);
+        /* Assert that only the caller got the reference */
+        assert(!ret); (void) ret;
+    }
+    else
+        Interrupt(task);
+
+    TaskRelease(task);
 }
 
 vlc_thumbnailer_t *vlc_thumbnailer_Create( vlc_object_t* parent)
@@ -329,38 +311,12 @@ vlc_thumbnailer_t *vlc_thumbnailer_Create( vlc_object_t* parent)
     }
 
     thumbnailer->parent = parent;
-    vlc_mutex_init(&thumbnailer->lock);
-    vlc_list_init(&thumbnailer->submitted_tasks);
 
     return thumbnailer;
 }
 
-static void
-CancelAllTasks(vlc_thumbnailer_t *thumbnailer)
-{
-    vlc_mutex_lock(&thumbnailer->lock);
-
-    task_t *task;
-    vlc_list_foreach(task, &thumbnailer->submitted_tasks, node)
-    {
-        bool canceled = vlc_executor_Cancel(thumbnailer->executor,
-                                            &task->runnable);
-        if (canceled)
-        {
-            NotifyThumbnail(task, NULL);
-            vlc_list_remove(&task->node);
-            TaskDelete(task);
-        }
-        /* Otherwise, the task will be finished and destroyed after run() */
-    }
-
-    vlc_mutex_unlock(&thumbnailer->lock);
-}
-
 void vlc_thumbnailer_Release( vlc_thumbnailer_t *thumbnailer )
 {
-    CancelAllTasks(thumbnailer);
-
     vlc_executor_Delete(thumbnailer->executor);
     free( thumbnailer );
 }


=====================================
src/libvlccore.sym
=====================================
@@ -814,7 +814,7 @@ vlc_encoder_Destroy
 vlc_thumbnailer_Create
 vlc_thumbnailer_RequestByTime
 vlc_thumbnailer_RequestByPos
-vlc_thumbnailer_Cancel
+vlc_thumbnailer_DestroyRequest
 vlc_thumbnailer_Release
 vlc_player_AddAssociatedMedia
 vlc_player_AddListener


=====================================
test/src/input/thumbnail.c
=====================================
@@ -135,23 +135,27 @@ static void test_thumbnails( libvlc_instance_t* p_vlc )
 
         vlc_mutex_lock( &ctx.lock );
 
+        vlc_thumbnailer_request_t* p_req;
         if ( test_params[i].b_use_pos )
         {
-            vlc_thumbnailer_RequestByPos( p_thumbnailer, test_params[i].f_pos,
+            p_req = vlc_thumbnailer_RequestByPos( p_thumbnailer, test_params[i].f_pos,
                 test_params[i].b_fast_seek ?
                     VLC_THUMBNAILER_SEEK_FAST : VLC_THUMBNAILER_SEEK_PRECISE,
                 p_item, test_params[i].i_timeout, thumbnailer_callback, &ctx );
         }
         else
         {
-            vlc_thumbnailer_RequestByTime( p_thumbnailer, test_params[i].i_time,
+            p_req = vlc_thumbnailer_RequestByTime( p_thumbnailer, test_params[i].i_time,
                 test_params[i].b_fast_seek ?
                     VLC_THUMBNAILER_SEEK_FAST : VLC_THUMBNAILER_SEEK_PRECISE,
                 p_item, test_params[i].i_timeout, thumbnailer_callback, &ctx );
         }
+        assert( p_req != NULL );
 
         while ( ctx.b_done == false )
             vlc_cond_wait( &ctx.cond, &ctx.lock );
+
+        vlc_thumbnailer_DestroyRequest( p_thumbnailer, p_req );
         vlc_mutex_unlock( &ctx.lock );
 
         input_item_Release( p_item );
@@ -184,7 +188,7 @@ static void test_cancel_thumbnail( libvlc_instance_t* p_vlc )
         VLC_TICK_INVALID, VLC_THUMBNAILER_SEEK_PRECISE, p_item,
         VLC_TICK_INVALID, thumbnailer_callback_cancel, NULL );
 
-    vlc_thumbnailer_Cancel( p_thumbnailer, p_req );
+    vlc_thumbnailer_DestroyRequest( p_thumbnailer, p_req );
 
     /* Check that thumbnailer_callback_cancel is not called, even after the
      * normal termination of the parsing. */



View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/5b79d6e608950d10928c4056c08c74f6f81c16a7...8c31fe0d623178b0f2e38772a2216ca8754ea9e4

-- 
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/5b79d6e608950d10928c4056c08c74f6f81c16a7...8c31fe0d623178b0f2e38772a2216ca8754ea9e4
You're receiving this email because of your account on code.videolan.org.


VideoLAN code repository instance


More information about the vlc-commits mailing list