Fix for race condition between radio and oemhook services.

The race condition was this:
1. Phone process crashes and restarts
2. Phone process calls IRadio.setResponseFunctions()
3. oemHookInd is sent from vendor RIL. At this point oemHook
callbacks are stale, and due to that the callback fails and
sets callback objects to NULL, including the IRadio ones which is
not correct.

Test: Basic telephony sanity. The condition was easily reproducible
on angler; verified that it works fine now.
Bug: 32020264

Change-Id: I33bbdf01b19d009530c78baae90188acb4986d15

diff --git a/libril/ril_service.cpp b/libril/ril_service.cpp
index 5a95fe0..b61b844 100644 (file)
--- a/libril/ril_service.cpp
+++ b/libril/ril_service.cpp
@@ -57,12 +57,14 @@ struct OemHookImpl;
 sp<RadioImpl> radioService[SIM_COUNT];
 sp<OemHookImpl> oemHookService[SIM_COUNT];
 // counter used for synchronization. It is incremented every time response callbacks are updated.
-volatile int32_t mCounter[SIM_COUNT];
+volatile int32_t mCounterRadio[SIM_COUNT];
+volatile int32_t mCounterOemHook[SIM_COUNT];
 #else
 sp<RadioImpl> radioService[1];
 sp<OemHookImpl> oemHookService[1];
 // counter used for synchronization. It is incremented every time response callbacks are updated.
-volatile int32_t mCounter[1];
+volatile int32_t mCounterRadio[1];
+volatile int32_t mCounterOemHook[1];
 #endif
 
 static pthread_rwlock_t radioServiceRwlock = PTHREAD_RWLOCK_INITIALIZER;
@@ -696,7 +698,7 @@ bool dispatchIccApdu(int serial, int slotId, int request, const SimApdu& message
     return true;
 }
 
-void checkReturnStatus(int32_t slotId, Return<void>& ret) {
+void checkReturnStatus(int32_t slotId, Return<void>& ret, bool isRadioService) {
     if (ret.isOk() == false) {
         RLOGE("checkReturnStatus: unable to call response/indication callback");
         // Remote process hosting the callbacks must be dead. Reset the callback objects;
@@ -706,7 +708,7 @@ void checkReturnStatus(int32_t slotId, Return<void>& ret) {
         // Caller should already hold rdlock, release that first
         // note the current counter to avoid overwriting updates made by another thread before
         // write lock is acquired.
-        int counter = mCounter[slotId];
+        int counter = isRadioService ? mCounterRadio[slotId] : mCounterOemHook[slotId];
         pthread_rwlock_t *radioServiceRwlockPtr = radio::getRadioServiceRwlock(slotId);
         int ret = pthread_rwlock_unlock(radioServiceRwlockPtr);
         assert(ret == 0);
@@ -716,12 +718,15 @@ void checkReturnStatus(int32_t slotId, Return<void>& ret) {
         assert(ret == 0);
 
         // make sure the counter value has not changed
-        if (counter == mCounter[slotId]) {
-            radioService[slotId]->mRadioResponse = NULL;
-            radioService[slotId]->mRadioIndication = NULL;
-            oemHookService[slotId]->mOemHookResponse = NULL;
-            oemHookService[slotId]->mOemHookIndication = NULL;
-            mCounter[slotId]++;
+        if (counter == (isRadioService ? mCounterRadio[slotId] : mCounterOemHook[slotId])) {
+            if (isRadioService) {
+                radioService[slotId]->mRadioResponse = NULL;
+                radioService[slotId]->mRadioIndication = NULL;
+            } else {
+                oemHookService[slotId]->mOemHookResponse = NULL;
+                oemHookService[slotId]->mOemHookIndication = NULL;
+            }
+            isRadioService ? mCounterRadio[slotId]++ : mCounterOemHook[slotId]++;
         } else {
             RLOGE("checkReturnStatus: not resetting responseFunctions as they likely "
                     "got updated on another thread");
@@ -738,7 +743,7 @@ void checkReturnStatus(int32_t slotId, Return<void>& ret) {
 }
 
 void RadioImpl::checkReturnStatus(Return<void>& ret) {
-    ::checkReturnStatus(mSlotId, ret);
+    ::checkReturnStatus(mSlotId, ret, true);
 }
 
 Return<void> RadioImpl::setResponseFunctions(
@@ -752,7 +757,7 @@ Return<void> RadioImpl::setResponseFunctions(
 
     mRadioResponse = radioResponseParam;
     mRadioIndication = radioIndicationParam;
-    mCounter[mSlotId]++;
+    mCounterRadio[mSlotId]++;
 
     ret = pthread_rwlock_unlock(radioServiceRwlockPtr);
     assert(ret == 0);
@@ -2359,7 +2364,7 @@ Return<void> OemHookImpl::setResponseFunctions(
 
     mOemHookResponse = oemHookResponseParam;
     mOemHookIndication = oemHookIndicationParam;
-    mCounter[mSlotId]++;
+    mCounterOemHook[mSlotId]++;
 
     ret = pthread_rwlock_unlock(radioServiceRwlockPtr);
     assert(ret == 0);
@@ -5731,7 +5736,7 @@ int radio::sendRequestRawResponse(int slotId,
         }
         Return<void> retStatus = oemHookService[slotId]->mOemHookResponse->
                 sendRequestRawResponse(responseInfo, data);
-        checkReturnStatus(slotId, retStatus);
+        checkReturnStatus(slotId, retStatus, false);
     } else {
         RLOGE("sendRequestRawResponse: oemHookService[%d]->mOemHookResponse == NULL",
                 slotId);
@@ -5764,7 +5769,7 @@ int radio::sendRequestStringsResponse(int slotId,
         Return<void> retStatus
                 = oemHookService[slotId]->mOemHookResponse->sendRequestStringsResponse(
                 responseInfo, data);
-        checkReturnStatus(slotId, retStatus);
+        checkReturnStatus(slotId, retStatus, false);
     } else {
         RLOGE("sendRequestStringsResponse: oemHookService[%d]->mOemHookResponse == "
                 "NULL", slotId);
@@ -7278,7 +7283,7 @@ int radio::oemHookRawInd(int slotId,
         RLOGD("oemHookRawInd");
         Return<void> retStatus = oemHookService[slotId]->mOemHookIndication->oemHookRaw(
                 convertIntToRadioIndicationType(indicationType), data);
-        checkReturnStatus(slotId, retStatus);
+        checkReturnStatus(slotId, retStatus, false);
     } else {
         RLOGE("oemHookRawInd: oemHookService[%d]->mOemHookIndication == NULL", slotId);
     }