Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 640387d3 authored by Michael Groover's avatar Michael Groover
Browse files

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
parent 7dfee2e7

src/java/com/android/internal/telephony/SubscriptionController.java

+56 −24
Original line number Diff line number Diff line
@@ -3278,7 +3278,7 @@ public class SubscriptionController extends ISub.Stub { 
                    callingPackage, "getSubscriptionsInGroup")

                    || (info.isEmbedded() && info.canManageSubscription(mContext, callingPackage));

        }).map(subscriptionInfo -> conditionallyRemoveIdentifiers(subscriptionInfo,

                callingPackage, "getSubscriptionInfoList"))

                callingPackage, "getSubscriptionsInGroup"))

        .collect(Collectors.toList());

    }
@@ -3546,42 +3546,56 @@ public class SubscriptionController extends ISub.Stub { 
    // 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;

            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));

                }

                    }).map(subscriptionInfo -> conditionallyRemoveIdentifiers(subscriptionInfo,

                            callingPackage, "getSubscriptionInfoList"))

                    .collect(Collectors.toList());

            }

            return subscriptions;

        }

    }
@@ -3597,8 +3611,26 @@ public class SubscriptionController extends ISub.Stub { 
    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();

        }

tests/telephonytests/src/com/android/internal/telephony/SubscriptionControllerTest.java

+35 −20
Original line number Diff line number Diff line
@@ -948,8 +948,7 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 


        SubscriptionInfo subscriptionInfo =

                mSubscriptionControllerUT.getActiveSubscriptionInfoForSimSlotIndex(0,

                        mCallingPackage, mCallingFeature);

                        mCallingPackage);

        assertNotNull(subscriptionInfo);

        assertEquals(UNAVAILABLE_ICCID, subscriptionInfo.getIccId());

        assertEquals(UNAVAILABLE_ICCID, subscriptionInfo.getCardString());
@@ -1031,7 +1029,7 @@ public class SubscriptionControllerTest extends TelephonyTest { 


        SubscriptionInfo subscriptionInfo =

                mSubscriptionControllerUT.getActiveSubscriptionInfoForSimSlotIndex(0,

                        mCallingPackage, mCallingFeature);

                        mCallingPackage);

        assertNotNull(subscriptionInfo);

        assertTrue(subscriptionInfo.getIccId().length() > 0);

        assertTrue(subscriptionInfo.getCardString().length() > 0);
@@ -1045,8 +1043,7 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        setupMocksForTelephonyPermissions();



        List<SubscriptionInfo> subInfoList =

                mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage,

                        mCallingFeature);

                mSubscriptionControllerUT.getActiveSubscriptionInfoList(mCallingPackage);

        assertTrue(subInfoList.size() > 0);

        for (SubscriptionInfo info : subInfoList) {

            assertEquals(UNAVAILABLE_ICCID, info.getIccId());
@@ -1071,6 +1067,23 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        }

    }



    @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
@@ -1078,8 +1091,7 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
        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 @@ public class SubscriptionControllerTest extends TelephonyTest { 
    }



    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

tests/telephonytests/src/com/android/internal/telephony/TelephonyTest.java

+1 −2
Original line number Diff line number Diff line
@@ -680,8 +680,7 @@ public abstract class TelephonyTest { 
        // 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