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

[pierre at videolabs.io]

Hi,

This patch move	the destruction	of the callback	to the Derived class 
to	avoid
race conditions when the callback is called during the object 
destruction.

Note that casting from the base class QVLCVariable to the Derived class 
is safe with a
static_cast as QVLCVariable is a non virtual base classe of Derived. 
hence no
pointer fixup is required.


On 2019-04-29 14:57, Pierre Lamot wrote:
> children object isn't built yet.
> 
>   Fixes #22187
> ---
>  modules/gui/qt/adapters/var_common_p.hpp | 18 +++++++
>  modules/gui/qt/adapters/variables.hpp    | 60 ++++++++++++++++--------
>  2 files changed, 59 insertions(+), 19 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..c500187c50 100644
> --- a/modules/gui/qt/adapters/variables.hpp
> +++ b/modules/gui/qt/adapters/variables.hpp
> @@ -112,39 +112,26 @@ 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);
>      }
> 
>      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);
> @@ -154,13 +141,24 @@ public:
>                  BaseType value =
> VLCVarTypeTraits<BaseType>::fromValue(currentvalue);
>                  if (m_value != value)
>                  {
> +                    Derived* derived = static_cast<Derived*>(this);
>                      m_value = value;
>                      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, this);
> +        }
> +    }
> +
> +    void clearObject()
> +    {
> +        if (m_object->get())
> +        {
> +            var_DelCallback( m_object->get(), qtu(m_property),
> value_modified, this );
> +            var_Destroy(m_object->get(), qtu(m_property));
> +            m_object->clear();
>          }
>      }
> 
> @@ -221,6 +219,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 +245,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 +271,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 +298,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: