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

@@ -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
 )
 

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<typename... T> struct MakeVoid {
-		typedef void type;
-	};
-	template<typename... T>
-	using void_t = typename MakeVoid<T...>::type;
-
-	template<typename T, typename U = void>
-	struct IsMapContainerImpl : std::false_type {};
-
-	template<typename T>
-	struct IsMapContainerImpl<
-		T,
-		void_t<
-			typename T::key_type,
-			typename T::mapped_type,
-			decltype(std::declval<T&>()[std::declval<const typename T::key_type&>()])
-		>
-	> : std::true_type {};
-};
-
-// Check if a type is a std container like map, unordered_map...
-template<typename T>
-struct IsMapContainer : Private::IsMapContainerImpl<T>::type {};
-
 // Generic public helper.
 template<
 	typename R,
 	typename P,
-	typename C,
-	typename = typename std::enable_if<!IsMapContainer<P>::value, P>::type
+	typename C
 >
 constexpr R *getPublicHelper (P *object, const C *) {
 	return static_cast<R *>(object);
@@ -173,22 +146,19 @@ constexpr R *getPublicHelper (P *object, const C *) {
 template<
 	typename R,
 	typename P,
-	typename C,
-	typename = typename std::enable_if<IsMapContainer<P>::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<R *>(it->second);
+inline R *getPublicHelper (const P &objectSet, const C *) {
+	auto it = objectSet.cbegin();
+	L_ASSERT(it != objectSet.cend());
+	return static_cast<R *>(*it);
 }
 
 #define L_DECLARE_PUBLIC(CLASS) \
 	inline CLASS *getPublic () { \
-		L_ASSERT(mPublic); \
 		return getPublicHelper<CLASS>(mPublic, this); \
 	} \
 	inline const CLASS *getPublic () const { \
-		L_ASSERT(mPublic); \
 		return getPublicHelper<const CLASS>(mPublic, this); \
 	} \
 	friend class CLASS;
@@ -239,14 +209,6 @@ struct AddConstMirror<const T, U> {
 		return std::static_pointer_cast<const CLASS>(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<const T, U> {
 #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
 

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<typename... T> struct MakeVoid {
+		typedef void type;
+	};
+	template<typename... T>
+	using void_t = typename MakeVoid<T...>::type;
+
+	template<typename T, typename U = void>
+	struct IsMapContainerImpl : std::false_type {};
+
+	template<typename T>
+	struct IsMapContainerImpl<
+		T,
+		void_t<
+			typename T::key_type,
+			typename T::mapped_type,
+			decltype(std::declval<T&>()[std::declval<const typename T::key_type&>()])
+		>
+	> : std::true_type {};
+};
+
+// Check if a type is a std container like map, unordered_map...
+template<typename T>
+struct IsMapContainer : Private::IsMapContainerImpl<T>::type {};
+
+LINPHONE_END_NAMESPACE
+
+#endif // ifndef _TRAITS_H_

src/address/address-p.h

@@ -20,6 +20,8 @@
 #ifndef _ADDRESS_P_H_
 #define _ADDRESS_P_H_
 
+#include <unordered_map>
+
 #include "address.h"
 #include "object/clonable-object-p.h"
 

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();

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;

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;

src/object/clonable-object-p.h

@@ -20,7 +20,7 @@
 #ifndef _CLONABLE_OBJECT_P_H_
 #define _CLONABLE_OBJECT_P_H_
 
-#include <unordered_map>
+#include <set>
 
 #include "linphone/utils/general.h"
 
@@ -38,14 +38,9 @@ public:
 	virtual ~ClonableObjectPrivate () = default;
 
 protected:
-	std::unordered_map<const ClonableObjectPrivate *, ClonableObject *> *mPublic = nullptr;
+	std::set<ClonableObject *> mPublic;
 
 private:
-	void ref ();
-	void unref ();
-
-	int nRefs = 0;
-
 	L_DECLARE_PUBLIC(ClonableObject);
 
 	// It's forbidden to copy directly one Clonable object private.

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_pointer<decltype(mPrivate->mPublic)>::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<ClonableObjectPrivate *>(&p);
-	(*mPrivate->mPublic)[mPrivate] = this;
-	mPrivate->ref();
+	mPrivate->mPublic.insert(this);
 }
 
 LINPHONE_END_NAMESPACE

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<std::decay<decltype(*src.getPrivate())>::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;