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

Commit a57ec5d8 authored by Kyunglyul Hyun's avatar Kyunglyul Hyun
Browse files

Limit gatt clients per app 

This change limits the number of GATT clients per app to 32.
This prevents a badly implemented app
from exhausting the global GATT client limit.

The commit also includes a fix for a case where
tBTA_GATTC_CLCB could be accessed after it was deallocated.

This issue was discovered while updating a test in GattClientTest.

Bug: 348559823
Bug: 366473955
Test: atest GattServiceTest GattClientTest

Change-Id: I9c600a867d0c2aee4f92d31fdbf3f8a0775a83c1
parent bdae16cc

android/app/src/com/android/bluetooth/gatt/ContextMap.java

+15 −4
Original line number Diff line number Diff line
@@ -64,13 +64,16 @@ public class ContextMap<C> { 
    /** Application entry mapping UUIDs to appIDs and callbacks. */

    public class App {

        /** The UUID of the application */

        public UUID uuid;

        public final UUID uuid;



        /** The id of the application */

        public int id;



        /** The uid of the application */

        public final int appUid;



        /** The package name of the application */

        public String name;

        public final String name;



        /** Application callbacks */

        public C callback;
@@ -85,9 +88,10 @@ public class ContextMap<C> { 
        private List<CallbackInfo> mCongestionQueue = new ArrayList<>();



        /** Creates a new app context. */

        App(UUID uuid, C callback, String name) {

        App(UUID uuid, C callback, int appUid, String name) {

            this.uuid = uuid;

            this.callback = callback;

            this.appUid = appUid;

            this.name = name;

        }
@@ -150,7 +154,7 @@ public class ContextMap<C> { 
            appName = "Unknown App (UID: " + appUid + ")";

        }

        synchronized (mAppsLock) {

            App app = new App(uuid, callback, appName);

            App app = new App(uuid, callback, appUid, appName);

            mApps.add(app);

            return app;

        }
@@ -334,6 +338,13 @@ public class ContextMap<C> { 
        return currentConnections;

    }



    /** Counts the number of applications that have a given app UID. */

    public int countByAppUid(int appUid) {

        synchronized (mAppsLock) {

            return (int) (mApps.stream().filter(app -> app.appUid == appUid).count());

        }

    }



    /** Erases all application context entries. */

    public void clear() {

        synchronized (mAppsLock) {

android/app/src/com/android/bluetooth/gatt/GattService.java

+13 −0
Original line number Diff line number Diff line
@@ -60,6 +60,7 @@ import android.content.pm.PackageManager; 
import android.content.pm.PackageManager.NameNotFoundException;

import android.content.pm.PackageManager.PackageInfoFlags;

import android.content.res.Resources;

import android.os.Binder;

import android.os.Build;

import android.os.Handler;

import android.os.HandlerThread;
@@ -142,6 +143,8 @@ public class GattService extends ProfileService { 
                "0201061AFF4C000215426C7565436861726D426561636F6E730EFE1355C509168020691E0EFE13551109426C7565436861726D5F31363936383500000000",

            };



    @VisibleForTesting static final int GATT_CLIENT_LIMIT_PER_APP = 32;



    public final TransitionalScanHelper mTransitionalScanHelper =

            new TransitionalScanHelper(this, this::isTestModeEnabled);
@@ -2083,6 +2086,16 @@ public class GattService extends ProfileService { 
                this, attributionSource, "GattService registerClient")) {

            return;

        }

        if (Flags.gattClientDynamicAllocation()

                && mClientMap.countByAppUid(Binder.getCallingUid()) >= GATT_CLIENT_LIMIT_PER_APP) {

            Log.w(TAG, "registerClient() - failed due to too many clients");

            try {

                callback.onClientRegistered(BluetoothGatt.GATT_FAILURE, 0);

            } catch (RemoteException e) {

                // do nothing

            }

            return;

        }



        Log.d(TAG, "registerClient() - UUID=" + uuid);

        mClientMap.add(uuid, callback, this);

android/app/tests/unit/src/com/android/bluetooth/gatt/GattServiceTest.java

+12 −0
Original line number Diff line number Diff line
@@ -366,6 +366,18 @@ public class GattServiceTest { 
                        uuid.getLeastSignificantBits(), uuid.getMostSignificantBits(), eattSupport);

    }



    @Test

    public void registerClient_checkLimitPerApp() {

        mSetFlagsRule.enableFlags(Flags.FLAG_GATT_CLIENT_DYNAMIC_ALLOCATION);

        doReturn(GattService.GATT_CLIENT_LIMIT_PER_APP).when(mClientMap).countByAppUid(anyInt());

        UUID uuid = UUID.randomUUID();

        IBluetoothGattCallback callback = mock(IBluetoothGattCallback.class);



        mService.registerClient(uuid, callback, /* eattSupport= */ true, mAttributionSource);

        verify(mClientMap, never()).add(any(), any(), any());

        verify(mNativeInterface, never()).gattClientRegisterApp(anyLong(), anyLong(), anyBoolean());

    }



    @Test

    public void unregisterClient() {

        int clientIf = 3;

framework/tests/bumble/src/android/bluetooth/GattClientTest.java

+36 −1
Original line number Diff line number Diff line
@@ -54,6 +54,7 @@ import org.junit.Rule; 
import org.junit.Test;

import org.junit.runner.RunWith;

import org.mockito.AdditionalMatchers;

import org.mockito.ArgumentCaptor;

import org.mockito.InOrder;

import org.mockito.invocation.Invocation;
@@ -480,6 +481,28 @@ public class GattClientTest { 
        return gatt;

    }



    /** Tries to connect GATT, it could fail and return null. */

    private BluetoothGatt tryConnectGatt(BluetoothGattCallback callback) {

        advertiseWithBumble();



        BluetoothDevice device =

                mAdapter.getRemoteLeDevice(

                        Utils.BUMBLE_RANDOM_ADDRESS, BluetoothDevice.ADDRESS_TYPE_RANDOM);



        BluetoothGatt gatt = device.connectGatt(mContext, false, callback);



        ArgumentCaptor<Integer> statusCaptor = ArgumentCaptor.forClass(Integer.class);

        ArgumentCaptor<Integer> stateCaptor = ArgumentCaptor.forClass(Integer.class);

        verify(callback, timeout(1000))

                .onConnectionStateChange(eq(gatt), statusCaptor.capture(), stateCaptor.capture());



        if (statusCaptor.getValue() == GATT_SUCCESS && stateCaptor.getValue() == STATE_CONNECTED) {

            return gatt;

        }

        gatt.close();

        return null;

    }



    private void disconnectAndWaitDisconnection(

            BluetoothGatt gatt, BluetoothGattCallback callback) {

        final int state = BluetoothProfile.STATE_DISCONNECTED;
@@ -578,12 +601,22 @@ public class GattClientTest { 
        registerGattService();



        List<BluetoothGatt> gatts = new ArrayList<>();

        boolean failed = false;

        final int repeatTimes = 100;



        try {

            for (int i = 0; i < repeatTimes; i++) {

                BluetoothGattCallback gattCallback = mock(BluetoothGattCallback.class);

                BluetoothGatt gatt = connectGattAndWaitConnection(gattCallback);

                BluetoothGatt gatt = tryConnectGatt(gattCallback);

                // If it fails, close an existing gatt instance and try again.

                if (gatt == null) {

                    failed = true;

                    BluetoothGatt connectedGatt = gatts.remove(0);

                    connectedGatt.disconnect();

                    connectedGatt.close();

                    gattCallback = mock(BluetoothGattCallback.class);

                    gatt = connectGattAndWaitConnection(gattCallback);

                }

                gatts.add(gatt);

                gatt.discoverServices();

                verify(gattCallback, timeout(10000)).onServicesDiscovered(any(), eq(GATT_SUCCESS));
@@ -601,5 +634,7 @@ public class GattClientTest { 
                gatt.close();

            }

        }

        // We should fail because we reached the limit.

        assertThat(failed).isTrue();

    }

}

system/bta/gatt/bta_gattc_act.cc

+2 −0
Original line number Diff line number Diff line
@@ -311,6 +311,8 @@ void bta_gattc_deregister(tBTA_GATTC_RCB* p_clreg) { 
      };

      bta_gattc_close(p_clcb.get(), &gattc_data);

    }

    // deallocated clcbs will not be accessed. Let them be claened up.

    bta_gattc_cleanup_clcb();

  } else {

    for (size_t i = 0; i < BTA_GATTC_CLCB_MAX; i++) {

      if (!bta_gattc_cb.clcb[i].in_use || (bta_gattc_cb.clcb[i].p_rcb != p_clreg)) {