[vlc-devel] [PATCH] qt: casting to children type in parent constructor is UB
Rémi Denis-Courmont
remi at remlab.net
Mon Apr 29 17:08:33 CEST 2019
Hi,
Well I don't challenge that the static cast between derived and base class is valid. But cast to/from void* still seems wrong to me as before.
Le 29 avril 2019 14:59:23 GMT+02:00, pierre at videolabs.io a écrit :
>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:
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20190429/1a457252/attachment.html>
More information about the vlc-devel
mailing list