From 375dc7b81795da9bfd8372f629d7fcef704a6d58 Mon Sep 17 00:00:00 2001
From: Aleksandar Donchev <Aleksander.Donchev@partner.bmw.de>
Date: Mon, 24 Apr 2017 17:14:32 +0200
Subject: Fix for bug in CAmSocketHandler causing invalid pointers and crash.

Signed-off-by: Christian Linke <christian.linke@bmw.de>

Change-Id: I8d3d2b436ac9fcd61c73a28145a731f327cab1e2
---
 AudioManagerUtilities/include/CAmSerializer.h      | 31 +++++----
 AudioManagerUtilities/include/CAmSocketHandler.h   | 11 ++--
 AudioManagerUtilities/src/CAmSocketHandler.cpp     | 75 ++++++++++++----------
 .../test/AmSerializerTest/CAmSerializerTest.cpp    | 69 +++++++++++++++++---
 4 files changed, 124 insertions(+), 62 deletions(-)

diff --git a/AudioManagerUtilities/include/CAmSerializer.h b/AudioManagerUtilities/include/CAmSerializer.h
index 18f503d..6bd6143 100644
--- a/AudioManagerUtilities/include/CAmSerializer.h
+++ b/AudioManagerUtilities/include/CAmSerializer.h
@@ -77,6 +77,12 @@ namespace am
      * PluginRoutingInterfaceAsync.
      *
      */
+
+    /**
+     * \defgroup Deprecated Obsolete class!
+     * @{
+     */
+
     namespace V1
     {
         class CAmSerializer
@@ -842,7 +848,9 @@ namespace am
             }
         };
 
-    }
+    } /* namespace V1 */
+
+    /**@}*/
 
     namespace V2
     {
@@ -967,16 +975,16 @@ namespace am
             int mReturnPipe[2]; //!< pipe handling returns
             sh_pollHandle_t mHandle;
             CAmSocketHandler* mpSocketHandler;
-            std::deque<CAmDelegagePtr> mListDelegatePoiters; //!< intermediate queue to store the pipe results
+            std::deque<CAmDelegagePtr> mListDelegatePointers; //!< intermediate queue to store the pipe results
 
         public:
 
             /**
              * get the size of delegate pointers
              */
-            int getListDelegatePoiters()
+            size_t getListDelegatePointers()
             {
-                return mListDelegatePoiters.size();
+                return mListDelegatePointers.size();
             }
 
             /**
@@ -1057,7 +1065,7 @@ namespace am
 
             /**
              * calls a function with variadic arguments threadsafe
-             * @param invocation is a type is produced by std::bind
+             * @param invocation is a type produced by std::bind
              * \section ex Example:
              * @code
              * CAmSerializer serial(&Sockethandler);
@@ -1125,7 +1133,7 @@ namespace am
                     logError("CAmSerializer::receiverCallback could not read pipe!");
                     throw std::runtime_error("CAmSerializer Could not read pipe!");
                 }
-                mListDelegatePoiters.assign(listPointers, listPointers + (numReads / sizeof(CAmDelegagePtr)));
+                mListDelegatePointers.assign(listPointers, listPointers + (numReads / sizeof(CAmDelegagePtr)));
             }
 
             /**
@@ -1135,7 +1143,7 @@ namespace am
             {
                 (void) handle;
                 (void) userData;
-                if (mListDelegatePoiters.empty())
+                if (mListDelegatePointers.empty())
                     return (false);
                 return (true);
             }
@@ -1147,11 +1155,11 @@ namespace am
             {
                 (void) handle;
                 (void) userData;
-                CAmDelegagePtr delegatePoiter = mListDelegatePoiters.front();
-                mListDelegatePoiters.pop_front();
+                CAmDelegagePtr delegatePoiter = mListDelegatePointers.front();
+                mListDelegatePointers.pop_front();
                 if (delegatePoiter->call(mReturnPipe))
                     delete delegatePoiter;
-                if (mListDelegatePoiters.empty())
+                if (mListDelegatePointers.empty())
                     return (false);
                 return (true);
             }
@@ -1169,7 +1177,7 @@ namespace am
                             mReturnPipe(), //
                             mHandle(),
                             mpSocketHandler(iSocketHandler),
-                            mListDelegatePoiters(), //
+                            mListDelegatePointers(), //
                             receiverCallbackT(this, &CAmSerializer::receiverCallback), //
                             dispatcherCallbackT(this, &CAmSerializer::dispatcherCallback), //
                             checkerCallbackT(this, &CAmSerializer::checkerCallback)
@@ -1205,5 +1213,6 @@ namespace am
     } /* namespace V2 */
 
     typedef V1::CAmSerializer CAmSerializer DEPRECATED("You should use V2::CAmSerializer instead!");
+
 } /* namespace am */
 #endif /* CAMSERIALIZER_H_ */
