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

Commit 80a7cc8f authored by Sal Savage's avatar Sal Savage
Browse files

Better protect staticly accessed resources to prevent exceptions 

A2DP and AVRCP talk to each other a static function so AVRCP can know if
A2DP has proper audio focus before a play request is sent. This function
is only valid once A2DP has been started, yielding to an object thats
created on start(). The function makes no checks for the static service
reference OR the stream handler before use. This can lead to NPEs in
rare cases.

This change turn grabbing focus from an unsafe state function to a
member function, aligning it with other functions AVRCP uses. It also
makes it mockable in tests better. Functions now check for null values
before using them and have sensible defaults in the case something is
wrong. Start and stop are now synchronized so we can make assumptions
about the state of the stream handler.

Additionally, AVRCP controller has been changed to match the new method
of consuming getFocusState() and the surrounding logic has been updated.
Tests have been added and cleaned up to cover the entire block of code.

Bug: 152632246
Test: Build, flash, test with A2DP source devices, atest
Change-Id: Ic58dcc2bf1985c173465e245787d5567250a5577
parent 7f562329

android/app/src/com/android/bluetooth/a2dpsink/A2dpSinkService.java

+70 −47
Original line number Diff line number Diff line
@@ -20,6 +20,7 @@ import android.bluetooth.BluetoothAudioConfig; 
import android.bluetooth.BluetoothDevice;

import android.bluetooth.BluetoothProfile;

import android.bluetooth.IBluetoothA2dpSink;

import android.media.AudioManager;

import android.util.Log;



