[vlc-devel] [PATCH] qt: casting to children type in parent constructor is UB

Pierre Lamot pierre at videolabs.io
Mon May 6 09:35:16 CEST 2019


  children object isn't built yet.

  Fixes #22187
---
 modules/gui/qt/adapters/var_common_p.hpp | 18 ++++++
 modules/gui/qt/adapters/variables.hpp    | 82 ++++++++++++++++--------
 2 files changed, 73 insertions(+), 27 deletions(-)

diff --git a/modules/gui/qt/adapters/var_common_p.hpp b/modules/gui/qt/adapters/var_common_p.hpp
index 47f8e89ca1..7ac324739b 100644
--- a/modules/gui/qt/adapters/var_common_p.hpp
+++ b/modules/gui/qt/adapters/var_common_p.hpp
@@ -51,6 +51,8 @@ public:
         assert(false);
     }
 
+    virtual void clear() {}
+
     operator bool() const {
         return get() != nullptr;
     }
@@ -81,6 +83,10 @@ public:
         m_object.reset(vout, hold);
     }
 
+    void clear() override {
+        m_object.reset(NULL, false);
+    }
+
 private:
     static void obj_hold( vout_thread_t* obj ) { vout_Hold(obj); }
     static void obj_release( vout_thread_t* obj ) { vout_Release(obj); }
@@ -108,6 +114,10 @@ public:
         m_object = p_intf;
     }
 
+    void clear() override {
+        m_object = NULL;
+    }
+
 private:
     intf_thread_t* m_object;
 };
@@ -133,6 +143,10 @@ public:
         m_object.reset(aout, hold);
     }
 
+    void clear() override {
+        m_object.reset(NULL, false);
+    }
+
 private:
     static void obj_hold( audio_output_t* obj ) { aout_Hold(obj); }
     static void obj_release( audio_output_t* obj ) { aout_Release(obj); }
@@ -157,6 +171,10 @@ public:
         m_object = obj;
     }
 
+    void clear() override {
+        m_object = NULL;
+    }
+
 private:
     vlc_object_t *m_object;
 };
