From feb9b89705b7705c3dd74bb630ad95679547d8ce Mon Sep 17 00:00:00 2001 From: Seulki Shin Date: Fri, 27 Mar 2020 14:05:16 -0700 Subject: [PATCH] audioservice: fix not able to disconnect VoIP call startBluetoothSco() is not safe enough from subseqent method call which was found on the Wechat. For instance, VoIP call will not be terminated properly in the following call flows: startBluetoothSco() (-> incCount) -> startBluetoothSco() (-> incCount) -> stopBluetoothSco() (-> decCount) -> at this point, it cannot reached to disconnectBluetoothScoAudioHelper() due to mStartCount is not 0 from requestScoState() To fix this, mStartCount was removed because it's usage is unclear. Also incCount, decCount was removed because it makes difficult to understand what the purpose of those functions from their name. Bug: 150312620 Test: manual test cell and VoIP calls with BT SCO HS. Change-Id: Iabc44795b8a87c10b00d84c67de9938bef385841 Signed-off-by: slki.shin --- .../server/audio/AudioDeviceBroker.java | 2 +- .../com/android/server/audio/BtHelper.java | 147 ++++++------------ 2 files changed, 52 insertions(+), 97 deletions(-) diff --git a/services/core/java/com/android/server/audio/AudioDeviceBroker.java b/services/core/java/com/android/server/audio/AudioDeviceBroker.java index b546120e2b954..c2c79d361996c 100644 --- a/services/core/java/com/android/server/audio/AudioDeviceBroker.java +++ b/services/core/java/com/android/server/audio/AudioDeviceBroker.java @@ -324,7 +324,7 @@ import java.io.PrintWriter; } /*package*/ void setBluetoothScoOn(boolean on, String eventSource) { - //Log.i(TAG, "setBluetoothScoOnInt: " + on + " " + eventSource); + //Log.i(TAG, "setBluetoothScoOn: " + on + " " + eventSource); synchronized (mDeviceStateLock) { if (on) { // do not accept SCO ON if SCO audio is not connected diff --git a/services/core/java/com/android/server/audio/BtHelper.java b/services/core/java/com/android/server/audio/BtHelper.java index 36332c0ad25cd..93d1bede9de84 100644 --- a/services/core/java/com/android/server/audio/BtHelper.java +++ b/services/core/java/com/android/server/audio/BtHelper.java @@ -58,6 +58,7 @@ public class BtHelper { } // List of clients having issued a SCO start request + @GuardedBy("BtHelper.this") private final @NonNull ArrayList mScoClients = new ArrayList(); // BluetoothHeadset API to control SCO connection @@ -356,9 +357,8 @@ public class BtHelper { // client is created. final long ident = Binder.clearCallingIdentity(); try { - eventSource += " client count before=" + client.getCount(); AudioService.sDeviceLogger.log(new AudioEventLogger.StringEvent(eventSource)); - client.incCount(scoAudioMode); + client.requestScoState(BluetoothHeadset.STATE_AUDIO_CONNECTED, scoAudioMode); } catch (NullPointerException e) { Log.e(TAG, "Null ScoClient", e); } @@ -375,9 +375,15 @@ public class BtHelper { // and this must be done on behalf of system server to make sure permissions are granted. final long ident = Binder.clearCallingIdentity(); if (client != null) { - eventSource += " client count before=" + client.getCount(); AudioService.sDeviceLogger.log(new AudioEventLogger.StringEvent(eventSource)); - client.decCount(); + client.requestScoState(BluetoothHeadset.STATE_AUDIO_DISCONNECTED, + SCO_MODE_VIRTUAL_CALL); + // If a disconnection is pending, the client will be removed whne clearAllScoClients() + // is called form receiveBtEvent() + if (mScoAudioState != SCO_STATE_DEACTIVATE_REQ + && mScoAudioState != SCO_STATE_DEACTIVATING) { + client.remove(false /*stop */, true /*unregister*/); + } } Binder.restoreCallingIdentity(ident); } @@ -657,25 +663,33 @@ public class BtHelper { @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void scoClientDied(Object obj) { final ScoClient client = (ScoClient) obj; + client.remove(true /*stop*/, false /*unregister*/); Log.w(TAG, "SCO client died"); - int index = mScoClients.indexOf(client); - if (index < 0) { - Log.w(TAG, "unregistered SCO client died"); - } else { - client.clearCount(true); - mScoClients.remove(client); - } } private class ScoClient implements IBinder.DeathRecipient { private IBinder mCb; // To be notified of client's death private int mCreatorPid; - private int mStartcount; // number of SCO connections started by this client ScoClient(IBinder cb) { mCb = cb; mCreatorPid = Binder.getCallingPid(); - mStartcount = 0; + } + + public void registerDeathRecipient() { + try { + mCb.linkToDeath(this, 0); + } catch (RemoteException e) { + Log.w(TAG, "ScoClient could not link to " + mCb + " binder death"); + } + } + + public void unregisterDeathRecipient() { + try { + mCb.unlinkToDeath(this, 0); + } catch (NoSuchElementException e) { + Log.w(TAG, "ScoClient could not not unregistered to binder"); + } } @Override @@ -685,70 +699,6 @@ public class BtHelper { mDeviceBroker.postScoClientDied(this); } - // @GuardedBy("AudioDeviceBroker.mSetModeLock") - // @GuardedBy("AudioDeviceBroker.mDeviceStateLock") - @GuardedBy("BtHelper.this") - void incCount(int scoAudioMode) { - if (!requestScoState(BluetoothHeadset.STATE_AUDIO_CONNECTED, scoAudioMode)) { - Log.e(TAG, "Request sco connected with scoAudioMode(" - + scoAudioMode + ") failed"); - return; - } - if (mStartcount == 0) { - try { - mCb.linkToDeath(this, 0); - } catch (RemoteException e) { - // client has already died! - Log.w(TAG, "ScoClient incCount() could not link to " - + mCb + " binder death"); - } - } - mStartcount++; - } - - // @GuardedBy("AudioDeviceBroker.mSetModeLock") - // @GuardedBy("AudioDeviceBroker.mDeviceStateLock") - @GuardedBy("BtHelper.this") - void decCount() { - if (mStartcount == 0) { - Log.w(TAG, "ScoClient.decCount() already 0"); - } else { - mStartcount--; - if (mStartcount == 0) { - try { - mCb.unlinkToDeath(this, 0); - } catch (NoSuchElementException e) { - Log.w(TAG, "decCount() going to 0 but not registered to binder"); - } - } - if (!requestScoState(BluetoothHeadset.STATE_AUDIO_DISCONNECTED, 0)) { - Log.w(TAG, "Request sco disconnected with scoAudioMode(0) failed"); - } - } - } - - // @GuardedBy("AudioDeviceBroker.mSetModeLock") - // @GuardedBy("AudioDeviceBroker.mDeviceStateLock") - @GuardedBy("BtHelper.this") - void clearCount(boolean stopSco) { - if (mStartcount != 0) { - try { - mCb.unlinkToDeath(this, 0); - } catch (NoSuchElementException e) { - Log.w(TAG, "clearCount() mStartcount: " - + mStartcount + " != 0 but not registered to binder"); - } - } - mStartcount = 0; - if (stopSco) { - requestScoState(BluetoothHeadset.STATE_AUDIO_DISCONNECTED, 0); - } - } - - int getCount() { - return mStartcount; - } - IBinder getBinder() { return mCb; } @@ -757,23 +707,14 @@ public class BtHelper { return mCreatorPid; } - private int totalCount() { - int count = 0; - for (ScoClient mScoClient : mScoClients) { - count += mScoClient.getCount(); - } - return count; - } - // @GuardedBy("AudioDeviceBroker.mSetModeLock") //@GuardedBy("AudioDeviceBroker.mDeviceStateLock") @GuardedBy("BtHelper.this") private boolean requestScoState(int state, int scoAudioMode) { checkScoAudioState(); - int clientCount = totalCount(); - if (clientCount != 0) { + if (mScoClients.size() != 1) { Log.i(TAG, "requestScoState: state=" + state + ", scoAudioMode=" + scoAudioMode - + ", clientCount=" + clientCount); + + ", num SCO clients=" + mScoClients.size()); return true; } if (state == BluetoothHeadset.STATE_AUDIO_CONNECTED) { @@ -842,12 +783,14 @@ public class BtHelper { mScoAudioState = SCO_STATE_ACTIVE_INTERNAL; broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_CONNECTED); break; + case SCO_STATE_ACTIVE_INTERNAL: + Log.w(TAG, "requestScoState: already in ACTIVE mode, simply return"); + break; default: Log.w(TAG, "requestScoState: failed to connect in state " + mScoAudioState + ", scoAudioMode=" + scoAudioMode); broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_DISCONNECTED); return false; - } } else if (state == BluetoothHeadset.STATE_AUDIO_DISCONNECTED) { switch (mScoAudioState) { @@ -893,6 +836,18 @@ public class BtHelper { } return true; } + + @GuardedBy("BtHelper.this") + void remove(boolean stop, boolean unregister) { + if (unregister) { + unregisterDeathRecipient(); + } + if (stop) { + requestScoState(BluetoothHeadset.STATE_AUDIO_DISCONNECTED, + SCO_MODE_VIRTUAL_CALL); + } + mScoClients.remove(this); + } } //----------------------------------------------------- @@ -946,6 +901,7 @@ public class BtHelper { } + @GuardedBy("BtHelper.this") private ScoClient getScoClient(IBinder cb, boolean create) { for (ScoClient existingClient : mScoClients) { if (existingClient.getBinder() == cb) { @@ -954,6 +910,7 @@ public class BtHelper { } if (create) { ScoClient newClient = new ScoClient(cb); + newClient.registerDeathRecipient(); mScoClients.add(newClient); return newClient; } @@ -964,18 +921,16 @@ public class BtHelper { //@GuardedBy("AudioDeviceBroker.mDeviceStateLock") @GuardedBy("BtHelper.this") private void clearAllScoClients(int exceptPid, boolean stopSco) { - ScoClient savedClient = null; + final ArrayList clients = new ArrayList(); for (ScoClient cl : mScoClients) { if (cl.getPid() != exceptPid) { - cl.clearCount(stopSco); - } else { - savedClient = cl; + clients.add(cl); } } - mScoClients.clear(); - if (savedClient != null) { - mScoClients.add(savedClient); + for (ScoClient cl : clients) { + cl.remove(stopSco, true /*unregister*/); } + } private boolean getBluetoothHeadset() {