import com.android.bluetooth.Utils;
@@ -47,6 +48,8 @@ public class A2dpSinkService extends ProfileService { 
    protected Map<BluetoothDevice, A2dpSinkStateMachine> mDeviceStateMap =

            new ConcurrentHashMap<>(1);



    private final Object mStreamHandlerLock = new Object();



    private A2dpSinkStreamHandler mA2dpSinkStreamHandler;

    private static A2dpSinkService sService;
@@ -56,22 +59,32 @@ public class A2dpSinkService extends ProfileService { 


    @Override

    protected boolean start() {

        initNative();

        sService = this;

        synchronized (mStreamHandlerLock) {

            mA2dpSinkStreamHandler = new A2dpSinkStreamHandler(this, this);

        }

        initNative();

        setA2dpSinkService(this);

        return true;

    }



    @Override

    protected boolean stop() {

        setA2dpSinkService(null);

        cleanupNative();

        for (A2dpSinkStateMachine stateMachine : mDeviceStateMap.values()) {

            stateMachine.quitNow();

        }

        sService = null;

        mDeviceStateMap.clear();

        synchronized (mStreamHandlerLock) {

            if (mA2dpSinkStreamHandler != null) {

                mA2dpSinkStreamHandler.cleanup();

                mA2dpSinkStreamHandler = null;

            }

        }

        return true;

    }



    public static A2dpSinkService getA2dpSinkService() {

    public static synchronized A2dpSinkService getA2dpSinkService() {

        return sService;

    }
@@ -80,9 +93,8 @@ public class A2dpSinkService extends ProfileService { 
     * @hide

     */

    @VisibleForTesting

    public static void setA2dpSinkService(A2dpSinkService service) {

    public static synchronized void setA2dpSinkService(A2dpSinkService service) {

        sService = service;

        sService.mA2dpSinkStreamHandler = new A2dpSinkStreamHandler(sService, sService);

    }
@@ -90,20 +102,36 @@ public class A2dpSinkService extends ProfileService { 
        mAdapter = BluetoothAdapter.getDefaultAdapter();

    }



    protected A2dpSinkStateMachine newStateMachine(BluetoothDevice device) {

        return new A2dpSinkStateMachine(device, this);

    }



    protected synchronized A2dpSinkStateMachine getStateMachine(BluetoothDevice device) {

        return mDeviceStateMap.get(device);

    }



    /**

     * Request audio focus such that the designated device can stream audio

     */

    public void requestAudioFocus(BluetoothDevice device, boolean request) {

        synchronized (mStreamHandlerLock) {

            if (mA2dpSinkStreamHandler == null) return;

            mA2dpSinkStreamHandler.requestAudioFocus(request);

        }

    }



    /**

     * Get the current Bluetooth Audio focus state

     *

     * @return AudioManger.AUDIOFOCUS_* states on success, or AudioManager.ERROR on error

     */

    public int getFocusState() {

        synchronized (mStreamHandlerLock) {

            if (mA2dpSinkStreamHandler == null) return AudioManager.ERROR;

            return mA2dpSinkStreamHandler.getFocusState();

        }

    }



    boolean isA2dpPlaying(BluetoothDevice device) {

        enforceCallingOrSelfPermission(

                BLUETOOTH_PRIVILEGED, "Need BLUETOOTH_PRIVILEGED permission");

        synchronized (mStreamHandlerLock) {

            if (mA2dpSinkStreamHandler == null) return false;

            return mA2dpSinkStreamHandler.isPlaying();

        }

    }



    @Override

    protected IProfileServiceBinder initBinder() {
@@ -225,7 +253,7 @@ public class A2dpSinkService extends ProfileService { 
     *

     * @return true if connection is successful, false otherwise.

     */

    public synchronized boolean connect(BluetoothDevice device) {

    public boolean connect(BluetoothDevice device) {

        enforceCallingOrSelfPermission(BLUETOOTH_PRIVILEGED,

                "Need BLUETOOTH_PRIVILEGED permission");

        if (device == null) {
@@ -242,13 +270,14 @@ public class A2dpSinkService extends ProfileService { 
                    + "> is CONNECTION_POLICY_FORBIDDEN");

            return false;

        }



        A2dpSinkStateMachine stateMachine = getOrCreateStateMachine(device);

        if (stateMachine != null) {

            stateMachine.connect();

            return true;

        } else {

            // a state machine instance doesn't exist yet, and the max has been reached.

            Log.e(TAG, "Maxed out on the number of allowed MAP connections. "

            Log.e(TAG, "Maxed out on the number of allowed A2DP Sink connections. "

                    + "Connect request rejected on " + device);

            return false;

        }
@@ -259,7 +288,7 @@ public class A2dpSinkService extends ProfileService { 
     *

     * @return true if disconnect is successful, false otherwise.

     */

    public synchronized boolean disconnect(BluetoothDevice device) {

    public boolean disconnect(BluetoothDevice device) {

        enforceCallingOrSelfPermission(BLUETOOTH_ADMIN_PERM, "Need BLUETOOTH ADMIN permission");

        if (DBG) {

            StringBuilder sb = new StringBuilder();
@@ -267,6 +296,7 @@ public class A2dpSinkService extends ProfileService { 
            Log.d(TAG, "A2DP disconnect device: " + device

                    + ", InstanceMap start state: " + sb.toString());

        }



        A2dpSinkStateMachine stateMachine = mDeviceStateMap.get(device);

        // a state machine instance doesn't exist. maybe it is already gone?

        if (stateMachine == null) {
@@ -292,13 +322,16 @@ public class A2dpSinkService extends ProfileService { 
    }



    protected A2dpSinkStateMachine getOrCreateStateMachine(BluetoothDevice device) {

        A2dpSinkStateMachine stateMachine = mDeviceStateMap.get(device);

        if (stateMachine == null) {

            stateMachine = newStateMachine(device);

            mDeviceStateMap.put(device, stateMachine);

            stateMachine.start();

        A2dpSinkStateMachine newStateMachine = new A2dpSinkStateMachine(device, this);

        A2dpSinkStateMachine existingStateMachine =

                mDeviceStateMap.putIfAbsent(device, newStateMachine);

        // Given null is not a valid value in our map, ConcurrentHashMap will return null if the

        // key was absent and our new value was added. We should then start and return it.

        if (existingStateMachine == null) {

            newStateMachine.start();

            return newStateMachine;

        }

        return stateMachine;

        return existingStateMachine;

    }



    List<BluetoothDevice> getDevicesMatchingConnectionStates(int[] states) {
@@ -329,7 +362,7 @@ public class A2dpSinkService extends ProfileService { 
     * {@link BluetoothProfile#STATE_CONNECTED} if this profile is connected, or

     * {@link BluetoothProfile#STATE_DISCONNECTING} if this profile is being disconnected

     */

    public synchronized int getConnectionState(BluetoothDevice device) {

    public int getConnectionState(BluetoothDevice device) {

        A2dpSinkStateMachine stateMachine = mDeviceStateMap.get(device);

        return (stateMachine == null) ? BluetoothProfile.STATE_DISCONNECTED

                : stateMachine.getState();
@@ -391,21 +424,6 @@ public class A2dpSinkService extends ProfileService { 
        }

    }



    /**

     * Get the current Bluetooth Audio focus state

     *

     * @return focus

     */

    public static int getFocusState() {

        return sService.mA2dpSinkStreamHandler.getFocusState();

    }



    boolean isA2dpPlaying(BluetoothDevice device) {

        enforceCallingOrSelfPermission(

                BLUETOOTH_PRIVILEGED, "Need BLUETOOTH_PRIVILEGED permission");

        return mA2dpSinkStreamHandler.isPlaying();

    }



    BluetoothAudioConfig getAudioConfig(BluetoothDevice device) {

        A2dpSinkStateMachine stateMachine = mDeviceStateMap.get(device);

        // a state machine instance doesn't exist. maybe it is already gone?
@@ -461,7 +479,11 @@ public class A2dpSinkService extends ProfileService { 
    }



    private void onAudioStateChanged(byte[] address, int state) {

        if (state == StackEvent.AUDIO_STATE_STARTED) {

        synchronized (mStreamHandlerLock) {

            if (mA2dpSinkStreamHandler == null) {

                Log.e(TAG, "Received audio state change before we've been started");

                return;

            } else if (state == StackEvent.AUDIO_STATE_STARTED) {

                mA2dpSinkStreamHandler.obtainMessage(

                        A2dpSinkStreamHandler.SRC_STR_START).sendToTarget();

            } else if (state == StackEvent.AUDIO_STATE_STOPPED
@@ -470,6 +492,7 @@ public class A2dpSinkService extends ProfileService { 
                        A2dpSinkStreamHandler.SRC_STR_STOP).sendToTarget();

            }

        }

    }



    private void onAudioConfigChanged(byte[] address, int sampleRate, int channelCount) {

        StackEvent event = StackEvent.audioConfigChanged(getDevice(address), sampleRate,

android/app/src/com/android/bluetooth/a2dpsink/A2dpSinkStreamHandler.java

+8 −0
Original line number Diff line number Diff line
@@ -117,6 +117,14 @@ public class A2dpSinkStreamHandler extends Handler { 
        mAudioManager = (AudioManager) context.getSystemService(Context.AUDIO_SERVICE);

    }



    /**

     * Safely clean up this stream handler object

     */

    public void cleanup() {

        abandonAudioFocus();

        removeCallbacksAndMessages(null);

    }



    void requestAudioFocus(boolean request) {

        obtainMessage(REQUEST_FOCUS, request).sendToTarget();

    }

android/app/src/com/android/bluetooth/avrcpcontroller/AvrcpControllerStateMachine.java

+23 −5
Original line number Diff line number Diff line
@@ -433,11 +433,29 @@ class AvrcpControllerStateMachine extends StateMachine { 


                case MESSAGE_PROCESS_PLAY_STATUS_CHANGED:

                    mAddressedPlayer.setPlayStatus(msg.arg1);

                    BluetoothMediaBrowserService.notifyChanged(

                            mAddressedPlayer.getPlaybackState());

                    if (mAddressedPlayer.getPlaybackState().getState()

                            == PlaybackStateCompat.STATE_PLAYING

                            && A2dpSinkService.getFocusState() == AudioManager.AUDIOFOCUS_NONE) {

                    if (!isActive()) {

                        sendMessage(MSG_AVRCP_PASSTHRU,

                                AvrcpControllerService.PASS_THRU_CMD_ID_PAUSE);

                        return true;

                    }



                    PlaybackStateCompat playbackState = mAddressedPlayer.getPlaybackState();

                    BluetoothMediaBrowserService.notifyChanged(playbackState);



                    int focusState = AudioManager.ERROR;

                    A2dpSinkService a2dpSinkService = A2dpSinkService.getA2dpSinkService();

                    if (a2dpSinkService != null) {

                        focusState = a2dpSinkService.getFocusState();

                    }



                    if (focusState == AudioManager.ERROR) {

                        sendMessage(MSG_AVRCP_PASSTHRU,

                                AvrcpControllerService.PASS_THRU_CMD_ID_PAUSE);

                        return true;

                    }



                    if (playbackState.getState() == PlaybackStateCompat.STATE_PLAYING

                            && focusState == AudioManager.AUDIOFOCUS_NONE) {

                        if (shouldRequestFocus()) {

                            mSessionCallbacks.onPrepare();

                        } else {

android/app/tests/unit/src/com/android/bluetooth/avrcpcontroller/AvrcpControllerStateMachineTest.java

+93 −9
Original line number Diff line number Diff line
@@ -127,6 +127,7 @@ public class AvrcpControllerStateMachineTest { 
        mAvrcpControllerService.start();

        mAvrcpControllerService.sBrowseTree = new BrowseTree(null);

        mAvrcpStateMachine = new AvrcpControllerStateMachine(mTestDevice, mAvrcpControllerService);

        mAvrcpStateMachine.start();

    }



    @After
@@ -134,6 +135,11 @@ public class AvrcpControllerStateMachineTest { 
        if (!mTargetContext.getResources().getBoolean(R.bool.profile_supported_avrcp_controller)) {

            return;

        }



        mAvrcpStateMachine.disconnect();

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        Assert.assertFalse(mAvrcpStateMachine.isActive());



        TestUtils.clearAdapterService(mA2dpAdapterService);

    }
@@ -251,7 +257,7 @@ public class AvrcpControllerStateMachineTest { 
        mAvrcpStateMachine.dump(sb);

        Assert.assertEquals(sb.toString(),

                "  mDevice: " + mTestDevice.toString()

                + "(null) name=AvrcpControllerStateMachine state=(null)\n"

                + "(null) name=AvrcpControllerStateMachine state=Disconnected\n"

                + "  isActive: false\n");

    }
@@ -618,7 +624,7 @@ public class AvrcpControllerStateMachineTest { 
    public void testPlaybackWhileMusicPlaying() {

        when(mMockResources.getBoolean(R.bool.a2dp_sink_automatically_request_audio_focus))

                .thenReturn(false);

        Assert.assertEquals(AudioManager.AUDIOFOCUS_NONE, A2dpSinkService.getFocusState());

        when(mA2dpSinkService.getFocusState()).thenReturn(AudioManager.AUDIOFOCUS_NONE);

        doReturn(true).when(mAudioManager).isMusicActive();

        setUpConnectedState(true, true);

        mAvrcpStateMachine.sendMessage(
@@ -628,9 +634,7 @@ public class AvrcpControllerStateMachineTest { 
        verify(mAvrcpControllerService,

                timeout(ASYNC_CALL_TIMEOUT_MILLIS).times(1)).sendPassThroughCommandNative(

                eq(mTestAddress), eq(AvrcpControllerService.PASS_THRU_CMD_ID_PAUSE), eq(KEY_DOWN));

        TestUtils.waitForLooperToFinishScheduledTask(

                A2dpSinkService.getA2dpSinkService().getMainLooper());

        Assert.assertEquals(AudioManager.AUDIOFOCUS_NONE, mA2dpSinkService.getFocusState());

        verify(mA2dpSinkService, never()).requestAudioFocus(mTestDevice, true);

    }



    /**
@@ -638,18 +642,92 @@ public class AvrcpControllerStateMachineTest { 
     */

    @Test

    public void testPlaybackWhileIdle() {

        Assert.assertEquals(AudioManager.AUDIOFOCUS_NONE, A2dpSinkService.getFocusState());

        when(mA2dpSinkService.getFocusState()).thenReturn(AudioManager.AUDIOFOCUS_NONE);

        doReturn(false).when(mAudioManager).isMusicActive();

        setUpConnectedState(true, true);

        mAvrcpStateMachine.sendMessage(

                AvrcpControllerStateMachine.MESSAGE_PROCESS_PLAY_STATUS_CHANGED,

                PlaybackStateCompat.STATE_PLAYING);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        TestUtils.waitForLooperToFinishScheduledTask(

                mA2dpSinkService.getMainLooper());

        verify(mA2dpSinkService).requestAudioFocus(mTestDevice, true);

    }



    /**

     * Test receiving a playback status of playing while we're in an error state

     * as it relates to getting audio focus.

     *

     * Verify we send a pause command and never attempt to request audio focus

     */

    @Test

    public void testPlaybackWhileErrorState() {

        when(mA2dpSinkService.getFocusState()).thenReturn(AudioManager.ERROR);

        setUpConnectedState(true, true);

        mAvrcpStateMachine.sendMessage(

                AvrcpControllerStateMachine.MESSAGE_PROCESS_PLAY_STATUS_CHANGED,

                PlaybackStateCompat.STATE_PLAYING);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        verify(mAvrcpControllerService,

                timeout(ASYNC_CALL_TIMEOUT_MILLIS).times(1)).sendPassThroughCommandNative(

                eq(mTestAddress), eq(AvrcpControllerService.PASS_THRU_CMD_ID_PAUSE), eq(KEY_DOWN));

        verify(mA2dpSinkService, never()).requestAudioFocus(mTestDevice, true);

    }



    /**

     * Test receiving a playback status of playing while we have focus

     *

     * Verify we do not send a pause command and never attempt to request audio focus

     */

    @Test

    public void testPlaybackWhilePlayingState() {

        when(mA2dpSinkService.getFocusState()).thenReturn(AudioManager.AUDIOFOCUS_GAIN);

        setUpConnectedState(true, true);

        Assert.assertTrue(mAvrcpStateMachine.isActive());

        mAvrcpStateMachine.sendMessage(

                AvrcpControllerStateMachine.MESSAGE_PROCESS_PLAY_STATUS_CHANGED,

                PlaybackStateCompat.STATE_PLAYING);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        verify(mAvrcpControllerService, never()).sendPassThroughCommandNative(

                eq(mTestAddress), eq(AvrcpControllerService.PASS_THRU_CMD_ID_PAUSE), eq(KEY_DOWN));

        verify(mA2dpSinkService, never()).requestAudioFocus(mTestDevice, true);

    }



    /**

     * Test receiving a playback status of playing from a device that isn't active

     *

     * Verify we do not send a pause command and never attempt to request audio focus

     */

    @Test

    public void testPlaybackWhileNotActiveDevice() {

        byte[] secondTestAddress = new byte[]{00, 01, 02, 03, 04, 06};

        BluetoothDevice secondTestDevice = mAdapter.getRemoteDevice(secondTestAddress);

        AvrcpControllerStateMachine secondAvrcpStateMachine =

                new AvrcpControllerStateMachine(secondTestDevice, mAvrcpControllerService);

        secondAvrcpStateMachine.start();



        setUpConnectedState(true, true);

        secondAvrcpStateMachine.connect(StackEvent.connectionStateChanged(true, true));

        TestUtils.waitForLooperToFinishScheduledTask(secondAvrcpStateMachine.getHandler()

                .getLooper());



        Assert.assertTrue(secondAvrcpStateMachine.isActive());

        Assert.assertFalse(mAvrcpStateMachine.isActive());



        mAvrcpStateMachine.sendMessage(

                AvrcpControllerStateMachine.MESSAGE_PROCESS_PLAY_STATUS_CHANGED,

                PlaybackStateCompat.STATE_PLAYING);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        verify(mAvrcpControllerService,

                timeout(ASYNC_CALL_TIMEOUT_MILLIS).times(1)).sendPassThroughCommandNative(

                eq(mTestAddress), eq(AvrcpControllerService.PASS_THRU_CMD_ID_PAUSE), eq(KEY_DOWN));

        verify(mA2dpSinkService, never()).requestAudioFocus(mTestDevice, true);



        secondAvrcpStateMachine.disconnect();

        TestUtils.waitForLooperToFinishScheduledTask(secondAvrcpStateMachine.getHandler()

                .getLooper());

        Assert.assertFalse(secondAvrcpStateMachine.isActive());

        Assert.assertFalse(mAvrcpStateMachine.isActive());

    }



    /**

     * Test that the correct device becomes active

     *
@@ -690,6 +768,12 @@ public class AvrcpControllerStateMachineTest { 
                .getLooper());

        Assert.assertFalse(mAvrcpStateMachine.isActive());

        Assert.assertTrue(secondAvrcpStateMachine.isActive());



        secondAvrcpStateMachine.disconnect();

        TestUtils.waitForLooperToFinishScheduledTask(secondAvrcpStateMachine.getHandler()

                .getLooper());

        Assert.assertFalse(secondAvrcpStateMachine.isActive());

        Assert.assertFalse(mAvrcpStateMachine.isActive());

    }



    /**
@@ -699,11 +783,11 @@ public class AvrcpControllerStateMachineTest { 
     */

    private int setUpConnectedState(boolean control, boolean browsing) {

        // Put test state machine into connected state

        mAvrcpStateMachine.start();

        Assert.assertThat(mAvrcpStateMachine.getCurrentState(),

                IsInstanceOf.instanceOf(AvrcpControllerStateMachine.Disconnected.class));



        mAvrcpStateMachine.connect(StackEvent.connectionStateChanged(control, browsing));

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        verify(mAvrcpControllerService, timeout(ASYNC_CALL_TIMEOUT_MILLIS).times(2)).sendBroadcast(

                mIntentArgument.capture(), eq(ProfileService.BLUETOOTH_PERM));

        Assert.assertThat(mAvrcpStateMachine.getCurrentState(),