diff --git a/modules/gui/qt/adapters/variables.hpp b/modules/gui/qt/adapters/variables.hpp
index 710880cf23..eae67af5be 100644
--- a/modules/gui/qt/adapters/variables.hpp
+++ b/modules/gui/qt/adapters/variables.hpp
@@ -105,6 +105,13 @@ struct VLCVarTypeTraits<float>
 template<typename Derived, typename BaseType>
 class QVLCVariable : public QObject
 {
+    typedef QVLCVariable<Derived, BaseType> SelfType;
+
+    struct QVLCVariableCRef  {
+        SelfType* self;
+    };
+    static_assert( std::is_pod<QVLCVariableCRef>::value,  "QVLCVariableCRef must be POD");
+
 public:
     template<typename T>
     QVLCVariable(T* object, QString property, QObject* parent)
@@ -112,55 +119,50 @@ public:
         , m_object(new VLCObjectHolderImpl<T>(nullptr))
         , m_property(property)
     {
-        resetObject<T>(object);
-        Derived* derived = static_cast<Derived*>(this);
-        connect(derived, &Derived::valueChangedInternal, this, &QVLCVariable<Derived, BaseType>::onValueChangedInternal, Qt::QueuedConnection);
+        cref.self = this;
     }
 
     virtual ~QVLCVariable()
     {
-        if (m_object->get())
-        {
-            Derived* derived = static_cast<Derived*>(this);
-            var_DelCallback(m_object->get(), qtu(m_property), value_modified, derived);
-            var_Destroy(m_object->get(), qtu(m_property));
-        }
+        assert(m_object->get() == nullptr);
     }
 
     ///change the object beeing observed
     template<typename T>
     void resetObject( T* object )
     {
-        Derived* derived = static_cast<Derived*>(this);
-        if (m_object->get()) {
-            var_DelCallback( m_object->get(), qtu(m_property), value_modified, derived );
-            var_Destroy(m_object->get(), qtu(m_property));
-        }
-
-        m_object->reset( object, true );
-        if (m_object->get())
+        clearObject();
+        if (object)
         {
+            m_object->reset( object, true );
             int type = var_Type(object, qtu(m_property));
             if (type == 0) //variable not found
             {
                 msg_Warn(m_object->get(), "variable %s not found in object", qtu(m_property));
-                m_object->reset((T*)nullptr);
+                m_object->clear();
                 return;
             }
             assert((type & VLC_VAR_CLASS) == VLCVarTypeTraits<BaseType>::var_type);
             vlc_value_t currentvalue;
             if (var_Get(m_object->get(), qtu(m_property), &currentvalue) == VLC_SUCCESS)
             {
-                BaseType value = VLCVarTypeTraits<BaseType>::fromValue(currentvalue);
-                if (m_value != value)
-                {
-                    m_value = value;
-                    emit derived->valueChanged( m_value );
-                }
+                Derived* derived = static_cast<Derived*>(this);
+                m_value = VLCVarTypeTraits<BaseType>::fromValue(currentvalue);
+                emit derived->valueChanged( m_value );
             }
 
             var_Create(m_object->get(), qtu(m_property), VLCVarTypeTraits<BaseType>::var_type);
-            var_AddCallback(m_object->get(), qtu(m_property), value_modified, derived);
+            var_AddCallback(m_object->get(), qtu(m_property), value_modified, &cref);
+        }
+    }
+
+    void clearObject()
+    {
+        if (m_object->get())
+        {
+            var_DelCallback( m_object->get(), qtu(m_property), value_modified, &cref );
+            var_Destroy(m_object->get(), qtu(m_property));
+            m_object->clear();
         }
     }
 
@@ -199,14 +201,16 @@ private:
     //executed on variable thread, this forwards the callback to the UI thread
     static int value_modified( vlc_object_t * object, char const *, vlc_value_t, vlc_value_t newValue, void * data)
     {
-        Derived* that = static_cast<Derived*>(data);
-        emit that->onValueChangedInternal( object, VLCVarTypeTraits<BaseType>::fromValue( newValue ) );
+        QVLCVariableCRef* cref = static_cast<QVLCVariableCRef*>(data);
+        Derived* derived = static_cast<Derived*>(cref->self);
+        emit derived->onValueChangedInternal( object, VLCVarTypeTraits<BaseType>::fromValue( newValue ) );
         return VLC_SUCCESS;
     }
 
     std::unique_ptr<VLCObjectHolder> m_object;
     QString m_property;
     BaseType m_value;
+    QVLCVariableCRef cref;
 };
 
 //specialisation
@@ -221,6 +225,12 @@ public:
     QVLCBool(T* object, QString property, QObject* parent = nullptr)
         : QVLCVariable<QVLCBool, bool>(object, property, parent)
     {
+        resetObject<T>(object);
+        connect(this, &QVLCBool::valueChangedInternal, this, &QVLCBool::onValueChangedInternal, Qt::QueuedConnection);
+    }
+
+    ~QVLCBool() {
+        clearObject();
     }
 
 public slots:
@@ -241,6 +251,12 @@ public:
     QVLCString(T* object, QString property, QObject* parent = nullptr)
         : QVLCVariable<QVLCString, QString>(object, property, parent)
     {
+        resetObject<T>(object);
+        connect(this, &QVLCString::valueChangedInternal, this, &QVLCString::onValueChangedInternal, Qt::QueuedConnection);
+    }
+
+    ~QVLCString() {
+        clearObject();
     }
 
 public slots:
@@ -261,6 +277,12 @@ public:
     QVLCFloat(T* object, QString property, QObject* parent = nullptr)
         : QVLCVariable<QVLCFloat, float>(object, property, parent)
     {
+        resetObject<T>(object);
+        connect(this, &QVLCFloat::valueChangedInternal, this, &QVLCFloat::onValueChangedInternal, Qt::QueuedConnection);
+    }
+
+    ~QVLCFloat() {
+        clearObject();
     }
 
 public slots:
@@ -282,6 +304,12 @@ public:
     QVLCInteger(T* object, QString property, QObject* parent = nullptr)
         : QVLCVariable<QVLCInteger, int64_t>(object, property, parent)
     {
+        resetObject<T>(object);
+        connect(this, &QVLCInteger::valueChangedInternal, this, &QVLCInteger::onValueChangedInternal, Qt::QueuedConnection);
+    }
+
+    ~QVLCInteger() {
+        clearObject();
     }
 
 public slots:
-- 
2.17.1



More information about the vlc-devel mailing list