diff --git a/AudioManagerUtilities/include/CAmSocketHandler.h b/AudioManagerUtilities/include/CAmSocketHandler.h
index c2a1aa3..0f9b710 100644
--- a/AudioManagerUtilities/include/CAmSocketHandler.h
+++ b/AudioManagerUtilities/include/CAmSocketHandler.h
@@ -321,7 +321,6 @@ namespace am
         int mPipe[2];
         int mDispatchDone; //this starts / stops the mainloop
 
-        VectorListPollfd_t mfdPollingArray; //!<the polling array for ppoll
         sh_identifier_s mSetPollKeys; //!A set of all used ppoll keys
         VectorListPoll_t mListPoll; //!<list that holds all information for the ppoll
         sh_identifier_s mSetTimerKeys; //!A set of all used timer keys
@@ -435,14 +434,14 @@ namespace am
          * @param a
          * @return
          */
-        inline static void prepare(am::CAmSocketHandler::sh_poll_s& row);
-
+        inline static void prepare(sh_poll_s& row);
+    
         /**
          * functor to return all fired events
          * @param a
          * @return
          */
-        inline static void fire(sh_poll_s* a);
+        inline static void fire(sh_poll_s& a);
 
         /**
          * functor to return all fired events
@@ -456,14 +455,14 @@ namespace am
          * @param a
          * @return
          */
-        inline static bool noDispatching(const sh_poll_s* a);
+        inline static bool noDispatching(const sh_poll_s& a);
 
         /**
          * checks if dispatching is already finished
          * @param a
          * @return
          */
