From 3688d0aa11c05d0412c44812a278271ad9877e0f Mon Sep 17 00:00:00 2001
From: Ray Essick <essick@google.com>
Date: Wed, 9 Oct 2019 15:31:58 -0700
Subject: [PATCH] Remove pid-caching from BufferPoolAccessor

BufferPoolAccessor cached the pid via a static constructor. If the
process forks after this, then multiple processes generating unique ids
using the same pid value. This resulted in connection ID collisions.
use getpid(), which  already caches and resets appropriately across fork().

Bug: 142423602
Bug: 133186424
Test: boot, watch log connectionIds, collision-induced failures are gone
---
 media/bufferpool/1.0/AccessorImpl.cpp | 13 +++++++++----
 media/bufferpool/1.0/AccessorImpl.h   |  1 -
 media/bufferpool/2.0/AccessorImpl.cpp | 13 +++++++++----
 media/bufferpool/2.0/AccessorImpl.h   |  1 -
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/media/bufferpool/1.0/AccessorImpl.cpp b/media/bufferpool/1.0/AccessorImpl.cpp
index 09006ca410..0d7e92f2c3 100644
--- a/media/bufferpool/1.0/AccessorImpl.cpp
+++ b/media/bufferpool/1.0/AccessorImpl.cpp
@@ -14,10 +14,11 @@
  * limitations under the License.
  */
 
-#define LOG_TAG "BufferPoolAccessor"
+#define LOG_TAG "BufferPoolAccessor1.0"
 //#define LOG_NDEBUG 0
 
 #include <sys/types.h>
+#include <stdint.h>
 #include <time.h>
 #include <unistd.h>
 #include <utils/Log.h>
@@ -127,7 +128,6 @@ bool contains(std::map<T, std::set<U>> *mapOfSet, T key, U value) {
     return false;
 }
 
-int32_t Accessor::Impl::sPid = getpid();
 uint32_t Accessor::Impl::sSeqId = time(nullptr);
 
 Accessor::Impl::Impl(
@@ -145,14 +145,19 @@ ResultStatus Accessor::Impl::connect(
     {
         std::lock_guard<std::mutex> lock(mBufferPool.mMutex);
         if (newConnection) {
-            ConnectionId id = (int64_t)sPid << 32 | sSeqId;
+            int32_t pid = getpid();
+            ConnectionId id = (int64_t)pid << 32 | sSeqId;
             status = mBufferPool.mObserver.open(id, fmqDescPtr);
             if (status == ResultStatus::OK) {
                 newConnection->initialize(accessor, id);
                 *connection = newConnection;
                 *pConnectionId = id;
                 mBufferPool.mConnectionIds.insert(id);
-                ++sSeqId;
+                if (sSeqId == UINT32_MAX) {
+                   sSeqId = 0;
+                } else {
+                    ++sSeqId;
+                }
             }
         }
         mBufferPool.processStatusMessages();
diff --git a/media/bufferpool/1.0/AccessorImpl.h b/media/bufferpool/1.0/AccessorImpl.h
index 84cb6854b0..a09cbe237f 100644
--- a/media/bufferpool/1.0/AccessorImpl.h
+++ b/media/bufferpool/1.0/AccessorImpl.h
@@ -61,7 +61,6 @@ private:
     // ConnectionId = pid : (timestamp_created + seqId)
     // in order to guarantee uniqueness for each connection
     static uint32_t sSeqId;
-    static int32_t sPid;
 
     const std::shared_ptr<BufferPoolAllocator> mAllocator;
 
diff --git a/media/bufferpool/2.0/AccessorImpl.cpp b/media/bufferpool/2.0/AccessorImpl.cpp
index 929a20e2c0..84ce172521 100644
--- a/media/bufferpool/2.0/AccessorImpl.cpp
+++ b/media/bufferpool/2.0/AccessorImpl.cpp
@@ -14,10 +14,11 @@
  * limitations under the License.
  */
 
-#define LOG_TAG "BufferPoolAccessor"
+#define LOG_TAG "BufferPoolAccessor2.0"
 //#define LOG_NDEBUG 0
 
 #include <sys/types.h>
+#include <stdint.h>
 #include <time.h>
 #include <unistd.h>
 #include <utils/Log.h>
@@ -134,7 +135,6 @@ bool contains(std::map<T, std::set<U>> *mapOfSet, T key, U value) {
     return false;
 }
 
-int32_t Accessor::Impl::sPid = getpid();
 uint32_t Accessor::Impl::sSeqId = time(nullptr);
 
 Accessor::Impl::Impl(
@@ -156,7 +156,8 @@ ResultStatus Accessor::Impl::connect(
     {
         std::lock_guard<std::mutex> lock(mBufferPool.mMutex);
         if (newConnection) {
-            ConnectionId id = (int64_t)sPid << 32 | sSeqId;
+            int32_t pid = getpid();
+            ConnectionId id = (int64_t)pid << 32 | sSeqId;
             status = mBufferPool.mObserver.open(id, statusDescPtr);
             if (status == ResultStatus::OK) {
                 newConnection->initialize(accessor, id);
@@ -166,7 +167,11 @@ ResultStatus Accessor::Impl::connect(
                 mBufferPool.mConnectionIds.insert(id);
                 mBufferPool.mInvalidationChannel.getDesc(invDescPtr);
                 mBufferPool.mInvalidation.onConnect(id, observer);
-                ++sSeqId;
+                if (sSeqId == UINT32_MAX) {
+                   sSeqId = 0;
+                } else {
+                    ++sSeqId;
+                }
             }
 
         }
diff --git a/media/bufferpool/2.0/AccessorImpl.h b/media/bufferpool/2.0/AccessorImpl.h
index 807e0f1444..9888be5107 100644
--- a/media/bufferpool/2.0/AccessorImpl.h
+++ b/media/bufferpool/2.0/AccessorImpl.h
@@ -75,7 +75,6 @@ private:
     // ConnectionId = pid : (timestamp_created + seqId)
     // in order to guarantee uniqueness for each connection
     static uint32_t sSeqId;
-    static int32_t sPid;
 
     const std::shared_ptr<BufferPoolAllocator> mAllocator;