contexthub: close invalid session open requests

- Move halCloseEndpointSession(NoThrow) into ContextHubEndpointManager
- Call client side onCloseEndpointSession separately
- When there's a duplicated/invalid open request, call
  closeEndpointSession as needed

Test: ContextHubEndpointTest
Flag: EXEMPT, bug fix
Bug: 387716930
Change-Id: I802e09184608341186591188d15ec943b654269e

3 files changed, 201 insertions, 49 deletions

diff --git a/services/core/java/com/android/server/location/contexthub/ContextHubEndpointBroker.java b/services/core/java/com/android/server/location/contexthub/ContextHubEndpointBroker.java
index 2d937bdcc683..9bbcfe171d9d 100644
--- a/services/core/java/com/android/server/location/contexthub/ContextHubEndpointBroker.java
+++ b/services/core/java/com/android/server/location/contexthub/ContextHubEndpointBroker.java
@@ -299,7 +299,8 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
             throw new IllegalArgumentException(
                     "Unknown session ID in closeSession: id=" + sessionId);
         }
-        halCloseEndpointSession(sessionId, ContextHubServiceUtil.toHalReason(reason));
+        mEndpointManager.halCloseEndpointSession(
+                sessionId, ContextHubServiceUtil.toHalReason(reason));
     }
 
     @Override
@@ -310,7 +311,7 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
             // Iterate in reverse since cleanupSessionResources will remove the entry
             for (int i = mSessionMap.size() - 1; i >= 0; i--) {
                 int id = mSessionMap.keyAt(i);
-                halCloseEndpointSessionNoThrow(id, Reason.ENDPOINT_GONE);
+                mEndpointManager.halCloseEndpointSessionNoThrow(id, Reason.ENDPOINT_GONE);
                 cleanupSessionResources(id);
             }
         }
@@ -442,7 +443,8 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
                     int id = mSessionMap.keyAt(i);
                     HubEndpointInfo target = mSessionMap.get(id).getRemoteEndpointInfo();
                     if (!hasEndpointPermissions(target)) {
-                        halCloseEndpointSessionNoThrow(id, Reason.PERMISSION_DENIED);
+                        mEndpointManager.halCloseEndpointSessionNoThrow(
+                                id, Reason.PERMISSION_DENIED);
                         onCloseEndpointSession(id, Reason.PERMISSION_DENIED);
                         // Resource cleanup is done in onCloseEndpointSession
                     }
@@ -501,17 +503,7 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
         mContextHubEndpointCallback.asBinder().linkToDeath(this, 0 /* flags */);
     }
 
-    /* package */ void onEndpointSessionOpenRequest(
-            int sessionId, HubEndpointInfo initiator, String serviceDescriptor) {
-        Optional<Byte> error =
-                onEndpointSessionOpenRequestInternal(sessionId, initiator, serviceDescriptor);
-        if (error.isPresent()) {
-            halCloseEndpointSessionNoThrow(sessionId, error.get());
-            onCloseEndpointSession(sessionId, error.get());
-            // Resource cleanup is done in onCloseEndpointSession
-        }
-    }
-
+    /** Handle close endpoint callback to the client side */
     /* package */ void onCloseEndpointSession(int sessionId, byte reason) {
         if (!cleanupSessionResources(sessionId)) {
             Log.w(TAG, "Unknown session ID in onCloseEndpointSession: id=" + sessionId);
@@ -583,7 +575,7 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
         }
     }
 
