From 22ebbf78220c39edf517a2c758079d6fc481ea7e Mon Sep 17 00:00:00 2001
From: Keith Mok <keithmok@google.com>
Date: Fri, 6 May 2022 07:39:04 +0000
Subject: [PATCH] vehicle-aidl: Fix various race condition

mThread should not be created in initization list,
since class member variables are not ready yet,
and the thread will access those variables.

Even if the shared variable is atomic, it must be modified under the
mutex in order to correctly publish the modification to the waiting
thread.

Bug: 231668271
Test: android.hardware.automotive.vehicle@V1-default-service_fuzzer
Change-Id: Ibbf36ddf9edcaa8d016349631784f54f6fd6c877
---
 .../default/impl/vhal_v2_0/GeneratorHub.cpp   | 13 ++++++--
 .../GeneratorHub/src/GeneratorHub.cpp         | 13 ++++++--
 .../utils/common/include/PendingRequestPool.h |  3 +-
 .../utils/common/src/PendingRequestPool.cpp   | 31 ++++++++++++-------
 4 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp b/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp
index 9be9ea7c8e..503afd2eb2 100644
--- a/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp
+++ b/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp
@@ -28,11 +28,18 @@ namespace V2_0 {
 
 namespace impl {
 
-GeneratorHub::GeneratorHub(const OnHalEvent& onHalEvent)
-    : mOnHalEvent(onHalEvent), mThread(&GeneratorHub::run, this) {}
+GeneratorHub::GeneratorHub(const OnHalEvent& onHalEvent) : mOnHalEvent(onHalEvent) {
+    mThread = std::thread(&GeneratorHub::run, this);
+}
 
 GeneratorHub::~GeneratorHub() {
-    mShuttingDownFlag.store(true);
+    {
+        // Even if the shared variable is atomic, it must be modified under the
+        // mutex in order to correctly publish the modification to the waiting
+        // thread.
+        std::unique_lock<std::mutex> g(mLock);
+        mShuttingDownFlag.store(true);
+    }
     mCond.notify_all();
     if (mThread.joinable()) {
         mThread.join();
diff --git a/automotive/vehicle/aidl/impl/fake_impl/GeneratorHub/src/GeneratorHub.cpp b/automotive/vehicle/aidl/impl/fake_impl/GeneratorHub/src/GeneratorHub.cpp
index 1690c78ef9..d815456a56 100644
--- a/automotive/vehicle/aidl/impl/fake_impl/GeneratorHub/src/GeneratorHub.cpp
+++ b/automotive/vehicle/aidl/impl/fake_impl/GeneratorHub/src/GeneratorHub.cpp
@@ -29,11 +29,18 @@ namespace fake {
 
 using ::android::base::ScopedLockAssertion;
 
-GeneratorHub::GeneratorHub(OnHalEvent&& onHalEvent)
-    : mOnHalEvent(onHalEvent), mThread(&GeneratorHub::run, this) {}
+GeneratorHub::GeneratorHub(OnHalEvent&& onHalEvent) : mOnHalEvent(onHalEvent) {
+    mThread = std::thread(&GeneratorHub::run, this);
+}
 
 GeneratorHub::~GeneratorHub() {
-    mShuttingDownFlag.store(true);
+    {
+        // Even if the shared variable is atomic, it must be modified under the
+        // mutex in order to correctly publish the modification to the waiting
+        // thread.
+        std::unique_lock<std::mutex> lock(mGeneratorsLock);
+        mShuttingDownFlag.store(true);
+    }
     mCond.notify_all();
     if (mThread.joinable()) {
         mThread.join();
diff --git a/automotive/vehicle/aidl/impl/utils/common/include/PendingRequestPool.h b/automotive/vehicle/aidl/impl/utils/common/include/PendingRequestPool.h
index 3f8db93ba3..28cf08e93b 100644
--- a/automotive/vehicle/aidl/impl/utils/common/include/PendingRequestPool.h
+++ b/automotive/vehicle/aidl/impl/utils/common/include/PendingRequestPool.h
@@ -21,7 +21,6 @@
 #include <android-base/result.h>
 #include <android-base/thread_annotations.h>
 
-#include <atomic>
 #include <list>
 #include <mutex>
 #include <thread>
@@ -85,7 +84,7 @@ class PendingRequestPool final {
     std::unordered_map<const void*, std::list<PendingRequest>> mPendingRequestsByClient
             GUARDED_BY(mLock);
     std::thread mThread;
-    std::atomic<bool> mThreadStop = false;
+    bool mThreadStop = false;
     std::condition_variable mCv;
     std::mutex mCvLock;
 
diff --git a/automotive/vehicle/aidl/impl/utils/common/src/PendingRequestPool.cpp b/automotive/vehicle/aidl/impl/utils/common/src/PendingRequestPool.cpp
index 0196edd2ca..ab504992e4 100644
--- a/automotive/vehicle/aidl/impl/utils/common/src/PendingRequestPool.cpp
+++ b/automotive/vehicle/aidl/impl/utils/common/src/PendingRequestPool.cpp
@@ -39,20 +39,27 @@ constexpr int64_t CHECK_TIME_IN_NANO = 1'000'000'000;
 
 }  // namespace
 
-PendingRequestPool::PendingRequestPool(int64_t timeoutInNano)
-    : mTimeoutInNano(timeoutInNano), mThread([this] {
-          // [this] must be alive within this thread because destructor would wait for this thread
-          // to exit.
-          int64_t sleepTime = std::min(mTimeoutInNano, static_cast<int64_t>(CHECK_TIME_IN_NANO));
-          std::unique_lock<std::mutex> lk(mCvLock);
-          while (!mCv.wait_for(lk, std::chrono::nanoseconds(sleepTime),
-                               [this] { return mThreadStop.load(); })) {
-              checkTimeout();
-          }
-      }) {}
+PendingRequestPool::PendingRequestPool(int64_t timeoutInNano) : mTimeoutInNano(timeoutInNano) {
+    mThread = std::thread([this] {
+        // [this] must be alive within this thread because destructor would wait for this thread
+        // to exit.
+        int64_t sleepTime = std::min(mTimeoutInNano, static_cast<int64_t>(CHECK_TIME_IN_NANO));
+        std::unique_lock<std::mutex> lk(mCvLock);
+        while (!mCv.wait_for(lk, std::chrono::nanoseconds(sleepTime),
+                             [this] { return mThreadStop; })) {
+            checkTimeout();
+        }
+    });
+}
 
 PendingRequestPool::~PendingRequestPool() {
-    mThreadStop = true;
+    {
+        // Even if the shared variable is atomic, it must be modified under the
+        // mutex in order to correctly publish the modification to the waiting
+        // thread.
+        std::unique_lock<std::mutex> lk(mCvLock);
+        mThreadStop = true;
+    }
     mCv.notify_all();
     if (mThread.joinable()) {
         mThread.join();