AMUtil: Fix startup sequence addFdPoll and removeFdPoll

In case plugins add new fdpoll elements and directly removing them
afterwards the state inside the structure will be changed to REMOVE.
REMOVE will lead all the time to an erase which will not work in case the
pointer is at .end() of vector or the element itself is not the correct
one. Both cases will be fixed just by checking if the fd is the same.

Signed-off-by: Jens Lorenz <jlorenz@de.adit-jv.com>

2 files changed, 87 insertions, 4 deletions

diff --git a/AudioManagerUtilities/src/CAmSocketHandler.cpp b/AudioManagerUtilities/src/CAmSocketHandler.cpp
index 1e7db6a..e5752b4 100644
--- a/AudioManagerUtilities/src/CAmSocketHandler.cpp
+++ b/AudioManagerUtilities/src/CAmSocketHandler.cpp
@@ -40,9 +40,9 @@
 #include <sys/timerfd.h>
 #endif
 
+#define END_EVENT                  (UINT64_MAX >> 1)
 #define SHPOLL_IS_ACTIVE(state)    (!(state == poll_states_e::REMOVE || state == poll_states_e::CLOSE))
 
-
 namespace am
 {
 
@@ -72,7 +72,7 @@ CAmSocketHandler::CAmSocketHandler() :
           ssize_t bytes = read(pollfd.fd, &events, sizeof(events));
           if (bytes == sizeof(events))
           {
-              if (events == UINT64_MAX-1)
+              if (events >= END_EVENT)
               {
                   for (auto & elem : mMapShPoll)
                   {
@@ -80,6 +80,7 @@ CAmSocketHandler::CAmSocketHandler() :
                           elem.second.state = poll_states_e::UNINIT;
                   }
                   mDispatchDone = true;
+              }
               return;
           }
 
@@ -145,7 +146,14 @@ void CAmSocketHandler::start_listenting()
                     // fallthrough
 
                 case poll_states_e::REMOVE:
-                    fdPollIt = fdPollingArray.erase(fdPollIt);
+                    /* The first check is needed for the restart behavior were an element is marked
+                     * as removed but not part of the polling array - which is stored on heap.
+                     * The second check is needed in case a map element was inserted newly but was
+                     * directly marked after as REMOVE but in between there was no sync. of the
+                     * polling array processed.
+                     */
+                    if (fdPollIt != fdPollingArray.end() && fdPollIt->fd == elem.pollfdValue.fd)
+                        fdPollIt = fdPollingArray.erase(fdPollIt);
                     it = mMapShPoll.erase(it);
                     continue;
 
@@ -246,7 +254,7 @@ void CAmSocketHandler::exit_mainloop()
     stop_listening();
 
     //fire the ending filedescriptor
-    const uint64_t events = UINT64_MAX-1;
+    const uint64_t events = END_EVENT;
     if (write(mEventFd, &events, sizeof(events)) < 0)
     {
         //Failed to write to event fd...
diff --git a/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp b/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp
index 085c3b1..a31ccf4 100644
--- a/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp
+++ b/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp
@@ -30,6 +30,7 @@
 #include <fcntl.h>
 #include <sys/un.h>
 #include <sys/poll.h>
+#include <sys/eventfd.h>
 #include "CAmDltWrapper.h"
 #include "CAmSocketHandler.h"
 
@@ -56,6 +57,12 @@ struct TestUserData
     float f;
 };
 
+struct TestStressUserData
+{
+    CAmSocketHandler &socket;
+    std::vector<sh_pollHandle_t> &handles;
+};
+
 MockIAmSignalHandler *pMockSignalHandler = NULL;
 static void signalHandler(int sig, siginfo_t *siginfo, void *context)
 {
@@ -308,6 +315,22 @@ void* threadCallbackUnixSocketAndTimers(void* data)
     return sendTestData(sock, (struct sockaddr*)&servAddr, sizeof(servAddr), 500000);
 }
 
+void* threadRaceFd(void* pData)
+{
+    struct TestStressUserData data = *(struct TestStressUserData*)pData;
+    usleep(50000);
+    auto elem = data.handles.begin();
+    std::advance(elem, data.handles.size() / 2);
+    data.socket.removeFDPoll(*elem, sh_rmv_e::RMV_N_CLS);
+    data.handles.erase(elem);
+}
+void* threadEnd(void* pData)
+{
+    struct TestStressUserData data = *(struct TestStressUserData*)pData;
+    usleep(1000000);
+    data.socket.exit_mainloop();
+}
+
 TEST(CAmSocketHandlerTest, stressTestUnixSocketAndTimers)
 {
 
@@ -345,6 +368,58 @@ TEST(CAmSocketHandlerTest, stressTestUnixSocketAndTimers)
     shutdown(socket_, SHUT_RDWR);
 }
 
+
+TEST(CAmSocketHandlerTest, fdStressTest)
+{
+    CAmSocketHandler myHandler;
+    ASSERT_FALSE(myHandler.fatalErrorOccurred());
+
+    //Check unkonw systemd fd ids
+    sh_pollHandle_t handle;
+    EXPECT_EQ(myHandler.addFDPoll(100, 0, NULL, NULL, NULL, NULL, NULL, handle), E_NON_EXISTENT);
+
+    int fd(-1);
+    std::vector<sh_pollHandle_t> handles(10);
+    for (auto& hndl : handles)
+    {
+        fd = eventfd(0, 0);
+        ASSERT_EQ(myHandler.addFDPoll(fd, POLL_IN, NULL, NULL, NULL, NULL, NULL, hndl), E_OK);
+    }
+
+    // remove/add check
+    ASSERT_EQ(myHandler.addFDPoll(fd, POLL_IN, NULL, NULL, NULL, NULL, NULL, handles.back()), E_ALREADY_EXISTS);
+    ASSERT_EQ(myHandler.removeFDPoll(handles.back()), E_OK);
+    ASSERT_EQ(myHandler.addFDPoll(fd, POLL_IN, NULL, NULL, NULL, NULL, NULL, handles.back()), E_OK);
+
+    // create a copy to check if all handles are removed
+    std::vector<sh_pollHandle_t> handlesCheckup(handles);
+
+    while (handles.size())
+    {
+        pthread_t tid1, tid2;
+
+        // this removes an element before starting the socket handler and we
+        // erase the last handle
+        myHandler.removeFDPoll(handles.back(), sh_rmv_e::RMV_N_CLS);
+        handles.erase(handles.end()-1);
+
+        TestStressUserData data = {myHandler, handles};
+        pthread_create(&tid1, NULL, threadEnd, &data);
+
+        // erase the handle in the middle
+        pthread_create(&tid2, NULL, threadRaceFd, &data);
+
+        myHandler.start_listenting();
+
+        pthread_join(tid2, NULL);
+        pthread_join(tid1, NULL);
+    }
+
+    // now do the check
+    for (auto& hndl : handlesCheckup)
+        EXPECT_EQ(myHandler.removeFDPoll(hndl), E_UNKNOWN) << "Handle " << hndl << " not correctly removed before";
+}
+
 TEST(CAmSocketHandlerTest, timersOneshot)
 {
     CAmSocketHandler myHandler;