Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 22ebbf78 authored by Keith Mok's avatar Keith Mok
Browse files

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
parent 85aebe3e

automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp

+10 −3
Original line number Diff line number Diff line
@@ -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() {

    {

        // 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();

automotive/vehicle/aidl/impl/fake_impl/GeneratorHub/src/GeneratorHub.cpp

+10 −3
Original line number Diff line number Diff line
@@ -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() {

    {

        // 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();

automotive/vehicle/aidl/impl/utils/common/include/PendingRequestPool.h

+1 −2
Original line number Diff line number Diff line
@@ -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;

automotive/vehicle/aidl/impl/utils/common/src/PendingRequestPool.cpp

+19 −12
Original line number Diff line number Diff line
@@ -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] {

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.load(); })) {

                             [this] { return mThreadStop; })) {

            checkTimeout();

        }

      }) {}

    });

}



PendingRequestPool::~PendingRequestPool() {

    {

        // 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();