CameraService: Link to client binder death at end of connect

If, within a binder Looper thread, a Binder object death notifier is
registered, and then a nested Binder transaction is begun, that
transaction may process a death notification callback for the
just-registered object, if it was dead at the time of registration.

This can lead to an unexpected nested call into the service, and cause
deadlock.

To avoid this, move the death notifier registration to the end of
handling the connect transaction.

Also remove one extra bit of disconnect logging.

Bug: 23525545
Change-Id: If01cbaf42704f55134118afefc9a8f7bdb014e09

2 files changed, 11 insertions, 8 deletions

diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp
index f5a0887..9230994 100644
--- a/services/camera/libcameraservice/CameraService.cpp
+++ b/services/camera/libcameraservice/CameraService.cpp
@@ -945,6 +945,16 @@ void CameraService::finishConnectLocked(const sp<BasicClient>& client,
         LOG_ALWAYS_FATAL("%s: Invalid state for CameraService, clients not evicted properly",
                 __FUNCTION__);
     }
+
+    // And register a death notification for the client callback. Do
+    // this last to avoid Binder policy where a nested Binder
+    // transaction might be pre-empted to service the client death
+    // notification if the client process dies before linkToDeath is
+    // invoked.
+    sp<IBinder> remoteCallback = client->getRemote();
+    if (remoteCallback != nullptr) {
+        remoteCallback->linkToDeath(this);
+    }
 }
 
 status_t CameraService::handleEvictionsLocked(const String8& cameraId, int clientPid,
@@ -1884,11 +1894,9 @@ CameraService::BasicClient::~BasicClient() {
 
 void CameraService::BasicClient::disconnect() {
     if (mDisconnected) {
-        ALOGE("%s: Disconnect called on already disconnected client for device %d", __FUNCTION__,
-                mCameraId);
         return;
     }
-    mDisconnected = true;;
+    mDisconnected = true;
 
     mCameraService->removeByClient(this);
     mCameraService->logDisconnected(String8::format("%d", mCameraId), mClientPid,
diff --git a/services/camera/libcameraservice/CameraService.h b/services/camera/libcameraservice/CameraService.h
index 330a19d..cd97b08 100644
--- a/services/camera/libcameraservice/CameraService.h
+++ b/services/camera/libcameraservice/CameraService.h
@@ -873,11 +873,6 @@ status_t CameraService::connectHelper(const sp<CALLBACK>& cameraCb, const String
             return ret;
         }
 
-        sp<IBinder> remoteCallback = client->getRemote();
-        if (remoteCallback != nullptr) {
-            remoteCallback->linkToDeath(this);
-        }
-
         // Update shim paremeters for legacy clients
         if (effectiveApiLevel == API_1) {
             // Assume we have always received a Client subclass for API1