OSDN Git Service

SAP: Fix exception at shutdown
authorCasper Bonde <c.bonde@samsung.com>
Fri, 17 Apr 2015 09:46:49 +0000 (11:46 +0200)
committerAndre Eisenbach <eisenbach@google.com>
Wed, 3 Jun 2015 05:23:43 +0000 (22:23 -0700)
Restructures the disconnect/shutdown code, to avoid exception.
Also ensure cleanup of notifications and profile public state.
This also fixes the bug introduced when handling ACL_disconnect
intent, where the rfcomm-listener was not restarted.

Change-Id: I3b4561f610abc77011689b7cfb67a4b54c41500c
Signed-off-by: Casper Bonde <c.bonde@samsung.com>
src/com/android/bluetooth/sap/SapRilReceiver.java
src/com/android/bluetooth/sap/SapServer.java
src/com/android/bluetooth/sap/SapService.java
tests/src/com/android/bluetooth/tests/SecurityTest.java

index 9ef430e..4bf515a 100644 (file)
@@ -97,9 +97,15 @@ public class SapRilReceiver implements Runnable {
         return mRilBtOutStream;
     }
 
-    private void onConnectComplete() {
-        if (mSapServerMsgHandler != null)
-            mSapServerMsgHandler.sendEmptyMessage(SapServer.SAP_MSG_RIL_CONNECT);
+    /**
+     * Notify SapServer that this class is ready for shutdown.
+     */
+    private void notifyShutdown() {
+        if (DEBUG) Log.i(TAG, "notifyShutdown()");
+        // If we are already shutdown, don't bother sending a notification.
+        synchronized (this) {
+            if (mSocket != null) sendShutdownMessage();
+        }
     }
 
     /**
@@ -114,19 +120,22 @@ public class SapRilReceiver implements Runnable {
          * stream from the socket, and when the socket is closed, the pending
          * reads never return nor throw and exception.
          * Hence here we use the shutdown method: */
-        if (mSocket != null) {
-            try {
-                mSocket.shutdownOutput();
-            } catch (IOException e) {}
-            try {
-                mSocket.shutdownInput();
-            } catch (IOException e) {}
-            try {
-                mSocket.close();
-            } catch (IOException ex) {
-                if (VERBOSE) Log.e(TAG,"Uncaught exception", ex);
+        synchronized (this) {
+            if (mSocket != null) {
+                try {
+                    mSocket.shutdownOutput();
+                } catch (IOException e) {}
+                try {
+                    mSocket.shutdownInput();
+                } catch (IOException e) {}
+                try {
+                    mSocket.close();
+                } catch (IOException ex) {
+                    if (VERBOSE) Log.e(TAG,"Uncaught exception", ex);
+                } finally {
+                    mSocket = null;
+                }
             }
-            mSocket = null;
         }
     }
 
@@ -197,7 +206,7 @@ public class SapRilReceiver implements Runnable {
             mRilBtOutStream = CodedOutputStreamMicro.newInstance(mSocket.getOutputStream());
 
             // Notify the SapServer that we have connected to the RilBtSocket
-            onConnectComplete();
+            sendRilConnectMessage();
 
             // The main loop - read messages and forward to SAP server
             for (;;) {
@@ -209,6 +218,11 @@ public class SapRilReceiver implements Runnable {
 
                 SapService.notifyUpdateWakeLock(mSapServiceHandler);
 
+                if (length == -1) {
+                    if (DEBUG) Log.i(TAG, "EOF reached - closing down.");
+                    break;
+                }
+
                 CodedInputStreamMicro msgStream =
                         CodedInputStreamMicro.newInstance(buffer, 0, length);
 
@@ -227,17 +241,27 @@ public class SapRilReceiver implements Runnable {
                     }
                 } // else simply ignore it
             }
+
         } catch (IOException e) {
-            shutdown(); /* Only needed in case of a connection error */
+            notifyShutdown(); /* Only needed in case of a connection error */
             Log.i(TAG, "'" + SOCKET_NAME_RIL_BT + "' socket inputStream closed", e);
-        }
-        finally {
+
+        finally {
             Log.i(TAG, "Disconnected from '" + SOCKET_NAME_RIL_BT + "' socket");
         }
     }
 
     /**
-     * Send message to the Sap Server Handler Thread
+     * Notify SapServer that the RIL socket is connected
+     */
+    private void sendRilConnectMessage() {
+        if (mSapServerMsgHandler != null) {
+            mSapServerMsgHandler.sendEmptyMessage(SapServer.SAP_MSG_RIL_CONNECT);
+        }
+    }
+
+    /**
+     * Send reply (solicited) message from the RIL to the Sap Server Handler Thread
      * @param sapMsg The message to send
      */
     private void sendClientMessage(SapMessage sapMsg) {
@@ -245,6 +269,19 @@ public class SapRilReceiver implements Runnable {
         mSapServerMsgHandler.sendMessage(newMsg);
     }
 
+    /**
+     * Send a shutdown signal to SapServer to indicate the
+     */
+    private void sendShutdownMessage() {
+        if (mSapServerMsgHandler != null) {
+            mSapServerMsgHandler.sendEmptyMessage(SapServer.SAP_RIL_SOCK_CLOSED);
+        }
+    }
+
+    /**
+     * Send indication (unsolicited) message from RIL to the Sap Server Handler Thread
+     * @param sapMsg The message to send
+     */
     private void sendRilIndMessage(SapMessage sapMsg) {
         Message newMsg = mSapServerMsgHandler.obtainMessage(SapServer.SAP_MSG_RIL_IND, sapMsg);
         mSapServerMsgHandler.sendMessage(newMsg);
index 7eed1d6..09a1975 100644 (file)
@@ -80,7 +80,8 @@ public class SapServer extends Thread implements Callback {
     public static final int SAP_MSG_RFC_REPLY =   0x00;
     public static final int SAP_MSG_RIL_CONNECT = 0x01;
     public static final int SAP_MSG_RIL_REQ =     0x02;
-    public static final int SAP_MSG_RIL_IND =     0x04;
+    public static final int SAP_MSG_RIL_IND =     0x03;
+    public static final int SAP_RIL_SOCK_CLOSED = 0x04;
 
     public static final String SAP_DISCONNECT_ACTION = "com.android.bluetooth.sap.action.DISCONNECT_ACTION";
     public static final String SAP_DISCONNECT_TYPE_EXTRA = "com.android.bluetooth.sap.extra.DISCONNECT_TYPE";
@@ -134,21 +135,25 @@ public class SapServer extends Thread implements Callback {
                     if(state != null) {
                         if(state.equals(TelephonyManager.EXTRA_STATE_IDLE)) {
                             if(DEBUG) Log.i(TAG, "sending RIL.ACTION_RIL_RECONNECT_OFF_REQ intent");
-                            // TODO: Send connect request to RIL
                             SapMessage fakeConReq = new SapMessage(SapMessage.ID_CONNECT_REQ);
                             fakeConReq.setMaxMsgSize(mMaxMsgSize);
                             onConnectRequest(fakeConReq);
                         }
                     }
                 }
-            } else if (intent.getAction().equals(SAP_DISCONNECT_ACTION)
-                    && mState != SAP_STATE.DISCONNECTED
-                    && mState != SAP_STATE.DISCONNECTING ) {
-
-                int disconnectType = intent.getIntExtra(SapServer.SAP_DISCONNECT_TYPE_EXTRA,SapMessage.DISC_GRACEFULL);
+            } else if (intent.getAction().equals(SAP_DISCONNECT_ACTION)) {
+                int disconnectType = intent.getIntExtra(SapServer.SAP_DISCONNECT_TYPE_EXTRA,
+                        SapMessage.DISC_GRACEFULL);
                 Log.v(TAG, " - Received SAP_DISCONNECT_ACTION type: " + disconnectType);
-                sendDisconnectInd(disconnectType);
 
+                if(disconnectType == SapMessage.DISC_RFCOMM) {
+                    // At timeout we need to close the RFCOMM socket to complete shutdown
+                    shutdown();
+                } else if( mState != SAP_STATE.DISCONNECTED
+                    && mState != SAP_STATE.DISCONNECTING ) {
+                    // The user pressed disconnect - initiate disconnect sequence.
+                    sendDisconnectInd(disconnectType);
+                }
             } else {
                 Log.w(TAG, "RIL-BT received unexpected Intent: " + intent.getAction());
             }
@@ -254,14 +259,21 @@ public class SapServer extends Thread implements Callback {
                     .build();
         }
 
-
-        notification.flags |= Notification.FLAG_NO_CLEAR |Notification.FLAG_ONLY_ALERT_ONCE; /* cannot be set with the builder */
+        // cannot be set with the builder
+        notification.flags |= Notification.FLAG_NO_CLEAR |Notification.FLAG_ONLY_ALERT_ONCE;
 
         NotificationManager notificationManager =
                 (NotificationManager) mContext.getSystemService(Context.NOTIFICATION_SERVICE);
 
         notificationManager.notify(NOTIFICATION_ID, notification);
     }
+
+    void clearNotification() {
+        NotificationManager notificationManager =
+                (NotificationManager) mContext.getSystemService(Context.NOTIFICATION_SERVICE);
+        notificationManager.cancel(SapServer.NOTIFICATION_ID);
+    }
+
     /**
      * The SapServer RFCOMM reader thread. Sets up the handler thread and handle
      * all read from the RFCOMM in-socket. This thread also handle writes to the RIL socket.
@@ -326,13 +338,27 @@ public class SapServer extends Thread implements Callback {
                             /* Forward these to the RIL regardless of the state, and clear any pending resp */
                             clearPendingRilResponses(msg);
                             break;
+                        case SapMessage.ID_SET_TRANSPORT_PROTOCOL_REQ:
+                            /* The RIL might support more protocols that specified in the SAP,
+                             * Allow only the valid values. */
+                            if(mState == SAP_STATE.CONNECTED
+                                    && msg.getTransportProtocol() != 0
+                                    && msg.getTransportProtocol() != 1) {
+                                Log.w(TAG, "Invalid TransportProtocol received:"
+                                        + msg.getTransportProtocol());
+                                // We shall only handle one request at the time, hence return error
+                                SapMessage errorReply = new SapMessage(SapMessage.ID_ERROR_RESP);
+                                sendClientMessage(errorReply);
+                                msg = null;
+                            }
+                            // Fall through
                         default:
                             /* remaining cases just needs to be forwarded to the RIL unless we are in busy state. */
                             if(mState != SAP_STATE.CONNECTED) {
                                 Log.w(TAG, "Message received in STATE != CONNECTED - state = " + mState.name());
                                 /* We shall only handle one request at the time, hence return error */
-                                SapMessage atrReply = new SapMessage(SapMessage.ID_ERROR_RESP);
-                                sendClientMessage(atrReply);
+                                SapMessage errorReply = new SapMessage(SapMessage.ID_ERROR_RESP);
+                                sendClientMessage(errorReply);
                                 msg = null;
                             }
                         }
@@ -376,23 +402,38 @@ public class SapServer extends Thread implements Callback {
                     Log.e(TAG, "Interrupt received while waitinf for de-init to complete", e);
                 }
             }
-            mContext.unregisterReceiver(mIntentReceiver);
+
+            if(mIntentReceiver != null) {
+                mContext.unregisterReceiver(mIntentReceiver);
+                mIntentReceiver = null;
+            }
+            stopDisconnectTimer();
+            clearNotification();
+
             if(mHandlerThread != null) try {
                 mHandlerThread.quit();
                 mHandlerThread.join();
+                mHandlerThread = null;
             } catch (InterruptedException e) {}
             if(mRilBtReceiverThread != null) try {
+                if(mRilBtReceiver != null) {
+                    mRilBtReceiver.shutdown();
+                    mRilBtReceiver = null;
+                }
                 mRilBtReceiverThread.join();
+                mRilBtReceiverThread = null;
             } catch (InterruptedException e) {}
 
             if(mRfcommIn != null) try {
                 if(VERBOSE) Log.i(TAG, "Closing mRfcommIn...");
                 mRfcommIn.close();
+                mRfcommIn = null;
             } catch (IOException e) {}
 
             if(mRfcommOut != null) try {
                 if(VERBOSE) Log.i(TAG, "Closing mRfcommOut...");
                 mRfcommOut.close();
+                mRfcommOut = null;
             } catch (IOException e) {}
 
             if (mSapServiceHandler != null) {
@@ -554,6 +595,12 @@ public class SapServer extends Thread implements Callback {
             sapMsg = (SapMessage) msg.obj;
             handleRilInd(sapMsg);
             break;
+        case SAP_RIL_SOCK_CLOSED:
+            /* The RIL socket was closed unexpectedly, send immediate disconnect indication
+               - close RFCOMM after timeout if no response. */
+            sendDisconnectInd(SapMessage.DISC_IMMEDIATE);
+            startDisconnectTimer(SapMessage.DISC_RFCOMM, DISCONNECT_TIMEOUT_RFCOMM);
+            break;
         default:
             /* Message not handled */
             return false;
@@ -578,6 +625,7 @@ public class SapServer extends Thread implements Callback {
         mRfcommIn = null;
         mRfcommOut = null;
         stopDisconnectTimer();
+        clearNotification();
     }
 
     private void startDisconnectTimer(int discType, int timeMs) {
@@ -648,32 +696,19 @@ public class SapServer extends Thread implements Callback {
                     break;
                 case SapMessage.ID_DISCONNECT_RESP:
                     if(mState == SAP_STATE.DISCONNECTING) {
-                        /* Close the RIL-BT output Stream and signal to SapRilReceiver to close down the input stream. */
-                        if(DEBUG) Log.i(TAG, "ID_DISCONNECT_RESP received in SAP_STATE.DISCONNECTING" +
-                                             " - shutdown bt-ril ");
-                        /* We need to close down the RilBtReceiverThread before signaling to RILJ
-                         * that it can re-open the socket to RILC. */
-                        mRilBtReceiver.shutdown();
-                        mRilBtOutStream = null;
-                        try {
-                            mRilBtReceiverThread.join();
-                        } catch (InterruptedException e) {
-                            Log.e(TAG_HANDLER, "Exception occured while waiting for thread to exit.", e);
-                        }
-                        mRilBtReceiverThread = null;
-
-                        // Wait for the thread to close, as we need the socket to be closed before signaling the RIL to reopen.
-                        // Disconnecting
-                        // Send the disconnect resp, and wait for the client to close the Rfcomm, but start a
-                        // timeout timer, just to be sure. Use alarm, to ensure we wake the host to close the
-                        // connection to minimize power consumption.
+                        /* Close the RIL-BT output Stream and signal to SapRilReceiver to close
+                         * down the input stream. */
+                        if(DEBUG) Log.i(TAG, "ID_DISCONNECT_RESP received in SAP_STATE." +
+                                "DISCONNECTING.");
+
+                        /* Send the disconnect resp, and wait for the client to close the Rfcomm,
+                         * but start a timeout timer, just to be sure. Use alarm, to ensure we wake
+                         * the host to close the connection to minimize power consumption. */
                         SapMessage disconnectResp = new SapMessage(SapMessage.ID_DISCONNECT_RESP);
                         changeState(SAP_STATE.DISCONNECTED);
                         sapMsg = disconnectResp;
-                        //since we are disconnected we remove the notification
-                        NotificationManager notificationManager =
-                                (NotificationManager) mContext.getSystemService(Context.NOTIFICATION_SERVICE);
-                        notificationManager.cancel(SapServer.NOTIFICATION_ID);
+                        startDisconnectTimer(SapMessage.DISC_RFCOMM, DISCONNECT_TIMEOUT_RFCOMM);
+                        mDeinitSignal.countDown(); /* Signal deinit complete */
                     } else { /* DISCONNECTED */
                         mDeinitSignal.countDown(); /* Signal deinit complete */
                         if(mIsLocalInitDisconnect == true) {
@@ -763,11 +798,15 @@ public class SapServer extends Thread implements Callback {
      */
     private void sendReply(SapMessage msg) {
         if(VERBOSE) Log.i(TAG_HANDLER, "sendReply() RFCOMM - " + SapMessage.getMsgTypeName(msg.getMsgType()));
-        try {
-            msg.write(mRfcommOut);
-            mRfcommOut.flush();
-        } catch (IOException e) {
-            Log.w(TAG_HANDLER, e);
+        if(mRfcommOut != null) { // Needed to handle brutal shutdown from car-kit and out of range
+            try {
+                msg.write(mRfcommOut);
+                mRfcommOut.flush();
+            } catch (IOException e) {
+                Log.w(TAG_HANDLER, e);
+                /* As we cannot write to the rfcomm channel we are disconnected.
+                   Shutdown and prepare for a new connect. */
+            }
         }
     }
 
index dff7163..2188a26 100644 (file)
@@ -66,8 +66,8 @@ public class SapService extends ProfileService {
      *
      * NOTE: While connected the the Nokia 616 car-kit it was noticed that the carkit do
      *       TRANSFER_APDU_REQ with 20-30 seconds interval, and it sends no requests less than 1 sec
-     *       apart. Additionally the responses from the RIL comes within a few ms, hence a one
-     *       second timeout should be enough.
+     *       apart. Additionally the responses from the RIL seems to come within 100 ms, hence a
+     *       one second timeout should be enough.
      */
     private static final int RELEASE_WAKE_LOCK_DELAY = 1000;
 
@@ -280,14 +280,6 @@ public class SapService extends ProfileService {
          * supposed to close the RFCOMM connection. */
         if (VERBOSE) Log.v(TAG, "SAP Service stopSapServerSession");
 
-        // Release the wake lock if SAP transactions is over
-        /* TODO: Why do we need to hold a wakeLock? I assume the lower layers will wake
-         * the host at incoming data, hence I'm not sure why we need this wake lock?
-         * Any SAP req/ind is triggered either by incoming data from the client, from the
-         * RIL deamon or a user initiated disconnect.
-         * Perhaps we should only hold a wakelock while handling a message?
-         */
-
         mAcceptThread = null;
         closeConnectionSocket();
         closeServerSocket();
@@ -374,6 +366,7 @@ public class SapService extends ProfileService {
                         intent.putExtra(BluetoothDevice.EXTRA_PACKAGE_NAME, getPackageName());
 
                         mIsWaitingAuthorization = true;
+                        setUserTimeoutAlarm();
                         sendBroadcast(intent, BLUETOOTH_ADMIN_PERM);
 
                         if (VERBOSE) Log.v(TAG, "waiting for authorization for connection from: "
@@ -622,9 +615,11 @@ public class SapService extends ProfileService {
         if (mAlarmManager == null){
             mAlarmManager =(AlarmManager) this.getSystemService (Context.ALARM_SERVICE);
         }
+        if(mRemoveTimeoutMsg) {
+            cancelUserTimeoutAlarm();
+        }
         mRemoveTimeoutMsg = true;
-        Intent timeoutIntent =
-                new Intent(USER_CONFIRM_TIMEOUT_ACTION);
+        Intent timeoutIntent = new Intent(USER_CONFIRM_TIMEOUT_ACTION);
         PendingIntent pIntent = PendingIntent.getBroadcast(this, 0, timeoutIntent, 0);
         mAlarmManager.set(AlarmManager.RTC_WAKEUP, System.currentTimeMillis()
                 + USER_CONFIRM_TIMEOUT_VALUE,pIntent);
@@ -632,8 +627,8 @@ public class SapService extends ProfileService {
 
     private void cancelUserTimeoutAlarm(){
         if (DEBUG)Log.d(TAG,"cancelUserTimeOutAlarm()");
-        Intent intent = new Intent(this, SapService.class);
-        PendingIntent sender = PendingIntent.getBroadcast(this, 0, intent, 0);
+        Intent timeoutIntent = new Intent(USER_CONFIRM_TIMEOUT_ACTION);
+        PendingIntent sender = PendingIntent.getBroadcast(this, 0, timeoutIntent, 0);
         AlarmManager alarmManager = (AlarmManager) this.getSystemService(Context.ALARM_SERVICE);
         alarmManager.cancel(sender);
         mRemoveTimeoutMsg = false;
@@ -729,7 +724,8 @@ public class SapService extends ProfileService {
                                     + result);
                         }
                     }
-                    stopSapServerSession();
+                    // Ensure proper cleanup, and prepare for new connect.
+                    mSessionStatusHandler.sendEmptyMessage(MSG_SERVERSESSION_CLOSE);
                 }
             } else if (action.equals(USER_CONFIRM_TIMEOUT_ACTION)){
                 if (DEBUG) Log.d(TAG, "USER_CONFIRM_TIMEOUT ACTION Received.");
@@ -740,7 +736,7 @@ public class SapService extends ProfileService {
                 BluetoothDevice device = intent.getParcelableExtra(BluetoothDevice.EXTRA_DEVICE);
 
                 if (mRemoteDevice == null || device == null) {
-                    Log.e(TAG, "Unexpected error!");
+                    Log.i(TAG, "Unexpected error!");
                     return;
                 }
 
@@ -748,10 +744,13 @@ public class SapService extends ProfileService {
 
                 if (mRemoteDevice.equals(device) && mRemoveTimeoutMsg) {
                     // Send any pending timeout now, as ACL got disconnected.
+                    cancelUserTimeoutAlarm();
                     mSessionStatusHandler.removeMessages(USER_TIMEOUT);
                     sendCancelUserConfirmationIntent(mRemoteDevice);
                     mIsWaitingAuthorization = false;
                     mRemoveTimeoutMsg = false;
+                    // Ensure proper cleanup, and prepare for new connect.
+                    mSessionStatusHandler.sendEmptyMessage(MSG_SERVERSESSION_CLOSE);
                 }
             }
         }
index e950201..39d9f58 100644 (file)
@@ -2,6 +2,7 @@ package com.android.bluetooth.tests;
 
 import android.bluetooth.BluetoothAdapter;
 import android.bluetooth.BluetoothDevice;
+import android.bluetooth.BluetoothSocket;
 import android.bluetooth.BluetoothUuid;
 import android.test.AndroidTestCase;
 import android.util.Log;
@@ -13,23 +14,34 @@ public class SecurityTest extends AndroidTestCase {
 
     public void connectSapNoSec() {
         BluetoothAdapter bt = BluetoothAdapter.getDefaultAdapter();
-        if(bt == null) {
+        if (bt == null) {
             Log.e(TAG,"No Bluetooth Device!");
             assertTrue(false);
         }
 
         BluetoothTestUtils.enableBt(bt);
+        Log.i(TAG,"BT Enabled");
         BluetoothDevice serverDevice = bt.getRemoteDevice(ObexTest.SERVER_ADDRESS);
-        try {
-            serverDevice.createInsecureRfcommSocketToServiceRecord(BluetoothUuid.SAP.getUuid());
-        } catch (IOException e) {
-            Log.e(TAG, "Failed to create connection", e);
-        }
+        Log.i(TAG,"ServerDevice: " + serverDevice);
 
         try {
-            Thread.sleep(1000);
+            BluetoothSocket socket =
+                    serverDevice.createInsecureRfcommSocketToServiceRecord(BluetoothUuid.SAP.getUuid());
+            Log.i(TAG,"createInsecureRfcommSocketToServiceRecord() - waiting for connect...");
+            socket.connect();
+            Log.i(TAG,"Connected!");
+            Thread.sleep(5000);
+            Log.i(TAG,"Closing...");
+            socket.close();
+            Log.i(TAG,"Closed!");
+
         } catch (InterruptedException e) {
             Log.w(TAG, "Sleep interrupted", e);
+            fail();
+
+        }  catch (IOException e) {
+            Log.e(TAG, "Failed to create connection", e);
+            fail();
         }
     }
 }