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
Bug: 173421434
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
Merged-In: I4d738955125ea4d39252fa582c41139fc5f6f784
diff --git a/src/java/com/android/internal/telephony/SubscriptionController.java b/src/java/com/android/internal/telephony/SubscriptionController.java
index a80df72..40ebf1b 100644
--- a/src/java/com/android/internal/telephony/SubscriptionController.java
+++ b/src/java/com/android/internal/telephony/SubscriptionController.java
@@ -3278,7 +3278,7 @@
callingPackage, "getSubscriptionsInGroup")
|| (info.isEmbedded() && info.canManageSubscription(mContext, callingPackage));
}).map(subscriptionInfo -> conditionallyRemoveIdentifiers(subscriptionInfo,
- callingPackage, "getSubscriptionInfoList"))
+ callingPackage, "getSubscriptionsInGroup"))
.collect(Collectors.toList());
}
@@ -3546,42 +3546,56 @@
// They are doing similar things except operating on different cache.
private List<SubscriptionInfo> getSubscriptionInfoListFromCacheHelper(
String callingPackage, List<SubscriptionInfo> cacheSubList) {
- boolean canReadAllPhoneState;
+ boolean canReadPhoneState = false;
+ boolean canReadIdentifiers = false;
try {
- canReadAllPhoneState = TelephonyPermissions.checkReadPhoneState(mContext,
+ canReadPhoneState = TelephonyPermissions.checkReadPhoneState(mContext,
SubscriptionManager.INVALID_SUBSCRIPTION_ID, Binder.getCallingPid(),
Binder.getCallingUid(), callingPackage, "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 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,
"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) {
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,
- "getSubscriptionInfoList");
- } catch (SecurityException e) {
- return false;
- }
- }).map(subscriptionInfo -> conditionallyRemoveIdentifiers(subscriptionInfo,
- callingPackage, "getSubscriptionInfoList"))
- .collect(Collectors.toList());
+ List<SubscriptionInfo> subscriptions = new ArrayList<>(cacheSubList.size());
+ for (SubscriptionInfo subscriptionInfo : cacheSubList) {
+ int subId = subscriptionInfo.getSubscriptionId();
+ boolean hasCarrierPrivileges = TelephonyPermissions.checkCarrierPrivilegeForSubId(
+ 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));
+ }
+ }
+ return subscriptions;
}
}
@@ -3597,8 +3611,26 @@
private SubscriptionInfo conditionallyRemoveIdentifiers(SubscriptionInfo subInfo,
String callingPackage, String message) {
SubscriptionInfo result = subInfo;
- if (!hasSubscriberIdentifierAccess(subInfo.getSubscriptionId(), callingPackage, message)) {
- result = new SubscriptionInfo(subInfo);
+ int subId = subInfo.getSubscriptionId();
+ boolean hasIdentifierAccess = hasSubscriberIdentifierAccess(subId, callingPackage, message);
+ return conditionallyRemoveIdentifiers(subInfo, hasIdentifierAccess);
+ }
+
+ /**
+ * Conditionally removes identifiers from the provided {@code subInfo} based on if the calling
+ * package {@code hasIdentifierAccess} and returns the potentially modified object.
+ *
+ * <p>If the caller specifies the package does not have identifier 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) {
+ if (hasIdentifierAccess) {
+ return subInfo;
+ }
+ SubscriptionInfo result = new SubscriptionInfo(subInfo);
+ if (!hasIdentifierAccess) {
result.clearIccId();
result.clearCardString();
}
diff --git a/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java b/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java
index 2e19e00..9b3de18 100644
--- a/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java
+++ b/tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java
@@ -948,8 +948,7 @@
int subId = getFirstSubId();
try {
- mSubscriptionControllerUT.getActiveSubscriptionInfo(subId, mCallingPackage,
- mCallingFeature);
+ mSubscriptionControllerUT.getActiveSubscriptionInfo(subId, mCallingPackage);
fail("getActiveSubscriptionInfo should fail when invoked with no permissions");
} catch (SecurityException expected) {
}
@@ -967,7 +966,7 @@
int subId = getFirstSubId();
SubscriptionInfo subscriptionInfo = mSubscriptionControllerUT.getActiveSubscriptionInfo(
- subId, mCallingPackage, mCallingFeature);
+ subId, mCallingPackage);
assertNotNull(subscriptionInfo);
assertEquals(UNAVAILABLE_ICCID, subscriptionInfo.getIccId());
assertEquals(UNAVAILABLE_ICCID, subscriptionInfo.getCardString());
@@ -981,7 +980,7 @@
int subId = getFirstSubId();
SubscriptionInfo subscriptionInfo = mSubscriptionControllerUT.getActiveSubscriptionInfo(
- subId, mCallingPackage, mCallingFeature);
+ subId, mCallingPackage);
assertNotNull(subscriptionInfo);
assertTrue(subscriptionInfo.getIccId().length() > 0);
assertTrue(subscriptionInfo.getCardString().length() > 0);
@@ -996,8 +995,7 @@
mContextFixture.removeCallingOrSelfPermission(ContextFixture.PERMISSION_ENABLE_ALL);
try {
- mSubscriptionControllerUT.getActiveSubscriptionInfoForSimSlotIndex(0, mCallingPackage,
- mCallingFeature);
+ mSubscriptionControllerUT.getActiveSubscriptionInfoForSimSlotIndex(0, mCallingPackage);
fail("getActiveSubscriptionInfoForSimSlotIndex should fail when invoked with no "
+ "permissions");
} catch (SecurityException expected) {
@@ -1016,7 +1014,7 @@
SubscriptionInfo subscriptionInfo =
mSubscriptionControllerUT.getActiveSubscriptionInfoForSimSlotIndex(0,
- mCallingPackage, mCallingFeature);
+ mCallingPackage);
assertNotNull(subscriptionInfo);
assertEquals(UNAVAILABLE_ICCID, subscriptionInfo.getIccId());
assertEquals(UNAVAILABLE_ICCID, subscriptionInfo.getCardString());
@@ -1031,7 +1029,7 @@
SubscriptionInfo subscriptionInfo =
mSubscriptionControllerUT.getActiveSubscriptionInfoForSimSlotIndex(0,
- mCallingPackage, mCallingFeature);
+ mCallingPackage);
assertNotNull(subscriptionInfo);
assertTrue(subscriptionInfo.getIccId().length() > 0);
assertTrue(subscriptionInfo.getCardString().length() > 0);
@@ -1045,8 +1043,7 @@
mContextFixture.removeCallingOrSelfPermission(ContextFixture.PERMISSION_ENABLE_ALL);
List<SubscriptionInfo> subInfoList =
- mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage,
- mCallingFeature);
+ mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage);
assertNotNull(subInfoList);
assertTrue(subInfoList.size() == 0);
}
@@ -1062,8 +1059,7 @@
setupMocksForTelephonyPermissions();
List<SubscriptionInfo> subInfoList =
- mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage,
- mCallingFeature);
+ mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage);
assertTrue(subInfoList.size() > 0);
for (SubscriptionInfo info : subInfoList) {
assertEquals(UNAVAILABLE_ICCID, info.getIccId());
@@ -1072,14 +1068,30 @@
}
@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();
+
+ List<SubscriptionInfo> subInfoList =
+ mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage);
+
+ assertEquals(1, subInfoList.size());
+ SubscriptionInfo subInfo = subInfoList.get(0);
+ assertEquals("test", subInfo.getIccId());
+ }
+
+ @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.
testInsertSim();
List<SubscriptionInfo> subInfoList =
- mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage,
- mCallingFeature);
+ mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage);
assertTrue(subInfoList.size() > 0);
for (SubscriptionInfo info : subInfoList) {
assertTrue(info.getIccId().length() > 0);
@@ -1096,8 +1108,7 @@
mContextFixture.removeCallingOrSelfPermission(ContextFixture.PERMISSION_ENABLE_ALL);
try {
- mSubscriptionControllerUT.getSubscriptionsInGroup(groupUuid, mCallingPackage,
- mCallingFeature);
+ mSubscriptionControllerUT.getSubscriptionsInGroup(groupUuid, mCallingPackage);
fail("getSubscriptionsInGroup should fail when invoked with no permissions");
} catch (SecurityException expected) {
}
@@ -1114,7 +1125,7 @@
setupMocksForTelephonyPermissions();
List<SubscriptionInfo> subInfoList = mSubscriptionControllerUT.getSubscriptionsInGroup(
- groupUuid, mCallingPackage, mCallingFeature);
+ groupUuid, mCallingPackage);
assertTrue(subInfoList.size() > 0);
for (SubscriptionInfo info : subInfoList) {
assertEquals(UNAVAILABLE_ICCID, info.getIccId());
@@ -1129,7 +1140,7 @@
ParcelUuid groupUuid = setupGetSubscriptionsInGroupTest();
List<SubscriptionInfo> subInfoList = mSubscriptionControllerUT.getSubscriptionsInGroup(
- groupUuid, mCallingPackage, mCallingFeature);
+ groupUuid, mCallingPackage);
assertTrue(subInfoList.size() > 0);
for (SubscriptionInfo info : subInfoList) {
assertTrue(info.getIccId().length() > 0);
@@ -1147,9 +1158,13 @@
}
private int getFirstSubId() throws Exception {
+ return getSubIdAtIndex(0);
+ }
+
+ private int getSubIdAtIndex(int index) throws Exception {
int[] subIds = mSubscriptionControllerUT.getActiveSubIdList(/*visibleOnly*/false);
- assertTrue(subIds != null && subIds.length != 0);
- return subIds[0];
+ 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 30653c2..b18ded6 100644
--- a/tests/telephonytests/src/com/android/internal/telephony/TelephonyTest.java
+++ b/tests/telephonytests/src/com/android/internal/telephony/TelephonyTest.java
@@ -680,8 +680,7 @@
// identifier access via an appop; ensure this query does not allow identifier access for
// any packages.
doReturn(AppOpsManager.MODE_DEFAULT).when(mAppOpsManager).noteOpNoThrow(
- eq(AppOpsManager.OPSTR_READ_DEVICE_IDENTIFIERS), anyInt(), anyString(),
- nullable(String.class), nullable(String.class));
+ eq(AppOpsManager.OPSTR_READ_DEVICE_IDENTIFIERS), anyInt(), anyString());
// TelephonyPermissions queries DeviceConfig to determine if the identifier access
// restrictions should be enabled; this results in a NPE when DeviceConfig uses
