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

Mon Apr 29 18:22:23 CEST 2019

Hi,

The C++11 standard states that:

4.10 - 2 (pointer conversion)
A prvalue of type “pointer to cv T,” where T is an object type, can be 
converted to a pr value of type “pointer to cv void”. The result of 
converting a “pointer to cv T” to a “pointer to cv void” points to the 
start of the storage location where the object of type T resides, as if 
the object is a most derived object (1.8) of type T(that is, not a base 
class subobject). The null pointer value is converted to the null 
pointer value of the destination type.

5.2.9 - 13 (static cast)
A pr value of type “pointer to cv1 void” can be converted to a pr value 
of type “pointer to cv2 T,” where T is an object type and cv2 is the 
same cv-qualification as, or greater cv-qualification than,cv1. The null 
pointer value is converted to the null pointer value of the destination 
type. A value of type pointer to object converted to “pointer to cv 
void” and back, possibly with different cv-qualification, shall have its 
original value

So, if I'm not mistaken, this is OK to cast back and forth to void*

On 2019-04-29 17:08, Rémi Denis-Courmont wrote:
> 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é.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel