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.
Bug: 79541486
Change-Id: I611db590d1fadbe43c74db71a1ea906dbe067c6d
CRs-Fixed: 2144976
(cherry picked from commit f1ee0f801e75dc89ff776bdefd148c63d128b6b4)
diff --git a/msm8909w_3100/android/location_api/GnssAPIClient.cpp b/msm8909w_3100/android/location_api/GnssAPIClient.cpp
index 2745f56..2e44c2e 100644
--- a/msm8909w_3100/android/location_api/GnssAPIClient.cpp
+++ b/msm8909w_3100/android/location_api/GnssAPIClient.cpp
@@ -81,8 +81,10 @@
{
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 @@
LOC_LOGD("%s]: (%02x)", __FUNCTION__, capabilitiesMask);
mLocationCapabilitiesMask = capabilitiesMask;
mLocationCapabilitiesCached = true;
- sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+
+ mMutex.lock();
+ auto gnssCbIface(mGnssCbIface);
+ mMutex.unlock();
if (gnssCbIface != nullptr) {
uint32_t data = 0;
@@ -322,7 +327,9 @@
void GnssAPIClient::onTrackingCb(Location location)
{
LOC_LOGD("%s]: (flags: %02x)", __FUNCTION__, location.flags);
- sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+ mMutex.lock();
+ auto gnssCbIface(mGnssCbIface);
+ mMutex.unlock();
if (gnssCbIface != nullptr) {
GnssLocation gnssLocation;
@@ -338,7 +345,9 @@
void GnssAPIClient::onGnssNiCb(uint32_t id, GnssNiNotification gnssNiNotification)
{
LOC_LOGD("%s]: (id: %d)", __FUNCTION__, id);
- sp<IGnssNiCallback> gnssNiCbIface = mGnssNiCbIface;
+ mMutex.lock();
+ auto gnssNiCbIface(mGnssNiCbIface);
+ mMutex.unlock();
if (gnssNiCbIface == nullptr) {
LOC_LOGE("%s]: mGnssNiCbIface is nullptr", __FUNCTION__);
@@ -411,7 +420,9 @@
void GnssAPIClient::onGnssSvCb(GnssSvNotification gnssSvNotification)
{
LOC_LOGD("%s]: (count: %zu)", __FUNCTION__, gnssSvNotification.count);
- sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+ mMutex.lock();
+ auto gnssCbIface(mGnssCbIface);
+ mMutex.unlock();
if (gnssCbIface != nullptr) {
IGnssCallback::GnssSvStatus svStatus;
@@ -426,7 +437,9 @@
void GnssAPIClient::onGnssNmeaCb(GnssNmeaNotification gnssNmeaNotification)
{
- sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+ mMutex.lock();
+ auto gnssCbIface(mGnssCbIface);
+ mMutex.unlock();
if (gnssCbIface != nullptr) {
android::hardware::hidl_string nmeaString;
@@ -443,7 +456,9 @@
void GnssAPIClient::onStartTrackingCb(LocationError error)
{
LOC_LOGD("%s]: (%d)", __FUNCTION__, error);
- sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+ mMutex.lock();
+ auto gnssCbIface(mGnssCbIface);
+ mMutex.unlock();
if (error == LOCATION_ERROR_SUCCESS && gnssCbIface != nullptr) {
auto r = gnssCbIface->gnssStatusCb(IGnssCallback::GnssStatusValue::ENGINE_ON);
@@ -462,7 +477,9 @@
void GnssAPIClient::onStopTrackingCb(LocationError error)
{
LOC_LOGD("%s]: (%d)", __FUNCTION__, error);
- sp<IGnssCallback> 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/msm8909w_3100/android/location_api/GnssAPIClient.h b/msm8909w_3100/android/location_api/GnssAPIClient.h
index 295a9e9..b5cffb1 100644
--- a/msm8909w_3100/android/location_api/GnssAPIClient.h
+++ b/msm8909w_3100/android/location_api/GnssAPIClient.h
@@ -30,7 +30,7 @@
#ifndef GNSS_API_CLINET_H
#define GNSS_API_CLINET_H
-
+#include <mutex>
#include <android/hardware/gnss/1.0/IGnss.h>
#include <android/hardware/gnss/1.0/IGnssCallback.h>
#include <android/hardware/gnss/1.0/IGnssNiCallback.h>
@@ -91,11 +91,10 @@
private:
sp<IGnssCallback> mGnssCbIface;
sp<IGnssNiCallback> mGnssNiCbIface;
-
+ std::mutex mMutex;
LocationAPIControlClient* mControlClient;
LocationCapabilitiesMask mLocationCapabilitiesMask;
bool mLocationCapabilitiesCached;
-
LocationOptions mLocationOptions;
};
diff --git a/msm8909w_3100/android/location_api/MeasurementAPIClient.cpp b/msm8909w_3100/android/location_api/MeasurementAPIClient.cpp
index dd7ceac..731c7ed 100644
--- a/msm8909w_3100/android/location_api/MeasurementAPIClient.cpp
+++ b/msm8909w_3100/android/location_api/MeasurementAPIClient.cpp
@@ -66,7 +66,9 @@
{
LOC_LOGD("%s]: (%p)", __FUNCTION__, &callback);
+ mMutex.lock();
mGnssMeasurementCbIface = callback;
+ mMutex.unlock();
LocationCallbacks locationCallbacks;
memset(&locationCallbacks, 0, sizeof(LocationCallbacks));
@@ -116,10 +118,14 @@
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/msm8909w_3100/android/location_api/MeasurementAPIClient.h b/msm8909w_3100/android/location_api/MeasurementAPIClient.h
index 422564d..8de1326 100644
--- a/msm8909w_3100/android/location_api/MeasurementAPIClient.h
+++ b/msm8909w_3100/android/location_api/MeasurementAPIClient.h
@@ -30,7 +30,7 @@
#ifndef MEASUREMENT_API_CLINET_H
#define MEASUREMENT_API_CLINET_H
-
+#include <mutex>
#include <android/hardware/gnss/1.0/IGnssMeasurement.h>
#include <android/hardware/gnss/1.0/IGnssMeasurementCallback.h>
#include <LocationAPIClientBase.h>
@@ -63,7 +63,7 @@
private:
sp<IGnssMeasurementCallback> mGnssMeasurementCbIface;
-
+ std::mutex mMutex;
bool mTracking;
};
