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

Commit f4647e03 authored by Yu Shan's avatar Yu Shan
Browse files

Avoid holding lock while calling callback. 

Avoid holding lock while calling property store
OnValueChangeCallback. This might cause dead lock if
VehiclePropertyStore is accessed within the callback.

Test: atest VehiclePropertyStoreTest
Bug: 306511577
Change-Id: I5e29e9715d4429ccde5145af385a363bac548af7
parent 052608fe

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

+14 −11
Original line number Diff line number Diff line
@@ -92,7 +92,7 @@ class VehiclePropertyStore final { 
    // used as the key.

    void registerProperty(

            const aidl::android::hardware::automotive::vehicle::VehiclePropConfig& config,

            TokenFunction tokenFunc = nullptr);

            TokenFunction tokenFunc = nullptr) EXCLUDES(mLock);



    // Stores provided value. Returns error if config wasn't registered. If 'updateStatus' is

    // true, the 'status' in 'propValue' would be stored. Otherwise, if this is a new value,
@@ -102,44 +102,47 @@ class VehiclePropertyStore final { 
    // 'EventMode' controls whether the 'OnValueChangeCallback' will be called for this operation.

    VhalResult<void> writeValue(VehiclePropValuePool::RecyclableType propValue,

                                bool updateStatus = false,

                                EventMode mode = EventMode::ON_VALUE_CHANGE);

                                EventMode mode = EventMode::ON_VALUE_CHANGE) EXCLUDES(mLock);



    // Remove a given property value from the property store. The 'propValue' would be used to

    // generate the key for the value to remove.

    void removeValue(

            const aidl::android::hardware::automotive::vehicle::VehiclePropValue& propValue);

            const aidl::android::hardware::automotive::vehicle::VehiclePropValue& propValue)

            EXCLUDES(mLock);



    // Remove all the values for the property.

    void removeValuesForProperty(int32_t propId);

    void removeValuesForProperty(int32_t propId) EXCLUDES(mLock);



    // Read all the stored values.

    std::vector<VehiclePropValuePool::RecyclableType> readAllValues() const;

    std::vector<VehiclePropValuePool::RecyclableType> readAllValues() const EXCLUDES(mLock);



    // Read all the values for the property.

    ValuesResultType readValuesForProperty(int32_t propId) const;

    ValuesResultType readValuesForProperty(int32_t propId) const EXCLUDES(mLock);



    // Read the value for the requested property. Returns {@code StatusCode::NOT_AVAILABLE} if the

    // value has not been set yet. Returns {@code StatusCode::INVALID_ARG} if the property is

    // not configured.

    ValueResultType readValue(

            const aidl::android::hardware::automotive::vehicle::VehiclePropValue& request) const;

            const aidl::android::hardware::automotive::vehicle::VehiclePropValue& request) const

            EXCLUDES(mLock);



    // Read the value for the requested property. Returns {@code StatusCode::NOT_AVAILABLE} if the

    // value has not been set yet. Returns {@code StatusCode::INVALID_ARG} if the property is

    // not configured.

    ValueResultType readValue(int32_t prop, int32_t area = 0, int64_t token = 0) const;

    ValueResultType readValue(int32_t prop, int32_t area = 0, int64_t token = 0) const

            EXCLUDES(mLock);



    // Get all property configs.

    std::vector<aidl::android::hardware::automotive::vehicle::VehiclePropConfig> getAllConfigs()

            const;

            const EXCLUDES(mLock);



    // Get the property config for the requested property.

    android::base::Result<const aidl::android::hardware::automotive::vehicle::VehiclePropConfig*,

                          VhalError>

    getConfig(int32_t propId) const;

    getConfig(int32_t propId) const EXCLUDES(mLock);



    // Set a callback that would be called when a property value has been updated.

    void setOnValueChangeCallback(const OnValueChangeCallback& callback);

    void setOnValueChangeCallback(const OnValueChangeCallback& callback) EXCLUDES(mLock);



    inline std::shared_ptr<VehiclePropValuePool> getValuePool() { return mValuePool; }

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