-    private Optional<Byte> onEndpointSessionOpenRequestInternal(
+    /* package */ Optional<Byte> onEndpointSessionOpenRequest(
             int sessionId, HubEndpointInfo initiator, String serviceDescriptor) {
         if (!hasEndpointPermissions(initiator)) {
             Log.e(
@@ -592,15 +584,41 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
                             + initiator
                             + " doesn't have permission for "
                             + mEndpointInfo);
-            return Optional.of(Reason.PERMISSION_DENIED);
+            byte reason = Reason.PERMISSION_DENIED;
+            onCloseEndpointSession(sessionId, reason);
+            return Optional.of(reason);
         }
 
+        // Check & handle error cases for duplicated session id.
+        final boolean existingSession;
+        final boolean existingSessionActive;
         synchronized (mOpenSessionLock) {
             if (hasSessionId(sessionId)) {
-                Log.e(TAG, "Existing session in onEndpointSessionOpenRequest: id=" + sessionId);
-                return Optional.of(Reason.UNSPECIFIED);
+                existingSession = true;
+                existingSessionActive = mSessionMap.get(sessionId).isActive();
+                Log.w(
+                        TAG,
+                        "onEndpointSessionOpenRequest: "
+                                + "Existing session ID: "
+                                + sessionId
+                                + ", isActive: "
+                                + existingSessionActive);
+            } else {
+                existingSession = false;
+                existingSessionActive = false;
+                mSessionMap.put(sessionId, new Session(initiator, true));
+            }
+        }
+
+        if (existingSession) {
+            if (existingSessionActive) {
+                // Existing session is already active, call onSessionOpenComplete.
+                openSessionRequestComplete(sessionId);
+                return Optional.empty();
             }
-            mSessionMap.put(sessionId, new Session(initiator, true));
+            // Reject the session open request for now. Consider invalidating previous pending
+            // session open request based on timeout.
+            return Optional.of(Reason.OPEN_ENDPOINT_SESSION_REQUEST_REJECTED);
         }
 
         boolean success =
@@ -608,7 +626,11 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
                         (consumer) ->
                                 consumer.onSessionOpenRequest(
                                         sessionId, initiator, serviceDescriptor));
-        return success ? Optional.empty() : Optional.of(Reason.UNSPECIFIED);
+        byte reason = Reason.UNSPECIFIED;
+        if (!success) {
+            onCloseEndpointSession(sessionId, reason);
+        }
+        return success ? Optional.empty() : Optional.of(reason);
     }
 
     private byte onMessageReceivedInternal(int sessionId, HubMessage message) {
@@ -655,29 +677,6 @@ public class ContextHubEndpointBroker extends IContextHubEndpoint.Stub
     }
 
     /**
-     * Calls the HAL closeEndpointSession API.
-     *
-     * @param sessionId The session ID to close
-     * @param halReason The HAL reason
-     */
-    private void halCloseEndpointSession(int sessionId, byte halReason) throws RemoteException {
-        try {
-            mHubInterface.closeEndpointSession(sessionId, halReason);
-        } catch (RemoteException | IllegalArgumentException | UnsupportedOperationException e) {
-            throw e;
-        }
-    }
-
-    /** Same as halCloseEndpointSession but does not throw the exception */
-    private void halCloseEndpointSessionNoThrow(int sessionId, byte halReason) {
-        try {
-            halCloseEndpointSession(sessionId, halReason);
-        } catch (RemoteException | IllegalArgumentException | UnsupportedOperationException e) {
-            Log.e(TAG, "Exception while calling HAL closeEndpointSession", e);
-        }
-    }
-
-    /**
      * Cleans up resources related to a session with the provided ID.
      *
      * @param sessionId The session ID to clean up resources for
diff --git a/services/core/java/com/android/server/location/contexthub/ContextHubEndpointManager.java b/services/core/java/com/android/server/location/contexthub/ContextHubEndpointManager.java
index 8ab581e1fb7a..e1561599517d 100644
--- a/services/core/java/com/android/server/location/contexthub/ContextHubEndpointManager.java
+++ b/services/core/java/com/android/server/location/contexthub/ContextHubEndpointManager.java
@@ -29,6 +29,7 @@ import android.hardware.contexthub.IContextHubEndpoint;
 import android.hardware.contexthub.IContextHubEndpointCallback;
 import android.hardware.contexthub.IEndpointCommunication;
 import android.hardware.contexthub.MessageDeliveryStatus;
+import android.hardware.contexthub.Reason;
 import android.os.RemoteException;
 import android.os.ServiceSpecificException;
 import android.util.Log;
@@ -42,6 +43,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Consumer;
@@ -316,6 +318,11 @@ import java.util.function.Consumer;
         }
     }
 
+    /** Returns if a sessionId can be allocated for the service hub. */
+    private boolean isSessionIdAllocatedForService(int sessionId) {
+        return sessionId > mMaxSessionId || sessionId < mMinSessionId;
+    }
+
     /**
      * Unregisters an endpoint given its ID.
      *
@@ -337,8 +344,7 @@ import java.util.function.Consumer;
         }
     }
 
-    @Override
-    public void onEndpointSessionOpenRequest(
+    private Optional<Byte> onEndpointSessionOpenRequestInternal(
             int sessionId,
             HubEndpointInfo.HubEndpointIdentifier destination,
             HubEndpointInfo.HubEndpointIdentifier initiator,
@@ -348,7 +354,7 @@ import java.util.function.Consumer;
                     TAG,
                     "onEndpointSessionOpenRequest: invalid destination hub ID: "
                             + destination.getHub());
-            return;
+            return Optional.of(Reason.ENDPOINT_INVALID);
         }
         ContextHubEndpointBroker broker = mEndpointMap.get(destination.getEndpoint());
         if (broker == null) {
@@ -356,7 +362,7 @@ import java.util.function.Consumer;
                     TAG,
                     "onEndpointSessionOpenRequest: unknown destination endpoint ID: "
                             + destination.getEndpoint());
-            return;
+            return Optional.of(Reason.ENDPOINT_INVALID);
         }
         HubEndpointInfo initiatorInfo = mHubInfoRegistry.getEndpointInfo(initiator);
         if (initiatorInfo == null) {
@@ -364,9 +370,29 @@ import java.util.function.Consumer;
                     TAG,
                     "onEndpointSessionOpenRequest: unknown initiator endpoint ID: "
                             + initiator.getEndpoint());
-            return;
+            return Optional.of(Reason.ENDPOINT_INVALID);
+        }
+        if (!isSessionIdAllocatedForService(sessionId)) {
+            Log.e(
+                    TAG,
+                    "onEndpointSessionOpenRequest: invalid session ID, rejected:"
+                            + " sessionId="
+                            + sessionId);
+            return Optional.of(Reason.OPEN_ENDPOINT_SESSION_REQUEST_REJECTED);
         }
-        broker.onEndpointSessionOpenRequest(sessionId, initiatorInfo, serviceDescriptor);
+        return broker.onEndpointSessionOpenRequest(sessionId, initiatorInfo, serviceDescriptor);
+    }
+
+    @Override
+    public void onEndpointSessionOpenRequest(
+            int sessionId,
+            HubEndpointInfo.HubEndpointIdentifier destination,
+            HubEndpointInfo.HubEndpointIdentifier initiator,
+            String serviceDescriptor) {
+        Optional<Byte> errorOptional =
+                onEndpointSessionOpenRequestInternal(
+                        sessionId, destination, initiator, serviceDescriptor);
+        errorOptional.ifPresent((error) -> halCloseEndpointSessionNoThrow(sessionId, error));
     }
 
     @Override
@@ -418,6 +444,30 @@ import java.util.function.Consumer;
         }
     }
 
+    /**
+     * Calls the HAL closeEndpointSession API.
+     *
+     * @param sessionId The session ID to close
+     * @param halReason The HAL reason
+     */
+    /* package */ void halCloseEndpointSession(int sessionId, byte halReason)
+            throws RemoteException {
+        try {
+            mHubInterface.closeEndpointSession(sessionId, halReason);
+        } catch (RemoteException | IllegalArgumentException | UnsupportedOperationException e) {
+            throw e;
+        }
+    }
+
+    /** Same as halCloseEndpointSession but does not throw the exception */
+    /* package */ void halCloseEndpointSessionNoThrow(int sessionId, byte halReason) {
+        try {
+            halCloseEndpointSession(sessionId, halReason);
+        } catch (RemoteException | IllegalArgumentException | UnsupportedOperationException e) {
+            Log.e(TAG, "Exception while calling HAL closeEndpointSession", e);
+        }
+    }
+
     @Override
     public String toString() {
         StringBuilder sb = new StringBuilder();
diff --git a/services/tests/servicestests/src/com/android/server/location/contexthub/ContextHubEndpointTest.java b/services/tests/servicestests/src/com/android/server/location/contexthub/ContextHubEndpointTest.java
index 565a9b6c1c44..6c9160603be6 100644
--- a/services/tests/servicestests/src/com/android/server/location/contexthub/ContextHubEndpointTest.java
+++ b/services/tests/servicestests/src/com/android/server/location/contexthub/ContextHubEndpointTest.java
@@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assertThat;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -67,12 +68,15 @@ public class ContextHubEndpointTest {
     private static final int SESSION_ID_RANGE = ContextHubEndpointManager.SERVICE_SESSION_RANGE;
     private static final int MIN_SESSION_ID = 0;
     private static final int MAX_SESSION_ID = MIN_SESSION_ID + SESSION_ID_RANGE - 1;
+    private static final int SESSION_ID_FOR_OPEN_REQUEST = MAX_SESSION_ID + 1;
+    private static final int INVALID_SESSION_ID_FOR_OPEN_REQUEST = MIN_SESSION_ID + 1;
 
     private static final String ENDPOINT_NAME = "Example test endpoint";
     private static final int ENDPOINT_ID = 1;
     private static final String ENDPOINT_PACKAGE_NAME = "com.android.server.location.contexthub";
 
     private static final String TARGET_ENDPOINT_NAME = "Example target endpoint";
+    private static final String ENDPOINT_SERVICE_DESCRIPTOR = "serviceDescriptor";
     private static final int TARGET_ENDPOINT_ID = 1;
 
     private static final int SAMPLE_MESSAGE_TYPE = 1234;
@@ -221,6 +225,105 @@ public class ContextHubEndpointTest {
     }
 
     @Test
+    public void testEndpointSessionOpenRequest() throws RemoteException {
+        assertThat(mEndpointManager.getNumAvailableSessions()).isEqualTo(SESSION_ID_RANGE);
+        IContextHubEndpoint endpoint = registerExampleEndpoint();
+
+        HubEndpointInfo targetInfo =
+                new HubEndpointInfo(
+                        TARGET_ENDPOINT_NAME,
+                        TARGET_ENDPOINT_ID,
+                        ENDPOINT_PACKAGE_NAME,
+                        Collections.emptyList());
+        mHubInfoRegistry.onEndpointStarted(new HubEndpointInfo[] {targetInfo});
+        mEndpointManager.onEndpointSessionOpenRequest(
+                SESSION_ID_FOR_OPEN_REQUEST,
+                endpoint.getAssignedHubEndpointInfo().getIdentifier(),
+                targetInfo.getIdentifier(),
+                ENDPOINT_SERVICE_DESCRIPTOR);
+
+        verify(mMockCallback)
+                .onSessionOpenRequest(
+                        SESSION_ID_FOR_OPEN_REQUEST, targetInfo, ENDPOINT_SERVICE_DESCRIPTOR);
+
+        // Accept
+        endpoint.openSessionRequestComplete(SESSION_ID_FOR_OPEN_REQUEST);
+        verify(mMockEndpointCommunications)
+                .endpointSessionOpenComplete(SESSION_ID_FOR_OPEN_REQUEST);
+
+        unregisterExampleEndpoint(endpoint);
+    }
+
+    @Test
+    public void testEndpointSessionOpenRequestWithInvalidSessionId() throws RemoteException {
+        assertThat(mEndpointManager.getNumAvailableSessions()).isEqualTo(SESSION_ID_RANGE);
+        IContextHubEndpoint endpoint = registerExampleEndpoint();
+
+        HubEndpointInfo targetInfo =
+                new HubEndpointInfo(
+                        TARGET_ENDPOINT_NAME,
+                        TARGET_ENDPOINT_ID,
+                        ENDPOINT_PACKAGE_NAME,
+                        Collections.emptyList());
+        mHubInfoRegistry.onEndpointStarted(new HubEndpointInfo[] {targetInfo});
+        mEndpointManager.onEndpointSessionOpenRequest(
+                INVALID_SESSION_ID_FOR_OPEN_REQUEST,
+                endpoint.getAssignedHubEndpointInfo().getIdentifier(),
+                targetInfo.getIdentifier(),
+                ENDPOINT_SERVICE_DESCRIPTOR);
+        verify(mMockEndpointCommunications)
+                .closeEndpointSession(
+                        INVALID_SESSION_ID_FOR_OPEN_REQUEST,
+                        Reason.OPEN_ENDPOINT_SESSION_REQUEST_REJECTED);
+        verify(mMockCallback, never())
+                .onSessionOpenRequest(
+                        INVALID_SESSION_ID_FOR_OPEN_REQUEST,
+                        targetInfo,
+                        ENDPOINT_SERVICE_DESCRIPTOR);
+
+        unregisterExampleEndpoint(endpoint);
+    }
+
+    @Test
+    public void testEndpointSessionOpenRequest_duplicatedSessionId_noopWhenSessionIsActive()
+            throws RemoteException {
+        assertThat(mEndpointManager.getNumAvailableSessions()).isEqualTo(SESSION_ID_RANGE);
+        IContextHubEndpoint endpoint = registerExampleEndpoint();
+
+        HubEndpointInfo targetInfo =
+                new HubEndpointInfo(
+                        TARGET_ENDPOINT_NAME,
+                        TARGET_ENDPOINT_ID,
+                        ENDPOINT_PACKAGE_NAME,
+                        Collections.emptyList());
+        mHubInfoRegistry.onEndpointStarted(new HubEndpointInfo[] {targetInfo});
+        mEndpointManager.onEndpointSessionOpenRequest(
+                SESSION_ID_FOR_OPEN_REQUEST,
+                endpoint.getAssignedHubEndpointInfo().getIdentifier(),
+                targetInfo.getIdentifier(),
+                ENDPOINT_SERVICE_DESCRIPTOR);
+        endpoint.openSessionRequestComplete(SESSION_ID_FOR_OPEN_REQUEST);
+        // Now session with id SESSION_ID_FOR_OPEN_REQUEST is active
+
+        // Duplicated session open request
+        mEndpointManager.onEndpointSessionOpenRequest(
+                SESSION_ID_FOR_OPEN_REQUEST,
+                endpoint.getAssignedHubEndpointInfo().getIdentifier(),
+                targetInfo.getIdentifier(),
+                ENDPOINT_SERVICE_DESCRIPTOR);
+
+        // Client API is only invoked once
+        verify(mMockCallback, times(1))
+                .onSessionOpenRequest(
+                        SESSION_ID_FOR_OPEN_REQUEST, targetInfo, ENDPOINT_SERVICE_DESCRIPTOR);
+        // HAL still receives two open complete notifications
+        verify(mMockEndpointCommunications, times(2))
+                .endpointSessionOpenComplete(SESSION_ID_FOR_OPEN_REQUEST);
+
+        unregisterExampleEndpoint(endpoint);
+    }
+
+    @Test
     public void testMessageTransaction() throws RemoteException {
         IContextHubEndpoint endpoint = registerExampleEndpoint();
         testMessageTransactionInternal(endpoint, /* deliverMessageStatus= */ true);