[vlc-commits] [Git][videolan/libvlcpp][master] EventManager: Fix potential use-after-free

[Hugo Beauzée-Luyssen]

Hugo Beauzée-Luyssen pushed to branch master at VideoLAN / libvlcpp


Commits:
62abf003 by Hugo Beauzée-Luyssen at 2019-06-20T16:23:25Z
EventManager: Fix potential use-after-free

When move-assigning, or copy-assigning, any object that holds an event
manager & inherits from Internal will have the internal class
copy/move constructed first, causing its internal shared pointer to
be overriden, and in turn the libvlc object to be released.
This is a problem if this object contains an event manager with
registered events, as the event manager will be overriden afterward,
causing its destructor to unregister the events after the libvlc
object it needs to unregister the events has been released.

This commit ensures we hold on to the object associated with the event
manager, and that we explicitely unregister all events before releasing
the object.

- - - - -


7 changed files:

- test/main.cpp
- vlcpp/EventManager.hpp
- vlcpp/Media.hpp
- vlcpp/MediaList.hpp
- vlcpp/MediaListPlayer.hpp
- vlcpp/MediaPlayer.hpp
- vlcpp/RendererDiscoverer.hpp


Changes:

=====================================
test/main.cpp
=====================================
@@ -157,4 +157,6 @@ int main(int ac, char** av)
         std::cout << f.name() << std::endl;
     }
     free(imgBuffer);
+    // Check that we don't use the old media player when releasing its event manager
+    mp = VLC::MediaPlayer{};
 }


