From e9fa9eb317f871602b13f27753146bfb8005d85e Mon Sep 17 00:00:00 2001 From: csoler Date: Thu, 21 Apr 2016 22:07:50 -0400 Subject: [PATCH] removed tricky memory leak in chat lobbies due to handling of partial messages --- libretroshare/src/chat/distantchat.cc | 2 ++ libretroshare/src/chat/distantchat.h | 2 +- libretroshare/src/chat/distributedchat.cc | 5 --- libretroshare/src/chat/distributedchat.h | 1 - libretroshare/src/chat/p3chatservice.cc | 42 +++++++++++++---------- libretroshare/src/chat/p3chatservice.h | 5 +-- 6 files changed, 30 insertions(+), 27 deletions(-) diff --git a/libretroshare/src/chat/distantchat.cc b/libretroshare/src/chat/distantchat.cc index 3c951bb43..805728905 100644 --- a/libretroshare/src/chat/distantchat.cc +++ b/libretroshare/src/chat/distantchat.cc @@ -268,6 +268,8 @@ bool DistantChatService::initiateDistantChatConnexion(const RsGxsId& to_gxs_id, item->PeerId(RsPeerId(tunnel_id)) ; handleRecvChatMsgItem(item) ; + delete item ; // item is replaced by NULL if partial, but this is not the case here. + return true ; } diff --git a/libretroshare/src/chat/distantchat.h b/libretroshare/src/chat/distantchat.h index 5dc21e1d6..444817c06 100644 --- a/libretroshare/src/chat/distantchat.h +++ b/libretroshare/src/chat/distantchat.h @@ -63,7 +63,7 @@ public: // derived in p3ChatService, so as to pass down some info virtual void handleIncomingItem(RsItem *) = 0; - virtual bool handleRecvChatMsgItem(RsChatMsgItem *ci)=0 ; + virtual bool handleRecvChatMsgItem(RsChatMsgItem *& ci)=0 ; bool handleOutgoingItem(RsChatItem *) ; bool handleRecvItem(RsChatItem *) ; diff --git a/libretroshare/src/chat/distributedchat.cc b/libretroshare/src/chat/distributedchat.cc index f77fb3718..88460f1f7 100644 --- a/libretroshare/src/chat/distributedchat.cc +++ b/libretroshare/src/chat/distributedchat.cc @@ -424,11 +424,6 @@ void DistributedChatService::checkSizeAndSendLobbyMessage(RsChatItem *msg) sendChatItem(msg) ; } -bool DistributedChatService::locked_checkAndRebuildPartialLobbyMessage(RsChatLobbyMsgItem *ci) -{ - return true ; -} - bool DistributedChatService::handleRecvItem(RsChatItem *item) { switch(item->PacketSubType()) diff --git a/libretroshare/src/chat/distributedchat.h b/libretroshare/src/chat/distributedchat.h index c311727b1..89160a850 100644 --- a/libretroshare/src/chat/distributedchat.h +++ b/libretroshare/src/chat/distributedchat.h @@ -90,7 +90,6 @@ class DistributedChatService void addToSaveList(std::list& list) const ; bool processLoadListItem(const RsItem *item) ; - bool locked_checkAndRebuildPartialLobbyMessage(RsChatLobbyMsgItem *) ; void checkSizeAndSendLobbyMessage(RsChatItem *) ; bool sendLobbyChat(const ChatLobbyId &lobby_id, const std::string&) ; diff --git a/libretroshare/src/chat/p3chatservice.cc b/libretroshare/src/chat/p3chatservice.cc index 3f0c87f3d..bafdb575c 100644 --- a/libretroshare/src/chat/p3chatservice.cc +++ b/libretroshare/src/chat/p3chatservice.cc @@ -428,7 +428,14 @@ bool p3ChatService::sendChat(ChatId destination, std::string msg) return true; } -bool p3ChatService::locked_checkAndRebuildPartialMessage(RsChatMsgItem *ci) +// This method might take control over the memory, or modify it, possibly adding missing parts. +// This function looks weird because it cannot duplicate the message since it does not know +// what type of object it is and the duplicate method of lobby messages is reserved for +// ChatLobby bouncing objects. +// +// Returns false if the item shouldn't be used (and replaced to NULL) + +bool p3ChatService::locked_checkAndRebuildPartialMessage(RsChatMsgItem *& ci) { // Check is the item is ending an incomplete item. // @@ -445,13 +452,16 @@ bool p3ChatService::locked_checkAndRebuildPartialMessage(RsChatMsgItem *ci) ci->message = it->second->message + ci->message ; ci->chatFlags |= it->second->chatFlags ; - + + // always remove existing partial. The compound message is in ci now. + delete it->second ; - - if(!ci_is_incomplete) - _pendingPartialMessages.erase(it) ; + _pendingPartialMessages.erase(it) ; } + // now decide what to do: if ci is incomplete, store it and replace the pointer with NULL + // if complete, return it. + if(ci_is_incomplete) { #ifdef CHAT_DEBUG @@ -459,7 +469,8 @@ bool p3ChatService::locked_checkAndRebuildPartialMessage(RsChatMsgItem *ci) #endif // The item is a partial message. Push it, and wait for the rest. // - _pendingPartialMessages[ci->PeerId()] = ci ; + _pendingPartialMessages[ci->PeerId()] = ci ; // cannot use duplicate() here + ci = NULL ; // takes memory ownership over ci return false ; } else @@ -503,8 +514,10 @@ void p3ChatService::handleIncomingItem(RsItem *item) // RsChatMsgItem *ci = dynamic_cast(item) ; if(ci != NULL) - { - if(! handleRecvChatMsgItem(ci)) + { + handleRecvChatMsgItem(ci); + + if(ci) delete ci ; return ; // don't delete! It's handled by handleRecvChatMsgItem in some specific cases only. @@ -665,7 +678,7 @@ bool p3ChatService::checkForMessageSecurity(RsChatMsgItem *ci) return true ; } -bool p3ChatService::handleRecvChatMsgItem(RsChatMsgItem *ci) +bool p3ChatService::handleRecvChatMsgItem(RsChatMsgItem *& ci) { time_t now = time(NULL); std::string name; @@ -674,15 +687,8 @@ bool p3ChatService::handleRecvChatMsgItem(RsChatMsgItem *ci) { RsStackMutex stack(mChatMtx); /********** STACK LOCKED MTX ******/ - // This crap is because chat lobby messages use a different method for chunking messages using an additional - // subpacket ID, and a list of lobbies. We cannot just collapse the two because it would make the normal chat - // (and chat lobbies) not backward compatible. - - if(!DistributedChatService::locked_checkAndRebuildPartialLobbyMessage(dynamic_cast(ci))) - return true ; - - if(!locked_checkAndRebuildPartialMessage(ci)) - return true ; + if(!locked_checkAndRebuildPartialMessage(ci)) // we make sure this call does not take control over the memory + return true ; // message is a subpart of an existing message. So everything ok, but we need to return. } // Check for security. This avoids bombing messages, and so on. diff --git a/libretroshare/src/chat/p3chatservice.h b/libretroshare/src/chat/p3chatservice.h index 4fc9e1baf..6dbdb557c 100644 --- a/libretroshare/src/chat/p3chatservice.h +++ b/libretroshare/src/chat/p3chatservice.h @@ -205,7 +205,7 @@ private: void receiveStateString(const RsPeerId& id,const std::string& s) ; /// methods for handling various Chat items. - bool handleRecvChatMsgItem(RsChatMsgItem *item) ; // returns false if the item should be deleted. + bool handleRecvChatMsgItem(RsChatMsgItem *&item) ; // NULL-ifies the item if memory ownership is taken void handleRecvChatStatusItem(RsChatStatusItem *item) ; void handleRecvChatAvatarItem(RsChatAvatarItem *item) ; @@ -220,7 +220,8 @@ private: void checkSizeAndSendMessage(RsChatMsgItem *item) ; // keep for compatibility for a few weeks. /// Called when a RsChatMsgItem is received. The item may be collapsed with any waiting partial chat item from the same peer. - bool locked_checkAndRebuildPartialMessage(RsChatMsgItem *) ; + /// if so, the chat item will be turned to NULL + bool locked_checkAndRebuildPartialMessage(RsChatMsgItem *&) ; RsChatAvatarItem *makeOwnAvatarItem() ; RsChatStatusItem *makeOwnCustomStateStringItem() ;