From 03bc75f2984c4e9cbe45004f700df8916c298c9c Mon Sep 17 00:00:00 2001 From: Kevin Tang Date: Fri, 15 Dec 2017 18:53:25 -0800 Subject: [PATCH] fixing the location api level race condition on callbacks At the layer right under HIDL impl, where the callback objs are received from HIDL and used by HAL layer, there is race condition volnerability which could yield using a incompletely copied sp obj. Added mutex protection. Change-Id: I611db590d1fadbe43c74db71a1ea906dbe067c6d CRs-Fixed: 2144976 --- android/location_api/GnssAPIClient.cpp | 31 ++++++++++++++----- android/location_api/GnssAPIClient.h | 5 ++- android/location_api/MeasurementAPIClient.cpp | 10 ++++-- android/location_api/MeasurementAPIClient.h | 4 +-- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/android/location_api/GnssAPIClient.cpp b/android/location_api/GnssAPIClient.cpp index 4d0ca3b1..996bbeea 100644 --- a/android/location_api/GnssAPIClient.cpp +++ b/android/location_api/GnssAPIClient.cpp @@ -81,8 +81,10 @@ void GnssAPIClient::gnssUpdateCallbacks(const sp& gpsCb, { LOC_LOGD("%s]: (%p %p)", __FUNCTION__, &gpsCb, &niCb); + mMutex.lock(); mGnssCbIface = gpsCb; mGnssNiCbIface = niCb; + mMutex.unlock(); LocationCallbacks locationCallbacks; memset(&locationCallbacks, 0, sizeof(LocationCallbacks)); @@ -278,7 +280,10 @@ void GnssAPIClient::onCapabilitiesCb(LocationCapabilitiesMask capabilitiesMask) LOC_LOGD("%s]: (%02x)", __FUNCTION__, capabilitiesMask); mLocationCapabilitiesMask = capabilitiesMask; mLocationCapabilitiesCached = true; - sp gnssCbIface = mGnssCbIface; + + mMutex.lock(); + auto gnssCbIface(mGnssCbIface); + mMutex.unlock(); if (gnssCbIface != nullptr) { uint32_t data = 0; @@ -322,7 +327,9 @@ void GnssAPIClient::onCapabilitiesCb(LocationCapabilitiesMask capabilitiesMask) void GnssAPIClient::onTrackingCb(Location location) { LOC_LOGD("%s]: (flags: %02x)", __FUNCTION__, location.flags); - sp gnssCbIface = mGnssCbIface; + mMutex.lock(); + auto gnssCbIface(mGnssCbIface); + mMutex.unlock(); if (gnssCbIface != nullptr) { GnssLocation gnssLocation; @@ -338,7 +345,9 @@ void GnssAPIClient::onTrackingCb(Location location) void GnssAPIClient::onGnssNiCb(uint32_t id, GnssNiNotification gnssNiNotification) { LOC_LOGD("%s]: (id: %d)", __FUNCTION__, id); - sp gnssNiCbIface = mGnssNiCbIface; + mMutex.lock(); + auto gnssNiCbIface(mGnssNiCbIface); + mMutex.unlock(); if (gnssNiCbIface == nullptr) { LOC_LOGE("%s]: mGnssNiCbIface is nullptr", __FUNCTION__); @@ -415,7 +424,9 @@ void GnssAPIClient::onGnssNiCb(uint32_t id, GnssNiNotification gnssNiNotificatio void GnssAPIClient::onGnssSvCb(GnssSvNotification gnssSvNotification) { LOC_LOGD("%s]: (count: %zu)", __FUNCTION__, gnssSvNotification.count); - sp gnssCbIface = mGnssCbIface; + mMutex.lock(); + auto gnssCbIface(mGnssCbIface); + mMutex.unlock(); if (gnssCbIface != nullptr) { IGnssCallback::GnssSvStatus svStatus; @@ -430,7 +441,9 @@ void GnssAPIClient::onGnssSvCb(GnssSvNotification gnssSvNotification) void GnssAPIClient::onGnssNmeaCb(GnssNmeaNotification gnssNmeaNotification) { - sp gnssCbIface = mGnssCbIface; + mMutex.lock(); + auto gnssCbIface(mGnssCbIface); + mMutex.unlock(); if (gnssCbIface != nullptr) { android::hardware::hidl_string nmeaString; @@ -447,7 +460,9 @@ void GnssAPIClient::onGnssNmeaCb(GnssNmeaNotification gnssNmeaNotification) void GnssAPIClient::onStartTrackingCb(LocationError error) { LOC_LOGD("%s]: (%d)", __FUNCTION__, error); - sp gnssCbIface = mGnssCbIface; + mMutex.lock(); + auto gnssCbIface(mGnssCbIface); + mMutex.unlock(); if (error == LOCATION_ERROR_SUCCESS && gnssCbIface != nullptr) { auto r = gnssCbIface->gnssStatusCb(IGnssCallback::GnssStatusValue::ENGINE_ON); @@ -466,7 +481,9 @@ void GnssAPIClient::onStartTrackingCb(LocationError error) void GnssAPIClient::onStopTrackingCb(LocationError error) { LOC_LOGD("%s]: (%d)", __FUNCTION__, error); - sp gnssCbIface = mGnssCbIface; + mMutex.lock(); + auto gnssCbIface(mGnssCbIface); + mMutex.unlock(); if (error == LOCATION_ERROR_SUCCESS && gnssCbIface != nullptr) { auto r = gnssCbIface->gnssStatusCb(IGnssCallback::GnssStatusValue::SESSION_END); diff --git a/android/location_api/GnssAPIClient.h b/android/location_api/GnssAPIClient.h index 295a9e9b..b5cffb1e 100644 --- a/android/location_api/GnssAPIClient.h +++ b/android/location_api/GnssAPIClient.h @@ -30,7 +30,7 @@ #ifndef GNSS_API_CLINET_H #define GNSS_API_CLINET_H - +#include #include #include #include @@ -91,11 +91,10 @@ public: private: sp mGnssCbIface; sp mGnssNiCbIface; - + std::mutex mMutex; LocationAPIControlClient* mControlClient; LocationCapabilitiesMask mLocationCapabilitiesMask; bool mLocationCapabilitiesCached; - LocationOptions mLocationOptions; }; diff --git a/android/location_api/MeasurementAPIClient.cpp b/android/location_api/MeasurementAPIClient.cpp index dd7ceac5..731c7eda 100644 --- a/android/location_api/MeasurementAPIClient.cpp +++ b/android/location_api/MeasurementAPIClient.cpp @@ -66,7 +66,9 @@ MeasurementAPIClient::measurementSetCallback(const sp& { LOC_LOGD("%s]: (%p)", __FUNCTION__, &callback); + mMutex.lock(); mGnssMeasurementCbIface = callback; + mMutex.unlock(); LocationCallbacks locationCallbacks; memset(&locationCallbacks, 0, sizeof(LocationCallbacks)); @@ -116,10 +118,14 @@ void MeasurementAPIClient::onGnssMeasurementsCb( LOC_LOGD("%s]: (count: %zu active: %zu)", __FUNCTION__, gnssMeasurementsNotification.count, mTracking); if (mTracking) { - if (mGnssMeasurementCbIface != nullptr) { + mMutex.lock(); + auto gnssMeasurementCbIface(mGnssMeasurementCbIface); + mMutex.unlock(); + + if (gnssMeasurementCbIface != nullptr) { IGnssMeasurementCallback::GnssData gnssData; convertGnssData(gnssMeasurementsNotification, gnssData); - auto r = mGnssMeasurementCbIface->GnssMeasurementCb(gnssData); + auto r = gnssMeasurementCbIface->GnssMeasurementCb(gnssData); if (!r.isOk()) { LOC_LOGE("%s] Error from GnssMeasurementCb description=%s", __func__, r.description().c_str()); diff --git a/android/location_api/MeasurementAPIClient.h b/android/location_api/MeasurementAPIClient.h index 422564d4..8de13264 100644 --- a/android/location_api/MeasurementAPIClient.h +++ b/android/location_api/MeasurementAPIClient.h @@ -30,7 +30,7 @@ #ifndef MEASUREMENT_API_CLINET_H #define MEASUREMENT_API_CLINET_H - +#include #include #include #include @@ -63,7 +63,7 @@ public: private: sp mGnssMeasurementCbIface; - + std::mutex mMutex; bool mTracking; };