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

Mon Apr 29 19:08:16 CEST 2019

Hi,

That text says that you can convert from T* to void* and back. Of course, that's possible. But that patch changes the code so that it converts from T* to void* and then forth.

Keep in mind that C++ is a piece of s**t where pointer equality is not an equivalence relation, and pointer conversions do not compose.

Le 29 avril 2019 18:22:23 GMT+02:00, pierre at videolabs.io a écrit :
>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

-- 
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/bd367607/attachment.html>