+46 −35
Original line number Diff line number Diff line
@@ -108,30 +108,34 @@ void VehiclePropertyStore::registerProperty(const VehiclePropConfig& config, 
VhalResult<void> VehiclePropertyStore::writeValue(VehiclePropValuePool::RecyclableType propValue,

                                                  bool updateStatus,

                                                  VehiclePropertyStore::EventMode eventMode) {

    bool valueUpdated = true;

    VehiclePropValue updatedValue;

    OnValueChangeCallback onValueChangeCallback = nullptr;

    {

        std::scoped_lock<std::mutex> g(mLock);



        int32_t propId = propValue->prop;



        VehiclePropertyStore::Record* record = getRecordLocked(propId);

        if (record == nullptr) {

        return StatusError(StatusCode::INVALID_ARG) << "property: " << propId << " not registered";

            return StatusError(StatusCode::INVALID_ARG)

                   << "property: " << propId << " not registered";

        }



        if (!isGlobalProp(propId) && getAreaConfig(*propValue, record->propConfig) == nullptr) {

            return StatusError(StatusCode::INVALID_ARG)

               << "no config for property: " << propId << " area: " << propValue->areaId;

                   << "no config for property: " << propId << " area ID: " << propValue->areaId;

        }



        VehiclePropertyStore::RecordId recId = getRecordIdLocked(*propValue, *record);

    bool valueUpdated = true;

        if (auto it = record->values.find(recId); it != record->values.end()) {

            const VehiclePropValue* valueToUpdate = it->second.get();

        int64_t oldTimestamp = valueToUpdate->timestamp;

            int64_t oldTimestampNanos = valueToUpdate->timestamp;

            VehiclePropertyStatus oldStatus = valueToUpdate->status;

            // propValue is outdated and drops it.

        if (oldTimestamp > propValue->timestamp) {

            if (oldTimestampNanos > propValue->timestamp) {

                return StatusError(StatusCode::INVALID_ARG)

                   << "outdated timestamp: " << propValue->timestamp;

                       << "outdated timestampNanos: " << propValue->timestamp;

            }

            if (!updateStatus) {

                propValue->status = oldStatus;
@@ -150,9 +154,16 @@ VhalResult<void> VehiclePropertyStore::writeValue(VehiclePropValuePool::Recyclab 
        if (eventMode == EventMode::NEVER) {

            return {};

        }

        updatedValue = *(record->values[recId]);

        if (mOnValueChangeCallback == nullptr) {

            return {};

        }

        onValueChangeCallback = mOnValueChangeCallback;

    }



    if ((eventMode == EventMode::ALWAYS || valueUpdated) && mOnValueChangeCallback != nullptr) {

        mOnValueChangeCallback(*(record->values[recId]));

    // Invoke the callback outside the lock to prevent dead-lock.

    if (eventMode == EventMode::ALWAYS || valueUpdated) {

        onValueChangeCallback(updatedValue);

    }

    return {};

}

automotive/vehicle/aidl/impl/utils/common/test/VehiclePropertyStoreTest.cpp

+18 −0
Original line number Diff line number Diff line
@@ -509,6 +509,24 @@ TEST_F(VehiclePropertyStoreTest, testPropertyChangeCallbackForceNoUpdate) { 
    ASSERT_EQ(updatedValue.prop, INVALID_PROP_ID);

}



TEST_F(VehiclePropertyStoreTest, testPropertyChangeCallbackUseVehiclePropertyStore_noDeadLock) {

    VehiclePropValue fuelCapacity = {

            .prop = toInt(VehicleProperty::INFO_FUEL_CAPACITY),

            .value = {.floatValues = {1.0}},

    };



    std::vector<VehiclePropConfig> configs;



    mStore->setOnValueChangeCallback(

            [this, &configs]([[maybe_unused]] const VehiclePropValue& value) {

                configs = mStore->getAllConfigs();

            });



    ASSERT_RESULT_OK(mStore->writeValue(mValuePool->obtain(fuelCapacity), /*updateStatus=*/true,

                                        VehiclePropertyStore::EventMode::ALWAYS));

    ASSERT_EQ(configs.size(), static_cast<size_t>(2));

}



}  // namespace vehicle

}  // namespace automotive

}  // namespace hardware