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
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 9be9ea7..503afd2 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 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 1690c78..d815456 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 @@
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 3f8db93..28cf08e 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 @@
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 0196edd..ab50499 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 @@
} // 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();