-        inline static bool dispatchingFinished(const sh_poll_s* a);
+        inline static bool dispatchingFinished(const sh_poll_s& a);
 
         /**
          * timer fire callback
diff --git a/AudioManagerUtilities/src/CAmSocketHandler.cpp b/AudioManagerUtilities/src/CAmSocketHandler.cpp
index 7c6fb9f..fe7cf58 100644
--- a/AudioManagerUtilities/src/CAmSocketHandler.cpp
+++ b/AudioManagerUtilities/src/CAmSocketHandler.cpp
@@ -45,7 +45,6 @@ namespace am
     CAmSocketHandler::CAmSocketHandler() :
             mPipe(), //
                     mDispatchDone(1), //
-                    mfdPollingArray(), //
                     mSetPollKeys(MAX_POLLHANDLE), //
                     mListPoll(), //
                     mSetTimerKeys(MAX_TIMERHANDLE),
@@ -108,39 +107,44 @@ namespace am
         clock_gettime(CLOCK_MONOTONIC, &mStartTime);
 #endif    
         timespec buffertime;
-        //freeze mListPoll by copying it - otherwise we get problems when we want to manipulate it during the next lines
-        std::list<sh_poll_s*> listPoll;
+        
+        std::list<sh_poll_s> listPoll;
+        VectorListPoll_t cloneListPoll;
         VectorListPoll_t::iterator listmPollIt;
         VectorListPollfd_t::iterator itMfdPollingArray;
-
+        VectorListPollfd_t fdPollingArray; //!<the polling array for ppoll
+        
         auto preparePollfd = [&](const sh_poll_s& row)
         {
             CAmSocketHandler::prepare((sh_poll_s&)row);
             pollfd temp = row.pollfdValue;
             temp.revents = 0;
-            mfdPollingArray.push_back(temp);
+            fdPollingArray.push_back(temp);
         };
 
         while (!mDispatchDone)
         {
             if (mRecreatePollfds)
             {
-                mfdPollingArray.clear();
+                fdPollingArray.clear();
+                //freeze mListPoll by copying it - otherwise we get problems when we want to manipulate it during the next lines
+                cloneListPoll = mListPoll;
                 //there was a change in the setup, so we need to recreate the fdarray from the list
-                std::for_each(mListPoll.begin(), mListPoll.end(), preparePollfd);
+                std::for_each(cloneListPoll.begin(), cloneListPoll.end(), preparePollfd);
                 mRecreatePollfds = false;
             }
             else
             {
                 //first we go through the registered filedescriptors and check if someone needs preparation:
-                std::for_each(mListPoll.begin(), mListPoll.end(), CAmSocketHandler::prepare);
+                std::for_each(cloneListPoll.begin(), cloneListPoll.end(), CAmSocketHandler::prepare);
             }
+
 #ifndef WITH_TIMERFD
             timerCorrection();
 #endif
             //block until something is on a filedescriptor
 
-            if ((pollStatus = ppoll(&mfdPollingArray[0], mfdPollingArray.size(), insertTime(buffertime), &sigmask)) < 0)
+            if ((pollStatus = ppoll(&fdPollingArray[0], fdPollingArray.size(), insertTime(buffertime), &sigmask)) < 0)
             {
                 if (errno == EINTR)
                 {
@@ -159,24 +163,25 @@ namespace am
                 //todo: here could be a timer that makes sure naughty plugins return!
                 listPoll.clear();
                 //stage 0+1, call firedCB
-                for (itMfdPollingArray = mfdPollingArray.begin(); itMfdPollingArray != mfdPollingArray.end(); itMfdPollingArray++)
+                for (itMfdPollingArray = fdPollingArray.begin(); itMfdPollingArray != fdPollingArray.end(); itMfdPollingArray++)
                 {
                     if (CAmSocketHandler::eventFired(*itMfdPollingArray))
-                    {
-                        listmPollIt = mListPoll.begin() + (itMfdPollingArray - mfdPollingArray.begin());
-                        am::CAmSocketHandler::sh_poll_s * pollItem = &((am::CAmSocketHandler::sh_poll_s&) (*listmPollIt));
-                        listPoll.push_back(pollItem);
-                        CAmSocketHandler::fire(pollItem);
+                    {                        
+                        listmPollIt = cloneListPoll.begin();
+                        std::advance(listmPollIt, std::distance(fdPollingArray.begin(), itMfdPollingArray));
+                       
+                        listPoll.push_back(*listmPollIt);
+                        CAmSocketHandler::fire(*listmPollIt);
                     }
                 }
-
+                
                 //stage 2, lets ask around if some dispatching is necessary, the ones who need stay on the list
-                listPoll.erase(std::remove_if(listPoll.begin(), listPoll.end(), CAmSocketHandler::noDispatching), listPoll.end());
+                listPoll.remove_if(CAmSocketHandler::noDispatching);
 
                 //stage 3, the ones left need to dispatch, we do this as long as there is something to dispatch..
                 do
                 {
-                    listPoll.erase(std::remove_if(listPoll.begin(), listPoll.end(), CAmSocketHandler::dispatchingFinished), listPoll.end());
+                    listPoll.remove_if(CAmSocketHandler::dispatchingFinished);
                 } while (!listPoll.empty());
 
             }
@@ -915,45 +920,45 @@ namespace am
     /**
      * fire callback
      */