=====================================
vlcpp/EventManager.hpp
=====================================
@@ -243,8 +243,25 @@ protected:
  */
 class MediaEventManager : public EventManager
 {
+    private:
+        // Hold on to the object firing events. Otherwise, when copying/move
+        // assigning over its object, the Internal parent class would be copied/moved
+        // first, and only then would the event manager be copied/moved.
+        // This can cause the last instance of the object to be released, and
+        // then used by the event manager as it unregisters its callbacks.
+        Media m_media;
+
     public:
-        MediaEventManager(InternalPtr ptr) : EventManager( ptr ) {}
+        MediaEventManager(InternalPtr ptr, Media m)
+            : EventManager( ptr )
+            , m_media( std::move( m ) )
+        {
+        }
+        ~MediaEventManager()
+        {
+            // Clear the events as long as the underlying VLC object is alive
+            m_lambdas.clear();
+        }
 
         /**
          * @brief onMetaChanged Registers an event called when a Media meta changes
@@ -406,8 +423,18 @@ class MediaEventManager : public EventManager
  */
 class MediaPlayerEventManager : public EventManager
 {
+    private:
+        MediaPlayer m_mp;
     public:
-        MediaPlayerEventManager(InternalPtr ptr) : EventManager( ptr ) {}
+        MediaPlayerEventManager(InternalPtr ptr, MediaPlayer mp)
+            : EventManager( ptr )
+            , m_mp( std::move( mp ) )
+        {
+        }
+        ~MediaPlayerEventManager()
+        {
+            m_lambdas.clear();
+        }
 
         /**
          * \brief onMediaChanged Registers an event called when the played media changes
@@ -820,8 +847,18 @@ class MediaPlayerEventManager : public EventManager
  */
 class MediaListEventManager : public EventManager
 {
+    private:
+        MediaList m_ml;
     public:
-        MediaListEventManager(InternalPtr ptr) : EventManager( ptr ) {}
+        MediaListEventManager(InternalPtr ptr, MediaList ml)
+            : EventManager( ptr )
+            , m_ml( std::move( ml ) )
+        {
+        }
+        ~MediaListEventManager()
+        {
+            m_lambdas.clear();
+        }
 
         /**
          * \brief onItemAdded Registers an event called when an item gets added to the media list
@@ -914,8 +951,18 @@ class MediaListEventManager : public EventManager
  */
 class MediaListPlayerEventManager : public EventManager
 {
+    private:
+        MediaListPlayer m_mlp;
     public:
-        MediaListPlayerEventManager(InternalPtr ptr) : EventManager( ptr ) {}
+        MediaListPlayerEventManager(InternalPtr ptr, MediaListPlayer mlp)
+            : EventManager( ptr )
+            , m_mlp( std::move( mlp ) )
+        {
+        }
+        ~MediaListPlayerEventManager()
+        {
+            m_lambdas.clear();
+        }
 
         template <typename Func>
         RegisteredEvent onPlayed(Func&& f)
@@ -981,8 +1028,18 @@ class MediaDiscovererEventManager : public EventManager
 */
 class RendererDiscovererEventManager : public EventManager
 {
+private:
+    RendererDiscoverer m_rd;
 public:
-    RendererDiscovererEventManager( InternalPtr ptr ) : EventManager(ptr) {}
+    RendererDiscovererEventManager( InternalPtr ptr, RendererDiscoverer rd )
+        : EventManager(ptr)
+        , m_rd( std::move( rd ) )
+    {
+    }
+    ~RendererDiscovererEventManager()
+    {
+        m_lambdas.clear();
+    }
 
     template <typename Func>
     RegisteredEvent onItemAdded( Func&& f )


=====================================
vlcpp/Media.hpp
=====================================
@@ -508,7 +508,7 @@ public:
         if ( m_eventManager == nullptr )
         {
             libvlc_event_manager_t* obj = libvlc_media_event_manager(*this);
-            m_eventManager = std::make_shared<MediaEventManager>( obj );
+            m_eventManager = std::make_shared<MediaEventManager>( obj, *this );
         }
         return *m_eventManager;
     }


=====================================
vlcpp/MediaList.hpp
=====================================
@@ -236,7 +236,7 @@ public:
         if ( m_eventManager == nullptr )
         {
             libvlc_event_manager_t* obj = libvlc_media_list_event_manager( *this );
-            m_eventManager = std::make_shared<MediaListEventManager>( obj );
+            m_eventManager = std::make_shared<MediaListEventManager>( obj, *this );
         }
         return *m_eventManager;
     }


=====================================
vlcpp/MediaListPlayer.hpp
=====================================
@@ -76,7 +76,7 @@ public:
         if ( m_eventManager == nullptr )
         {
             libvlc_event_manager_t* obj = libvlc_media_list_player_event_manager(*this);
-            m_eventManager = std::make_shared<MediaListPlayerEventManager>( obj );
+            m_eventManager = std::make_shared<MediaListPlayerEventManager>( obj, *this );
         }
         return *m_eventManager;
     }


=====================================
vlcpp/MediaPlayer.hpp
=====================================
@@ -149,7 +149,7 @@ public:
         if ( m_eventManager == nullptr )
         {
             libvlc_event_manager_t* obj = libvlc_media_player_event_manager( *this );
-            m_eventManager = std::make_shared<MediaPlayerEventManager>( obj );
+            m_eventManager = std::make_shared<MediaPlayerEventManager>( obj, *this );
         }
         return *m_eventManager;
     }


=====================================
vlcpp/RendererDiscoverer.hpp
=====================================
@@ -95,7 +95,7 @@ public:
         if ( m_eventManager == nullptr )
         {
             libvlc_event_manager_t* obj = libvlc_renderer_discoverer_event_manager( *this );
-            m_eventManager = std::make_shared<RendererDiscovererEventManager>( obj );
+            m_eventManager = std::make_shared<RendererDiscovererEventManager>( obj, *this );
         }
         return *m_eventManager;
     }



View it on GitLab: https://code.videolan.org/videolan/libvlcpp/commit/62abf00340cd49ad54075bb4a9b8b24471a0c22b

-- 
View it on GitLab: https://code.videolan.org/videolan/libvlcpp/commit/62abf00340cd49ad54075bb4a9b8b24471a0c22b
You're receiving this email because of your account on code.videolan.org.