From 2ad1f46105fb9e63c83278bb4d183ecdd874f1e1 Mon Sep 17 00:00:00 2001 From: Ronan Abhamon Date: Mon, 13 Nov 2017 11:11:35 +0100 Subject: [PATCH] fix(ClonableObject): fix a fatal bug in ClonableObject: - Create a A Object - Create a B Object - Copy A in B - Delete B - Use L_Q or access public => Crash --- include/CMakeLists.txt | 7 ++-- include/linphone/utils/general.h | 52 ++++---------------------- include/linphone/utils/traits.h | 57 +++++++++++++++++++++++++++++ src/address/address-p.h | 2 + src/chat/chat-room/chat-room-id.cpp | 2 + src/chat/chat-room/chat-room-id.h | 3 ++ src/db/session/db-session.cpp | 4 +- src/object/clonable-object-p.h | 9 +---- src/object/clonable-object.cpp | 49 +++++++------------------ src/object/clonable-object.h | 16 +++++--- 10 files changed, 104 insertions(+), 97 deletions(-) create mode 100644 include/linphone/utils/traits.h diff --git a/include/CMakeLists.txt b/include/CMakeLists.txt index 31f1c52d6..f78e49c63 100644 --- a/include/CMakeLists.txt +++ b/include/CMakeLists.txt @@ -76,14 +76,14 @@ set(ROOT_HEADER_FILES set(C_API_HEADER_FILES c-address.h c-api.h - c-call.h c-call-cbs.h c-call-stats.h + c-call.h c-callbacks.h - c-chat-message.h c-chat-message-cbs.h - c-chat-room.h + c-chat-message.h c-chat-room-cbs.h + c-chat-room.h c-dial-plan.h c-event-log.h c-participant.h @@ -100,6 +100,7 @@ set(UTILS_HEADER_FILES enum-generator.h general.h magic-macros.h + traits.h utils.h ) diff --git a/include/linphone/utils/general.h b/include/linphone/utils/general.h index b03c23804..3f73757fd 100644 --- a/include/linphone/utils/general.h +++ b/include/linphone/utils/general.h @@ -132,38 +132,11 @@ class ObjectPrivate; friend class Tester; #endif -namespace Private { - // See: http://en.cppreference.com/w/cpp/types/void_t - template struct MakeVoid { - typedef void type; - }; - template - using void_t = typename MakeVoid::type; - - template - struct IsMapContainerImpl : std::false_type {}; - - template - struct IsMapContainerImpl< - T, - void_t< - typename T::key_type, - typename T::mapped_type, - decltype(std::declval()[std::declval()]) - > - > : std::true_type {}; -}; - -// Check if a type is a std container like map, unordered_map... -template -struct IsMapContainer : Private::IsMapContainerImpl::type {}; - // Generic public helper. template< typename R, typename P, - typename C, - typename = typename std::enable_if::value, P>::type + typename C > constexpr R *getPublicHelper (P *object, const C *) { return static_cast(object); @@ -173,22 +146,19 @@ constexpr R *getPublicHelper (P *object, const C *) { template< typename R, typename P, - typename C, - typename = typename std::enable_if::value, P>::type + typename C > -inline R *getPublicHelper (const P *map, const C *context) { - auto it = map->find(context); - L_ASSERT(it != map->cend()); - return static_cast(it->second); +inline R *getPublicHelper (const P &objectSet, const C *) { + auto it = objectSet.cbegin(); + L_ASSERT(it != objectSet.cend()); + return static_cast(*it); } #define L_DECLARE_PUBLIC(CLASS) \ inline CLASS *getPublic () { \ - L_ASSERT(mPublic); \ return getPublicHelper(mPublic, this); \ } \ inline const CLASS *getPublic () const { \ - L_ASSERT(mPublic); \ return getPublicHelper(mPublic, this); \ } \ friend class CLASS; @@ -239,14 +209,6 @@ struct AddConstMirror { return std::static_pointer_cast(Object::getSharedFromThis()); \ } -#define L_USE_DEFAULT_SHARE_IMPL(CLASS, PARENT_CLASS) \ - CLASS::CLASS (const CLASS &src) : PARENT_CLASS(*src.getPrivate()) {} \ - CLASS &CLASS::operator= (const CLASS &src) { \ - if (this != &src) \ - setRef(*src.getPrivate()); \ - return *this; \ - } - // ----------------------------------------------------------------------------- // Wrapper public. // ----------------------------------------------------------------------------- @@ -254,7 +216,7 @@ struct AddConstMirror { #define L_DECL_C_STRUCT(STRUCT) typedef struct _ ## STRUCT STRUCT; #define L_DECL_C_STRUCT_PREFIX_LESS(STRUCT) typedef struct STRUCT STRUCT; -#endif +#endif // ifdef __cplusplus LINPHONE_END_NAMESPACE diff --git a/include/linphone/utils/traits.h b/include/linphone/utils/traits.h new file mode 100644 index 000000000..87c413510 --- /dev/null +++ b/include/linphone/utils/traits.h @@ -0,0 +1,57 @@ +/* + * traits.h + * Copyright (C) 2010-2017 Belledonne Communications SARL + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +#ifndef _TRAITS_H_ +#define _TRAITS_H_ + +#include "general.h" + +// ============================================================================= + +LINPHONE_BEGIN_NAMESPACE + +namespace Private { + // See: http://en.cppreference.com/w/cpp/types/void_t + template struct MakeVoid { + typedef void type; + }; + template + using void_t = typename MakeVoid::type; + + template + struct IsMapContainerImpl : std::false_type {}; + + template + struct IsMapContainerImpl< + T, + void_t< + typename T::key_type, + typename T::mapped_type, + decltype(std::declval()[std::declval()]) + > + > : std::true_type {}; +}; + +// Check if a type is a std container like map, unordered_map... +template +struct IsMapContainer : Private::IsMapContainerImpl::type {}; + +LINPHONE_END_NAMESPACE + +#endif // ifndef _TRAITS_H_ diff --git a/src/address/address-p.h b/src/address/address-p.h index 8dac81823..ce7d273b5 100644 --- a/src/address/address-p.h +++ b/src/address/address-p.h @@ -20,6 +20,8 @@ #ifndef _ADDRESS_P_H_ #define _ADDRESS_P_H_ +#include + #include "address.h" #include "object/clonable-object-p.h" diff --git a/src/chat/chat-room/chat-room-id.cpp b/src/chat/chat-room/chat-room-id.cpp index 9887f1229..9f5a4482c 100644 --- a/src/chat/chat-room/chat-room-id.cpp +++ b/src/chat/chat-room/chat-room-id.cpp @@ -44,6 +44,8 @@ ChatRoomId::ChatRoomId ( d->localAddress = localAddress; } +L_USE_DEFAULT_CLONABLE_OBJECT_SHARED_IMPL(ChatRoomId); + bool ChatRoomId::operator== (const ChatRoomId &chatRoomId) const { L_D(); const ChatRoomIdPrivate *dChatRoomId = chatRoomId.getPrivate(); diff --git a/src/chat/chat-room/chat-room-id.h b/src/chat/chat-room/chat-room-id.h index 3ab346e01..4703600d5 100644 --- a/src/chat/chat-room/chat-room-id.h +++ b/src/chat/chat-room/chat-room-id.h @@ -31,6 +31,9 @@ class ChatRoomIdPrivate; class LINPHONE_PUBLIC ChatRoomId : public ClonableObject { public: ChatRoomId (const SimpleAddress &peerAddress, const SimpleAddress &localAddress); + ChatRoomId (const ChatRoomId &src); + + ChatRoomId &operator= (const ChatRoomId &src); bool operator== (const ChatRoomId &chatRoomId) const; bool operator!= (const ChatRoomId &chatRoomId) const; diff --git a/src/db/session/db-session.cpp b/src/db/session/db-session.cpp index 1dd0610cc..471caad0d 100644 --- a/src/db/session/db-session.cpp +++ b/src/db/session/db-session.cpp @@ -30,13 +30,13 @@ DbSession::DbSession (Type type) : ClonableObject(*new DbSessionPrivate) { d->type = type; } -L_USE_DEFAULT_SHARE_IMPL(DbSession, ClonableObject); - DbSession::operator bool () const { L_D(); return d->isValid; } +L_USE_DEFAULT_CLONABLE_OBJECT_SHARED_IMPL(DbSession); + DbSession::Type DbSession::getBackendType () const { L_D(); return d->type; diff --git a/src/object/clonable-object-p.h b/src/object/clonable-object-p.h index af30d627c..9faad38c0 100644 --- a/src/object/clonable-object-p.h +++ b/src/object/clonable-object-p.h @@ -20,7 +20,7 @@ #ifndef _CLONABLE_OBJECT_P_H_ #define _CLONABLE_OBJECT_P_H_ -#include +#include #include "linphone/utils/general.h" @@ -38,14 +38,9 @@ public: virtual ~ClonableObjectPrivate () = default; protected: - std::unordered_map *mPublic = nullptr; + std::set mPublic; private: - void ref (); - void unref (); - - int nRefs = 0; - L_DECLARE_PUBLIC(ClonableObject); // It's forbidden to copy directly one Clonable object private. diff --git a/src/object/clonable-object.cpp b/src/object/clonable-object.cpp index 831e7879a..961c13d67 100644 --- a/src/object/clonable-object.cpp +++ b/src/object/clonable-object.cpp @@ -27,62 +27,41 @@ using namespace std; LINPHONE_BEGIN_NAMESPACE -// TODO: Use atomic counter? - -void ClonableObjectPrivate::ref () { - ++nRefs; -} - -void ClonableObjectPrivate::unref () { - if (--nRefs == 0) { - delete mPublic; - delete this; - } -} - // ----------------------------------------------------------------------------- L_OBJECT_IMPL(ClonableObject); -ClonableObject::ClonableObject (ClonableObjectPrivate &p) : mPrivate(&p) { - // Q-pointer must be empty. It's a constructor that takes a new private data. - L_ASSERT(!mPrivate->mPublic); - - mPrivate->mPublic = new remove_pointermPublic)>::type(); - (*mPrivate->mPublic)[mPrivate] = this; - mPrivate->ref(); -} - -ClonableObject::ClonableObject (const ClonableObjectPrivate &p) { - // Cannot access to Q-pointer. It's a copy constructor from private data. - L_ASSERT(!mPrivate); - +ClonableObject::ClonableObject (ClonableObjectPrivate &p) { setRef(p); } +#define UNREF() \ + do { \ + auto &h = mPrivate->mPublic; \ + h.erase(this); \ + if (h.empty()) \ + delete mPrivate; \ + } while (false); + ClonableObject::~ClonableObject () { - mPrivate->mPublic->erase(mPrivate); - mPrivate->unref(); + UNREF(); } void ClonableObject::setRef (const ClonableObjectPrivate &p) { // Q-pointer must exist if private data is defined. - L_ASSERT(!mPrivate || mPrivate->mPublic); + L_ASSERT(!mPrivate || !mPrivate->mPublic.empty()); // Nothing, same reference. if (&p == mPrivate) return; // Unref previous private data. - if (mPrivate) { - mPrivate->mPublic->erase(mPrivate); - mPrivate->unref(); - } + if (mPrivate) + UNREF(); // Add and reference new private data. mPrivate = const_cast(&p); - (*mPrivate->mPublic)[mPrivate] = this; - mPrivate->ref(); + mPrivate->mPublic.insert(this); } LINPHONE_END_NAMESPACE diff --git a/src/object/clonable-object.h b/src/object/clonable-object.h index 30df6b61f..4decdc8e4 100644 --- a/src/object/clonable-object.h +++ b/src/object/clonable-object.h @@ -25,6 +25,16 @@ // ============================================================================= +#define L_USE_DEFAULT_CLONABLE_OBJECT_SHARED_IMPL(CLASS) \ + CLASS::CLASS (const CLASS &src) : ClonableObject( \ + const_cast::type &>(*src.getPrivate()) \ + ) {} \ + CLASS &CLASS::operator= (const CLASS &src) { \ + if (this != &src) \ + setRef(*src.getPrivate()); \ + return *this; \ + } + LINPHONE_BEGIN_NAMESPACE /* @@ -38,13 +48,9 @@ public: virtual ~ClonableObject (); protected: - // Use a new ClonableObjectPrivate without owner. explicit ClonableObject (ClonableObjectPrivate &p); - // If you want share an existing ClonableObjectPrivate, call this function. - explicit ClonableObject (const ClonableObjectPrivate &p); - - // Change the ClonableObjectPrivate, it can be shared. + // Change the ClonableObjectPrivate. Unref previous. void setRef (const ClonableObjectPrivate &p); ClonableObjectPrivate *mPrivate = nullptr;