From acd65a80a9c8f9e8c2f6be47fdf09e90bebbd357 Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Tue, 24 Sep 2024 21:41:57 +0000 Subject: [PATCH] Use read-write lock to guard property config. Property config now may change dynamically due to late-init. We might also support dynamic config later. This CL uses read write lock to properly guard access to config map. Flag: EXEMPT HAL change Test: atest DefaultVehicleHalTest Bug: 342470570 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:35401eb1ba251507ed68748b19b0c4a2fcc56c40) Merged-In: I0df79040c363e66baab48631f457752532d2967d Change-Id: I0df79040c363e66baab48631f457752532d2967d --- .../impl/vhal/include/DefaultVehicleHal.h | 35 ++- .../aidl/impl/vhal/src/DefaultVehicleHal.cpp | 210 +++++++++++------- 2 files changed, 158 insertions(+), 87 deletions(-) diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h index fa2a310d4e..b58d0f5302 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h +++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -138,12 +139,11 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { // Only used for testing. int32_t mTestInterfaceVersion = 0; - // mConfigsByPropId and mConfigFile is lazy initialized. - mutable std::mutex mConfigInitLock; - mutable bool mConfigInit GUARDED_BY(mConfigInitLock) = false; + mutable std::atomic mConfigInit = false; + mutable std::shared_timed_mutex mConfigLock; mutable std::unordered_map mConfigsByPropId - GUARDED_BY(mConfigInitLock); - mutable std::unique_ptr mConfigFile GUARDED_BY(mConfigInitLock); + GUARDED_BY(mConfigLock); + mutable std::unique_ptr mConfigFile GUARDED_BY(mConfigLock); std::mutex mLock; std::unordered_map> mOnBinderDiedContexts @@ -175,7 +175,10 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { android::base::Result> checkDuplicateRequests( const std::vector& requests); - VhalResult checkSubscribeOptions(const std::vector& options); + VhalResult checkSubscribeOptions( + const std::vector& options, + const std::unordered_map& configsByPropId) + REQUIRES_SHARED(mConfigLock); VhalResult checkPermissionHelper(const aidlvhal::VehiclePropValue& value, aidlvhal::VehiclePropertyAccess accessToTest) const; @@ -184,7 +187,7 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { VhalResult checkWritePermission(const aidlvhal::VehiclePropValue& value) const; - android::base::Result getConfig(int32_t propId) const; + android::base::Result getConfig(int32_t propId) const; void onBinderDiedWithContext(const AIBinder* clientId); @@ -196,7 +199,7 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { bool checkDumpPermission(); - bool getAllPropConfigsFromHardwareLocked() const REQUIRES(mConfigInitLock); + bool getAllPropConfigsFromHardwareLocked() const EXCLUDES(mConfigLock); // The looping handler function to process all onBinderDied or onBinderUnlinked events in // mBinderEvents. @@ -209,10 +212,12 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { int32_t getVhalInterfaceVersion() const; - // Gets mConfigsByPropId, lazy init it if necessary. - const std::unordered_map& getConfigsByPropId() const; - // Gets mConfigFile, lazy init it if necessary. - const ndk::ScopedFileDescriptor* getConfigFile() const; + // Gets mConfigsByPropId, lazy init it if necessary. Note that the reference is only valid in + // the scope of the callback and it is guaranteed that read lock is obtained during the + // callback. + void getConfigsByPropId( + std::function&)> + callback) const EXCLUDES(mConfigLock); // Puts the property change events into a queue so that they can handled in batch. static void batchPropertyChangeEvent( @@ -239,6 +244,12 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { static void onBinderUnlinked(void* cookie); + static void parseSubscribeOptions( + const std::vector& options, + const std::unordered_map& configsByPropId, + std::vector& onChangeSubscriptions, + std::vector& continuousSubscriptions); + // Test-only // Set the default timeout for pending requests. void setTimeout(int64_t timeoutInNano); diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp index 9dc039df13..e062a286b2 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp @@ -95,6 +95,18 @@ float getDefaultSampleRateHz(float sampleRateHz, float minSampleRateHz, float ma return sampleRateHz; } +class SCOPED_CAPABILITY SharedScopedLockAssertion { + public: + SharedScopedLockAssertion(std::shared_timed_mutex& mutex) ACQUIRE_SHARED(mutex) {} + ~SharedScopedLockAssertion() RELEASE() {} +}; + +class SCOPED_CAPABILITY UniqueScopedLockAssertion { + public: + UniqueScopedLockAssertion(std::shared_timed_mutex& mutex) ACQUIRE(mutex) {} + ~UniqueScopedLockAssertion() RELEASE() {} +}; + } // namespace DefaultVehicleHal::DefaultVehicleHal(std::unique_ptr vehicleHardware) @@ -355,68 +367,82 @@ bool DefaultVehicleHal::getAllPropConfigsFromHardwareLocked() const { } filteredConfigs.push_back(std::move(config)); } - for (auto& config : filteredConfigs) { - mConfigsByPropId[config.prop] = config; - } - VehiclePropConfigs vehiclePropConfigs; - vehiclePropConfigs.payloads = std::move(filteredConfigs); - auto result = LargeParcelableBase::parcelableToStableLargeParcelable(vehiclePropConfigs); - if (!result.ok()) { - ALOGE("failed to convert configs to shared memory file, error: %s, code: %d", - result.error().message().c_str(), static_cast(result.error().code())); - mConfigFile = nullptr; - return false; + + { + std::unique_lock configWriteLock(mConfigLock); + UniqueScopedLockAssertion lockAssertion(mConfigLock); + + for (auto& config : filteredConfigs) { + mConfigsByPropId[config.prop] = config; + } + VehiclePropConfigs vehiclePropConfigs; + vehiclePropConfigs.payloads = std::move(filteredConfigs); + auto result = LargeParcelableBase::parcelableToStableLargeParcelable(vehiclePropConfigs); + if (!result.ok()) { + ALOGE("failed to convert configs to shared memory file, error: %s, code: %d", + result.error().message().c_str(), static_cast(result.error().code())); + mConfigFile = nullptr; + return false; + } + + if (result.value() != nullptr) { + mConfigFile = std::move(result.value()); + } } - if (result.value() != nullptr) { - mConfigFile = std::move(result.value()); - } + mConfigInit = true; return true; } -const ScopedFileDescriptor* DefaultVehicleHal::getConfigFile() const { - std::scoped_lock lockGuard(mConfigInitLock); +void DefaultVehicleHal::getConfigsByPropId( + std::function&)> callback) const { if (!mConfigInit) { CHECK(getAllPropConfigsFromHardwareLocked()) << "Failed to get property configs from hardware"; - mConfigInit = true; } - return mConfigFile.get(); -} -const std::unordered_map& DefaultVehicleHal::getConfigsByPropId() - const { - std::scoped_lock lockGuard(mConfigInitLock); - if (!mConfigInit) { - CHECK(getAllPropConfigsFromHardwareLocked()) - << "Failed to get property configs from hardware"; - mConfigInit = true; - } - return mConfigsByPropId; + std::shared_lock configReadLock(mConfigLock); + SharedScopedLockAssertion lockAssertion(mConfigLock); + + callback(mConfigsByPropId); } ScopedAStatus DefaultVehicleHal::getAllPropConfigs(VehiclePropConfigs* output) { - const ScopedFileDescriptor* configFile = getConfigFile(); - const auto& configsByPropId = getConfigsByPropId(); - if (configFile != nullptr) { + if (!mConfigInit) { + CHECK(getAllPropConfigsFromHardwareLocked()) + << "Failed to get property configs from hardware"; + } + + std::shared_lock configReadLock(mConfigLock); + SharedScopedLockAssertion lockAssertion(mConfigLock); + + if (mConfigFile != nullptr) { output->payloads.clear(); - output->sharedMemoryFd.set(dup(configFile->get())); + output->sharedMemoryFd.set(dup(mConfigFile->get())); return ScopedAStatus::ok(); } - output->payloads.reserve(configsByPropId.size()); - for (const auto& [_, config] : configsByPropId) { + + output->payloads.reserve(mConfigsByPropId.size()); + for (const auto& [_, config] : mConfigsByPropId) { output->payloads.push_back(config); } return ScopedAStatus::ok(); } -Result DefaultVehicleHal::getConfig(int32_t propId) const { - const auto& configsByPropId = getConfigsByPropId(); - auto it = configsByPropId.find(propId); - if (it == configsByPropId.end()) { - return Error() << "no config for property, ID: " << propId; - } - return &(it->second); +Result DefaultVehicleHal::getConfig(int32_t propId) const { + Result result; + getConfigsByPropId([this, &result, propId](const auto& configsByPropId) { + SharedScopedLockAssertion lockAssertion(mConfigLock); + + auto it = configsByPropId.find(propId); + if (it == configsByPropId.end()) { + result = Error() << "no config for property, ID: " << propId; + return; + } + // Copy the VehiclePropConfig + result = it->second; + }); + return result; } Result DefaultVehicleHal::checkProperty(const VehiclePropValue& propValue) { @@ -425,15 +451,15 @@ Result DefaultVehicleHal::checkProperty(const VehiclePropValue& propValue) if (!result.ok()) { return result.error(); } - const VehiclePropConfig* config = result.value(); - const VehicleAreaConfig* areaConfig = getAreaConfig(propValue, *config); + const VehiclePropConfig& config = result.value(); + const VehicleAreaConfig* areaConfig = getAreaConfig(propValue, config); if (!isGlobalProp(propId) && areaConfig == nullptr) { // Ignore areaId for global property. For non global property, check whether areaId is // allowed. areaId must appear in areaConfig. return Error() << "invalid area ID: " << propValue.areaId << " for prop ID: " << propId << ", not listed in config"; } - if (auto result = checkPropValue(propValue, config); !result.ok()) { + if (auto result = checkPropValue(propValue, &config); !result.ok()) { return Error() << "invalid property value: " << propValue.toString() << ", error: " << getErrorMsg(result); } @@ -659,17 +685,27 @@ ScopedAStatus DefaultVehicleHal::setValues(const CallbackType& callback, ScopedAStatus DefaultVehicleHal::getPropConfigs(const std::vector& props, VehiclePropConfigs* output) { std::vector configs; - const auto& configsByPropId = getConfigsByPropId(); - for (int32_t prop : props) { - auto it = configsByPropId.find(prop); - if (it != configsByPropId.end()) { - configs.push_back(it->second); - } else { - return ScopedAStatus::fromServiceSpecificErrorWithMessage( - toInt(StatusCode::INVALID_ARG), - StringPrintf("no config for property, ID: %" PRId32, prop).c_str()); + ScopedAStatus status = ScopedAStatus::ok(); + getConfigsByPropId([this, &configs, &status, &props](const auto& configsByPropId) { + SharedScopedLockAssertion lockAssertion(mConfigLock); + + for (int32_t prop : props) { + auto it = configsByPropId.find(prop); + if (it != configsByPropId.end()) { + configs.push_back(it->second); + } else { + status = ScopedAStatus::fromServiceSpecificErrorWithMessage( + toInt(StatusCode::INVALID_ARG), + StringPrintf("no config for property, ID: %" PRId32, prop).c_str()); + return; + } } + }); + + if (!status.isOk()) { + return status; } + return vectorToStableLargeParcelable(std::move(configs), output); } @@ -691,8 +727,8 @@ bool areaConfigsHaveRequiredAccess(const std::vector& areaCon } VhalResult DefaultVehicleHal::checkSubscribeOptions( - const std::vector& options) { - const auto& configsByPropId = getConfigsByPropId(); + const std::vector& options, + const std::unordered_map& configsByPropId) { for (const auto& option : options) { int32_t propId = option.propId; auto it = configsByPropId.find(propId); @@ -757,23 +793,15 @@ VhalResult DefaultVehicleHal::checkSubscribeOptions( } } } + return {}; } -ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback, - const std::vector& options, - [[maybe_unused]] int32_t maxSharedMemoryFileCount) { - // TODO(b/205189110): Use shared memory file count. - if (callback == nullptr) { - return ScopedAStatus::fromExceptionCode(EX_NULL_POINTER); - } - if (auto result = checkSubscribeOptions(options); !result.ok()) { - ALOGE("subscribe: invalid subscribe options: %s", getErrorMsg(result).c_str()); - return toScopedAStatus(result); - } - std::vector onChangeSubscriptions; - std::vector continuousSubscriptions; - const auto& configsByPropId = getConfigsByPropId(); +void DefaultVehicleHal::parseSubscribeOptions( + const std::vector& options, + const std::unordered_map& configsByPropId, + std::vector& onChangeSubscriptions, + std::vector& continuousSubscriptions) { for (const auto& option : options) { int32_t propId = option.propId; // We have already validate config exists. @@ -831,6 +859,34 @@ ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback, onChangeSubscriptions.push_back(std::move(optionCopy)); } } +} + +ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback, + const std::vector& options, + [[maybe_unused]] int32_t maxSharedMemoryFileCount) { + // TODO(b/205189110): Use shared memory file count. + if (callback == nullptr) { + return ScopedAStatus::fromExceptionCode(EX_NULL_POINTER); + } + std::vector onChangeSubscriptions; + std::vector continuousSubscriptions; + ScopedAStatus returnStatus = ScopedAStatus::ok(); + getConfigsByPropId([this, &returnStatus, &options, &onChangeSubscriptions, + &continuousSubscriptions](const auto& configsByPropId) { + SharedScopedLockAssertion lockAssertion(mConfigLock); + + if (auto result = checkSubscribeOptions(options, configsByPropId); !result.ok()) { + ALOGE("subscribe: invalid subscribe options: %s", getErrorMsg(result).c_str()); + returnStatus = toScopedAStatus(result); + return; + } + parseSubscribeOptions(options, configsByPropId, onChangeSubscriptions, + continuousSubscriptions); + }); + + if (!returnStatus.isOk()) { + return returnStatus; + } { // Lock to make sure onBinderDied would not be called concurrently. @@ -891,13 +947,13 @@ VhalResult DefaultVehicleHal::checkPermissionHelper( return StatusError(StatusCode::INVALID_ARG) << getErrorMsg(result); } - const VehiclePropConfig* config = result.value(); - const VehicleAreaConfig* areaConfig = getAreaConfig(value, *config); + const VehiclePropConfig& config = result.value(); + const VehicleAreaConfig* areaConfig = getAreaConfig(value, config); if (areaConfig == nullptr && !isGlobalProp(propId)) { return StatusError(StatusCode::INVALID_ARG) << "no config for area ID: " << value.areaId; } - if (!hasRequiredAccess(config->access, accessToTest) && + if (!hasRequiredAccess(config.access, accessToTest) && (areaConfig == nullptr || !hasRequiredAccess(areaConfig->access, accessToTest))) { return StatusError(StatusCode::ACCESS_DENIED) << StringPrintf("Property %" PRId32 " does not have the following access: %" PRId32, @@ -966,7 +1022,6 @@ binder_status_t DefaultVehicleHal::dump(int fd, const char** args, uint32_t numA } DumpResult result = mVehicleHardware->dump(options); if (result.refreshPropertyConfigs) { - std::scoped_lock lockGuard(mConfigInitLock); getAllPropConfigsFromHardwareLocked(); } dprintf(fd, "%s", (result.buffer + "\n").c_str()); @@ -974,11 +1029,16 @@ binder_status_t DefaultVehicleHal::dump(int fd, const char** args, uint32_t numA return STATUS_OK; } dprintf(fd, "Vehicle HAL State: \n"); - const auto& configsByPropId = getConfigsByPropId(); + std::unordered_map configsByPropIdCopy; + getConfigsByPropId([this, &configsByPropIdCopy](const auto& configsByPropId) { + SharedScopedLockAssertion lockAssertion(mConfigLock); + + configsByPropIdCopy = configsByPropId; + }); { std::scoped_lock lockGuard(mLock); dprintf(fd, "Interface version: %" PRId32 "\n", getVhalInterfaceVersion()); - dprintf(fd, "Containing %zu property configs\n", configsByPropId.size()); + dprintf(fd, "Containing %zu property configs\n", configsByPropIdCopy.size()); dprintf(fd, "Currently have %zu getValues clients\n", mGetValuesClients.size()); dprintf(fd, "Currently have %zu setValues clients\n", mSetValuesClients.size()); dprintf(fd, "Currently have %zu subscribe clients\n", countSubscribeClients());