-    void CAmSocketHandler::fire(sh_poll_s* a)
+    void CAmSocketHandler::fire(sh_poll_s& a)
     {
         try
         {
-            a->firedCB(a->pollfdValue, a->handle, a->userData);
+            a.firedCB(a.pollfdValue, a.handle, a.userData);
         } catch (std::exception& e)
         {
-            logError("Sockethandler: Exception in FireCallback,caught", e.what());
+            logError("Sockethandler: Exception in Preparecallback,caught", e.what());
         }
     }
 
-    /**
-     * event triggered
-     */
-    bool CAmSocketHandler::eventFired(const pollfd& a)
-    {
-        return (a.revents == 0 ? false : true);
-    }
-
     /**
      * should disptach
      */
-    bool CAmSocketHandler::noDispatching(const sh_poll_s* a)
+    bool CAmSocketHandler::noDispatching(const sh_poll_s& a)
     {
         //remove from list of there is no checkCB
-        if (!a->checkCB)
+        if (nullptr == a.checkCB)
             return (true);
-        return (!a->checkCB(a->handle, a->userData));
+        return (!a.checkCB(a.handle, a.userData));
     }
 
     /**
      * disptach
      */
-    bool CAmSocketHandler::dispatchingFinished(const sh_poll_s* a)
+    bool CAmSocketHandler::dispatchingFinished(const sh_poll_s& a)
     {
         //remove from list of there is no dispatchCB
-        if (!a->dispatchCB)
+        if (nullptr == a.dispatchCB)
             return (true);
-        return (!a->dispatchCB(a->handle, a->userData));
+        return (!a.dispatchCB(a.handle, a.userData));
+    }
+
+    /**
+     * event triggered
+     */
+    bool CAmSocketHandler::eventFired(const pollfd& a)
+    {
+        return (a.revents == 0 ? false : true);
     }
 
     /**
diff --git a/AudioManagerUtilities/test/AmSerializerTest/CAmSerializerTest.cpp b/AudioManagerUtilities/test/AmSerializerTest/CAmSerializerTest.cpp
index 508ed45..49c6738 100644
--- a/AudioManagerUtilities/test/AmSerializerTest/CAmSerializerTest.cpp
+++ b/AudioManagerUtilities/test/AmSerializerTest/CAmSerializerTest.cpp
@@ -79,7 +79,7 @@ struct SerializerData
     V2::CAmSerializer *pSerializer;
 };
 
-void* ptSerializer(void* data)
+void* ptSerializerSync(void* data)
 {
     SerializerData *pData = (SerializerData*) data;
     std::string testStr(pData->testStr);
@@ -91,11 +91,27 @@ void* ptSerializer(void* data)
 #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
     pData->pSerializer->syncCall(pData->pSerCb, &MockIAmSerializerCb::check);
     pData->pSerializer->syncCall(pData->pSerCb, &MockIAmSerializerCb::checkInt, pData->result);
-    pData->pSerializer->syncCall(pData->pSerCb, &MockIAmSerializerCb::dispatchData, result, ten, pData->testStr);
+    pData->pSerializer->syncCall(pData->pSerCb, &MockIAmSerializerCb::dispatchData, result, ten, pData->testStr);        
+#pragma GCC diagnostic pop
+    return (NULL);
+}
+
+void* ptSerializerASync(void* data)
+{
+    SerializerData *pData = (SerializerData*) data;
+    std::string testStr;
+    bool result = false;
+    int r = 0;
+    const uint32_t ten = 10;
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
     for (uint32_t i = 0; i < 5; i++)
+    {
+        testStr = pData->testStr;
         pData->pSerializer->asyncCall(pData->pSerCb, &MockIAmSerializerCb::dispatchData, i, testStr);
-
+    }
+    pData->testStr = testStr;
     pData->pSerializer->asyncInvocation(std::bind([]()->bool
     {   return 1;}));
     pData->pSerializer->asyncInvocation(std::bind([](const int i, int & result)
@@ -105,6 +121,7 @@ void* ptSerializer(void* data)
     pData->pSerializer->asyncCall(pData->pSerCb, &MockIAmSerializerCb::check);
 
     pData->pSerializer->asyncCall(pData->pSerCb, &MockIAmSerializerCb::checkInt);
+        
 #pragma GCC diagnostic pop
     return (NULL);
 }
@@ -113,7 +130,7 @@ ACTION(ActionDispatchData){
 arg1="DispatchData";
 }
 
-TEST(CAmSerializerTest, serializerTest)
+TEST(CAmSerializerTest, syncTest)
 {
     pthread_t serThread;
 
@@ -135,14 +152,12 @@ TEST(CAmSerializerTest, serializerTest)
     serializerData.pSerCb = &serCb;
     serializerData.pSocketHandler = &myHandler;
     serializerData.pSerializer = &serializer;
-    pthread_create(&serThread, NULL, ptSerializer, &serializerData);
+    pthread_create(&serThread, NULL, ptSerializerSync, &serializerData);
 
-    EXPECT_CALL(serCb,check()).Times(3);
-    EXPECT_CALL(serCb,checkInt()).Times(2).WillRepeatedly(Return(100));
+    EXPECT_CALL(serCb,check()).Times(1);
+    EXPECT_CALL(serCb,checkInt()).Times(1).WillRepeatedly(Return(100));
+    EXPECT_CALL(serCb,dispatchData(10,testStr)).Times(1).WillRepeatedly(DoAll(ActionDispatchData(), Return(true)));
 
-    EXPECT_CALL(serCb,dispatchData(10,testStr)).WillOnce(DoAll(ActionDispatchData(), Return(true)));
-    for (int i = 0; i < 5; i++)
-        EXPECT_CALL(serCb,dispatchData(i,testStr)).WillOnce(DoAll(ActionDispatchData(), Return(true)));
     myHandler.start_listenting();
 
     pthread_join(serThread, NULL);
@@ -150,6 +165,40 @@ TEST(CAmSerializerTest, serializerTest)
     ASSERT_TRUE(serializerData.result == 100);
 }
 
+TEST(CAmSerializerTest, asyncTest)
+{
+    pthread_t serThread;
+
+    MockIAmSerializerCb serCb;
+    CAmSocketHandler myHandler;
+    std::string testStr("testStr");
+    V2::CAmSerializer serializer(&myHandler);
+    sh_timerHandle_t handle;
+    timespec timeout4;
+    timeout4.tv_nsec = 0;
+    timeout4.tv_sec = 3;
+    CAmTimerSockethandlerController testCallback4(&myHandler, timeout4);
+    myHandler.addTimer(timeout4, &testCallback4.pTimerCallback, handle, NULL);
+    EXPECT_CALL(testCallback4,timerCallback(handle,NULL)).Times(1);
+
+    SerializerData serializerData;
+    serializerData.result = 0;
+    serializerData.testStr = testStr;
+    serializerData.pSerCb = &serCb;
+    serializerData.pSocketHandler = &myHandler;
+    serializerData.pSerializer = &serializer;
+    pthread_create(&serThread, NULL, ptSerializerASync, &serializerData);
+
+    EXPECT_CALL(serCb,check()).Times(2);
+    EXPECT_CALL(serCb,checkInt()).Times(1).WillRepeatedly(Return(100));
+    for (int i = 0; i < 5; i++)
+        EXPECT_CALL(serCb,dispatchData(i,testStr)).WillOnce(DoAll(ActionDispatchData(), Return(true)));
+
+    myHandler.start_listenting();
+
+    pthread_join(serThread, NULL);
+}
+
 int main(int argc, char **argv)
 {
     ::testing::InitGoogleTest(&argc, argv);
-- 
cgit v1.2.1