Move permission checks out of synchronized block
SubscriptionController#getSubscriptionInfoListFromCacheHelper is
invoked by a majority of the AIDL methods to obtain SubscriptionInfo
objects. This commit cuts down on lock contention by performing a
majority of the permission checks before the synchronized block;
with this only a single carrier privilege check may be required for
each subscription.
Fixes: 157642567
Test: atest SubscriptionControllerTest
Test: Manually invoked test app with multiple background threads
invoking SubscriptionManager methods to query for
SubscriptionInfo objects; saw an average 45-50ms improvement
Change-Id: I4d738955125ea4d39252fa582c41139fc5f6f784
diff --git a/src/java/com/android/internal/telephony/SubscriptionController.java b/src/java/com/android/internal/telephony/SubscriptionController.java
index 0ec9c67..3eeb63a 100644
--- a/src/java/com/android/internal/telephony/SubscriptionController.java
+++ b/src/java/com/android/internal/telephony/SubscriptionController.java
@@ -3611,7 +3611,7 @@
callingPackage, callingFeatureId, "getSubscriptionsInGroup")
|| info.canManageSubscription(mContext, callingPackage);
}).map(subscriptionInfo -> conditionallyRemoveIdentifiers(subscriptionInfo,
- callingPackage, callingFeatureId, "getSubscriptionInfoList"))
+ callingPackage, callingFeatureId, "getSubscriptionsInGroup"))
.collect(Collectors.toList());
}
@@ -3897,43 +3897,62 @@
// They are doing similar things except operating on different cache.
private List<SubscriptionInfo> getSubscriptionInfoListFromCacheHelper(
String callingPackage, String callingFeatureId, List<SubscriptionInfo> cacheSubList) {
- boolean canReadAllPhoneState;
+ boolean canReadPhoneState = false;
+ boolean canReadIdentifiers = false;
+ boolean canReadPhoneNumber = false;
try {
- canReadAllPhoneState = TelephonyPermissions.checkReadPhoneState(mContext,
+ canReadPhoneState = TelephonyPermissions.checkReadPhoneState(mContext,
SubscriptionManager.INVALID_SUBSCRIPTION_ID, Binder.getCallingPid(),
Binder.getCallingUid(), callingPackage, callingFeatureId,
"getSubscriptionInfoList");
// If the calling package has the READ_PHONE_STATE permission then check if the caller
- // also has access to subscriber identifiers to ensure that the ICC ID and any other
- // unique identifiers are removed if the caller should not have access.
- if (canReadAllPhoneState) {
- canReadAllPhoneState = hasSubscriberIdentifierAccess(
+ // also has access to subscriber identifiers and the phone number to ensure that the ICC
+ // ID and any other unique identifiers are removed if the caller should not have access.
+ if (canReadPhoneState) {
+ canReadIdentifiers = hasSubscriberIdentifierAccess(
+ SubscriptionManager.INVALID_SUBSCRIPTION_ID, callingPackage,
+ callingFeatureId, "getSubscriptionInfoList");
+ canReadPhoneNumber = hasPhoneNumberAccess(
SubscriptionManager.INVALID_SUBSCRIPTION_ID, callingPackage,
callingFeatureId, "getSubscriptionInfoList");
}
} catch (SecurityException e) {
- canReadAllPhoneState = false;
+ // If a SecurityException is thrown during the READ_PHONE_STATE check then the only way
+ // to access a subscription is to have carrier privileges for its subId; an app with
+ // carrier privileges for a subscription is also granted access to all identifiers so
+ // the identifier and phone number access checks are not required.
}
synchronized (mSubInfoListLock) {
// If the caller can read all phone state, just return the full list.
- if (canReadAllPhoneState) {
+ if (canReadIdentifiers && canReadPhoneNumber) {
return new ArrayList<>(cacheSubList);
}
-
// Filter the list to only include subscriptions which the caller can manage.
- return cacheSubList.stream()
- .filter(subscriptionInfo -> {
- try {
- return TelephonyPermissions.checkCallingOrSelfReadPhoneState(mContext,
- subscriptionInfo.getSubscriptionId(), callingPackage,
- callingFeatureId, "getSubscriptionInfoList");
- } catch (SecurityException e) {
- return false;
- }
- }).map(subscriptionInfo -> conditionallyRemoveIdentifiers(subscriptionInfo,
- callingPackage, callingFeatureId, "getSubscriptionInfoList"))
- .collect(Collectors.toList());
+ List<SubscriptionInfo> subscriptions = new ArrayList<>(cacheSubList.size());
+ for (SubscriptionInfo subscriptionInfo : cacheSubList) {
+ int subId = subscriptionInfo.getSubscriptionId();
+ boolean hasCarrierPrivileges = TelephonyPermissions.checkCarrierPrivilegeForSubId(
+ mContext, subId);
+ // If the caller does not have the READ_PHONE_STATE permission nor carrier
+ // privileges then they cannot access the current subscription.
+ if (!canReadPhoneState && !hasCarrierPrivileges) {
+ continue;
+ }
+ // If the caller has carrier privileges then they are granted access to all
+ // identifiers for their subscription.
+ if (hasCarrierPrivileges) {
+ subscriptions.add(subscriptionInfo);
+ } else {
+ // The caller does not have carrier privileges for this subId, filter the
+ // identifiers in the subscription based on the results of the initial
+ // permission checks.
+ subscriptions.add(
+ conditionallyRemoveIdentifiers(subscriptionInfo, canReadIdentifiers,
+ canReadPhoneNumber));
+ }
+ }
+ return subscriptions;
}
}
@@ -3954,15 +3973,30 @@
callingFeatureId, message);
boolean hasPhoneNumberAccess = hasPhoneNumberAccess(subId, callingPackage, callingFeatureId,
message);
- if (!hasIdentifierAccess || !hasPhoneNumberAccess) {
- result = new SubscriptionInfo(subInfo);
- if (!hasIdentifierAccess) {
- result.clearIccId();
- result.clearCardString();
- }
- if (!hasPhoneNumberAccess) {
- result.clearNumber();
- }
+ return conditionallyRemoveIdentifiers(subInfo, hasIdentifierAccess, hasPhoneNumberAccess);
+ }
+
+ /**
+ * Conditionally removes identifiers from the provided {@code subInfo} based on if the calling
+ * package {@code hasIdentifierAccess} and {@code hasPhoneNumberAccess} and returns the
+ * potentially modified object.
+ *
+ * <p>If the caller specifies the package does not have identifier or phone number access
+ * a clone of the provided SubscriptionInfo is created and modified to avoid altering
+ * SubscriptionInfo objects in a cache.
+ */
+ private SubscriptionInfo conditionallyRemoveIdentifiers(SubscriptionInfo subInfo,
+ boolean hasIdentifierAccess, boolean hasPhoneNumberAccess) {
+ if (hasIdentifierAccess && hasPhoneNumberAccess) {
+ return subInfo;
+ }
+ SubscriptionInfo result = new SubscriptionInfo(subInfo);
+ if (!hasIdentifierAccess) {
+ result.clearIccId();
+ result.clearCardString();
+ }
+ if (!hasPhoneNumberAccess) {
+ result.clearNumber();
}
return result;
}
diff --git a/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java b/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java
index cf65e45..97cd02a 100644
--- a/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java
+++ b/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java
@@ -1272,6 +1272,54 @@
}
@Test
+ public void testGetActiveSubscriptionInfoListWithCarrierPrivilegesOnOneSubId()
+ throws Exception {
+ // If an app does not have the READ_PHONE_STATE permission but has carrier privileges on one
+ // out of multiple sub IDs then the SubscriptionInfo for that subId should be returned with
+ // the ICC ID and phone number.
+ testInsertSim();
+ doReturn(2).when(mTelephonyManager).getPhoneCount();
+ mSubscriptionControllerUT.addSubInfoRecord("test2", 1);
+ int firstSubId = getFirstSubId();
+ int secondSubId = getSubIdAtIndex(1);
+ mSubscriptionControllerUT.setDisplayNumber(DISPLAY_NUMBER, secondSubId);
+ setupIdentifierCarrierPrivilegesTest();
+ mContextFixture.removeCallingOrSelfPermission(Manifest.permission.READ_PHONE_STATE);
+ setCarrierPrivilegesForSubId(false, firstSubId);
+ setCarrierPrivilegesForSubId(true, secondSubId);
+
+ List<SubscriptionInfo> subInfoList =
+ mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage,
+ mCallingFeature);
+
+ assertEquals(1, subInfoList.size());
+ SubscriptionInfo subInfo = subInfoList.get(0);
+ assertEquals("test2", subInfo.getIccId());
+ assertEquals(DISPLAY_NUMBER, subInfo.getNumber());
+ }
+
+ @Test
+ public void testGetActiveSubscriptionInfoListWithIdentifierAccessWithoutNumberAccess()
+ throws Exception {
+ // An app with access to device identifiers may not have access to the device phone number
+ // (ie an app that passes the device / profile owner check or an app that has been granted
+ // the device identifiers appop); this test verifies that an app with identifier access
+ // can read the ICC ID but does not receive the phone number.
+ testInsertSim();
+ setupReadPhoneNumbersTest();
+ setIdentifierAccess(true);
+
+ List<SubscriptionInfo> subInfoList =
+ mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage,
+ mCallingFeature);
+
+ assertEquals(1, subInfoList.size());
+ SubscriptionInfo subInfo = subInfoList.get(0);
+ assertEquals("test", subInfo.getIccId());
+ assertEquals(UNAVAILABLE_NUMBER, subInfo.getNumber());
+ }
+
+ @Test
public void testGetActiveSubscriptionInfoListWithPrivilegedPermission() throws Exception {
// If the calling package has the READ_PRIVILEGED_PHONE_STATE permission or carrier
// privileges the ICC ID should be available in the SubscriptionInfo objects in the List.
@@ -1401,9 +1449,13 @@
}
private int getFirstSubId() throws Exception {
- int[] subIds = mSubscriptionControllerUT.getActiveSubIdList(/*visibleOnly*/false);
- assertTrue(subIds != null && subIds.length != 0);
- return subIds[0];
+ return getSubIdAtIndex(0);
+ }
+
+ private int getSubIdAtIndex(int index) throws Exception {
+ int[] subIds = mSubscriptionControllerUT.getActiveSubIdList(/*visibileOnly*/false);
+ assertTrue(subIds != null && subIds.length > index);
+ return subIds[index];
}
@Test
diff --git a/tests/telephonytests/src/com/android/internal/telephony/TelephonyTest.java b/tests/telephonytests/src/com/android/internal/telephony/TelephonyTest.java
index 7c0bdd9..4952b39 100644
--- a/tests/telephonytests/src/com/android/internal/telephony/TelephonyTest.java
+++ b/tests/telephonytests/src/com/android/internal/telephony/TelephonyTest.java
@@ -102,6 +102,7 @@
import com.android.server.pm.permission.PermissionManagerService;
import org.mockito.Mock;
+import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
@@ -833,6 +834,14 @@
mTelephonyManager).getCarrierPrivilegeStatus(anyInt());
}
+ protected void setCarrierPrivilegesForSubId(boolean hasCarrierPrivileges, int subId) {
+ TelephonyManager mockTelephonyManager = Mockito.mock(TelephonyManager.class);
+ doReturn(mockTelephonyManager).when(mTelephonyManager).createForSubscriptionId(subId);
+ doReturn(hasCarrierPrivileges ? TelephonyManager.CARRIER_PRIVILEGE_STATUS_HAS_ACCESS
+ : TelephonyManager.CARRIER_PRIVILEGE_STATUS_NO_ACCESS).when(
+ mockTelephonyManager).getCarrierPrivilegeStatus(anyInt());
+ }
+
protected final void waitForHandlerAction(Handler h, long timeoutMillis) {
final CountDownLatch lock = new CountDownLatch(1);
h.post(lock::countDown);
