From 99e5e01b131234e489ddc1d7d7adf4da0d646367 Mon Sep 17 00:00:00 2001
From: Kevin Tang <zhikait@codeaurora.org>
Date: Fri, 11 Jan 2019 17:41:30 -0800
Subject: [PATCH] LocIpc could be using data member from a deleted obj

startListeningBlocking is meant to be called under a reader
thread, which is the case if startListeningNonBlocking calls
it. A LocIpc client may delete the LocIpc obj, which would
trigger sending an ABORT msg to the reader thread before it
is subsequently deleted, in which case, it is possible that
when the reader thread comes around to process the ABORT msg,
referencing the data members of the possibly stale obj would
cause unpredictable behaviors.

Change-Id: I441af85c04d92b6fff695c020e3e0b4bd5e90409
CRs-Fixed: 2380093
---
 utils/LocIpc.cpp | 117 +++++++++++++++++++++--------------------------
 utils/LocIpc.h   |   3 +-
 2 files changed, 53 insertions(+), 67 deletions(-)

diff --git a/utils/LocIpc.cpp b/utils/LocIpc.cpp
index f7e163da..7347b781 100644
--- a/utils/LocIpc.cpp
+++ b/utils/LocIpc.cpp
@@ -69,8 +69,9 @@ bool LocIpc::startListeningNonBlocking(const std::string& name) {
 }
 
 bool LocIpc::startListeningBlocking(const std::string& name) {
-
+    bool stopRequested = false;
     int fd = socket(AF_UNIX, SOCK_DGRAM, 0);
+
     if (fd < 0) {
         LOC_LOGe("create socket error. reason:%s", strerror(errno));
         return false;
@@ -87,89 +88,75 @@ bool LocIpc::startListeningBlocking(const std::string& name) {
 
     if (::bind(fd, (struct sockaddr*)&addr, sizeof(addr)) < 0) {
         LOC_LOGe("bind socket error. reason:%s", strerror(errno));
-        ::close(fd);
-        fd = -1;
-        return false;
-    }
+    } else {
+        mIpcFd = fd;
 
-    mIpcFd = fd;
+        // inform that the socket is ready to receive message
+        onListenerReady();
 
-    // inform that the socket is ready to receive message
-    onListenerReady();
-
-    ssize_t nBytes = 0;
-    std::string msg = "";
-    std::string abort = LOC_MSG_ABORT;
-    while (1) {
-        msg.resize(LOC_MSG_BUF_LEN);
-        nBytes = ::recvfrom(mIpcFd, (void*)(msg.data()), msg.size(), 0, NULL, NULL);
-        if (nBytes < 0) {
-            break;
-        } else if (nBytes == 0) {
-            continue;
-        }
-
-        if (strncmp(msg.data(), abort.c_str(), abort.length()) == 0) {
-            LOC_LOGi("recvd abort msg.data %s", msg.data());
-            break;
-        }
-
-        if (strncmp(msg.data(), LOC_MSG_HEAD, sizeof(LOC_MSG_HEAD) - 1)) {
-            // short message
-            msg.resize(nBytes);
-            onReceive(msg);
-        } else {
-            // long message
-            size_t msgLen = 0;
-            sscanf(msg.data(), LOC_MSG_HEAD"%zu", &msgLen);
-            msg.resize(msgLen);
-            size_t msgLenReceived = 0;
-            while ((msgLenReceived < msgLen) && (nBytes > 0)) {
-                nBytes = recvfrom(mIpcFd, (void*)&(msg[msgLenReceived]),
-                        msg.size() - msgLenReceived, 0, NULL, NULL);
-                msgLenReceived += nBytes;
+        ssize_t nBytes = 0;
+        std::string msg = "";
+        std::string abort = LOC_MSG_ABORT;
+        while (1) {
+            msg.resize(LOC_MSG_BUF_LEN);
+            nBytes = ::recvfrom(fd, (void*)(msg.data()), msg.size(), 0, NULL, NULL);
+            if (nBytes < 0) {
+                LOC_LOGe("cannot read socket. reason:%s", strerror(errno));
+                break;
+            } else if (0 == nBytes) {
+                continue;
             }
-            if (nBytes > 0) {
-                onReceive(msg);
-            } else {
+
+            if (strncmp(msg.data(), abort.c_str(), abort.length()) == 0) {
+                LOC_LOGi("recvd abort msg.data %s", msg.data());
+                stopRequested = true;
                 break;
             }
+
+            if (strncmp(msg.data(), LOC_MSG_HEAD, sizeof(LOC_MSG_HEAD) - 1)) {
+                // short message
+                msg.resize(nBytes);
+                onReceive(msg);
+            } else {
+                // long message
+                size_t msgLen = 0;
+                sscanf(msg.data(), LOC_MSG_HEAD"%zu", &msgLen);
+                msg.resize(msgLen);
+                size_t msgLenReceived = 0;
+                while ((msgLenReceived < msgLen) && (nBytes > 0)) {
+                    nBytes = recvfrom(fd, (void*)&(msg[msgLenReceived]),
+                                      msg.size() - msgLenReceived, 0, NULL, NULL);
+                    msgLenReceived += nBytes;
+                }
+                if (nBytes > 0) {
+                    onReceive(msg);
+                } else {
+                    LOC_LOGe("cannot read socket. reason:%s", strerror(errno));
+                    break;
+                }
+            }
         }
     }
 
-    if (mStopRequested) {
-        mStopRequested = false;
-        return true;
-    } else {
-        LOC_LOGe("cannot read socket. reason:%s", strerror(errno));
-        (void)::close(mIpcFd);
-        mIpcFd = -1;
-        return false;
+    if (::close(fd)) {
+        LOC_LOGe("cannot close socket:%s", strerror(errno));
     }
+    unlink(name.c_str());
+
+    return stopRequested;
 }
 
 void LocIpc::stopListening() {
-
     const char *socketName = nullptr;
-    mStopRequested = true;
 
-    if (mRunnable) {
+    if (mIpcFd >= 0) {
         std::string abort = LOC_MSG_ABORT;
         socketName = (reinterpret_cast<LocIpcRunnable *>(mRunnable))->mIpcName.c_str();
         send(socketName, abort);
-        mRunnable = nullptr;
-    }
-
-    if (mIpcFd >= 0) {
-        if (::close(mIpcFd)) {
-            LOC_LOGe("cannot close socket:%s", strerror(errno));
-        }
         mIpcFd = -1;
     }
-
-    //delete from the file system at the end
-    if (socketName) {
-        unlink(socketName);
+    if (mRunnable) {
+        mRunnable = nullptr;
     }
 }
 
diff --git a/utils/LocIpc.h b/utils/LocIpc.h
index a1a994d1..87f2ff8c 100644
--- a/utils/LocIpc.h
+++ b/utils/LocIpc.h
@@ -44,7 +44,7 @@ class LocIpcSender;
 class LocIpc {
 friend LocIpcSender;
 public:
-    inline LocIpc() : mIpcFd(-1), mStopRequested(false), mRunnable(nullptr) {}
+    inline LocIpc() : mIpcFd(-1), mRunnable(nullptr) {}
     inline virtual ~LocIpc() { stopListening(); }
 
     // Listen for new messages in current thread. Calling this funciton will
@@ -93,7 +93,6 @@ private:
             const uint8_t data[], uint32_t length);
 
     int mIpcFd;
-    bool mStopRequested;
     LocThread mThread;
     LocRunnable *mRunnable;
 };