Merge "Detach CDM transport upon encountering an secure channel error" into udc-dev

services/companion/java/com/android/server/companion/securechannel/SecureChannel.java

@@ -130,6 +130,7 @@ public class SecureChannel {
         if (DEBUG) {
             Slog.d(TAG, "Starting secure channel.");
         }
+        mStopped = false;
         new Thread(() -> {
             try {
                 // 1. Wait for the next handshake message and process it.
@@ -184,6 +185,17 @@ public class SecureChannel {
         KeyStoreUtils.cleanUp(mAlias);
     }
 
+    /**
+     * Return true if the channel is currently inactive.
+     * The channel could have been stopped by either {@link SecureChannel#stop()} or by
+     * encountering a fatal error.
+     *
+     * @return true if the channel is currently inactive.
+     */
+    public boolean isStopped() {
+        return mStopped;
+    }
+
     /**
      * Start exchanging handshakes to create a secure layer asynchronously. When the handshake is
      * completed successfully, then the {@link Callback#onSecureConnection()} will trigger. Any
@@ -290,6 +302,7 @@ public class SecureChannel {
             try {
                 data = new byte[length];
             } catch (OutOfMemoryError error) {
+                Streams.skipByReading(mInput, Long.MAX_VALUE);
                 throw new SecureChannelException("Payload is too large.", error);
             }
 

services/companion/java/com/android/server/companion/transport/CompanionTransportManager.java

@@ -137,13 +137,7 @@ public class CompanionTransportManager {
         synchronized (mTransports) {
             for (int i = 0; i < associationIds.length; i++) {
                 if (mTransports.contains(associationIds[i])) {
-                    try {
+                    mTransports.get(associationIds[i]).requestForResponse(message, data);
-                        mTransports.get(associationIds[i]).sendMessage(message, data);
-                    } catch (IOException e) {
-                        Slog.e(TAG, "Failed to send message 0x" + Integer.toHexString(message)
-                                + " data length " + data.length + " to association "
-                                + associationIds[i]);
-                    }
                 }
             }
         }
@@ -244,6 +238,7 @@ public class CompanionTransportManager {
         }
 
         addMessageListenersToTransport(transport);
+        transport.setOnTransportClosedListener(this::detachSystemDataTransport);
         transport.start();
         synchronized (mTransports) {
             mTransports.put(associationId, transport);
@@ -321,4 +316,14 @@ public class CompanionTransportManager {
             transport.addListener(mMessageListeners.keyAt(i), mMessageListeners.valueAt(i));
         }
     }
+
+    void detachSystemDataTransport(Transport transport) {
+        int associationId = transport.mAssociationId;
+        AssociationInfo association = mAssociationStore.getAssociationById(associationId);
+        if (association != null) {
+            detachSystemDataTransport(association.getPackageName(),
+                    association.getUserId(),
+                    association.getId());
+        }
+    }
 }

services/companion/java/com/android/server/companion/transport/RawTransport.java

@@ -70,6 +70,8 @@ class RawTransport extends Transport {
         }
         IoUtils.closeQuietly(mRemoteIn);
         IoUtils.closeQuietly(mRemoteOut);
+
+        super.close();
     }
 
     @Override

services/companion/java/com/android/server/companion/transport/SecureTransport.java

@@ -21,7 +21,6 @@ import android.content.Context;
 import android.os.ParcelFileDescriptor;
 import android.util.Slog;
 
-import com.android.internal.annotations.GuardedBy;
 import com.android.server.companion.securechannel.AttestationVerifier;
 import com.android.server.companion.securechannel.SecureChannel;
 
@@ -35,7 +34,6 @@ class SecureTransport extends Transport implements SecureChannel.Callback {
 
     private volatile boolean mShouldProcessRequests = false;
 
-    @GuardedBy("mRequestQueue")
     private final BlockingQueue<byte[]> mRequestQueue = new ArrayBlockingQueue<>(100);
 
     SecureTransport(int associationId, ParcelFileDescriptor fd, Context context) {
@@ -64,6 +62,8 @@ class SecureTransport extends Transport implements SecureChannel.Callback {
     void close() {
         mSecureChannel.close();
         mShouldProcessRequests = false;
+
+        super.close();
     }
 
     @Override
@@ -81,13 +81,19 @@ class SecureTransport extends Transport implements SecureChannel.Callback {
         }
 
         // Queue up a message to send
-        synchronized (mRequestQueue) {
+        try {
             mRequestQueue.add(ByteBuffer.allocate(HEADER_LENGTH + data.length)
                     .putInt(message)
                     .putInt(sequence)
                     .putInt(data.length)
                     .put(data)
                     .array());
+        } catch (IllegalStateException e) {
+            // Request buffer can only be full if too many requests are being added or
+            // the request processing thread is dead. Assume latter and detach the transport.
+            Slog.w(TAG, "Failed to queue message 0x" + Integer.toHexString(message)
+                    + " . Request buffer is full; detaching transport.", e);
+            close();
         }
     }
 
@@ -96,8 +102,8 @@ class SecureTransport extends Transport implements SecureChannel.Callback {
         try {
             mSecureChannel.establishSecureConnection();
         } catch (Exception e) {
-            Slog.w(TAG, "Failed to initiate secure channel handshake.", e);
+            Slog.e(TAG, "Failed to initiate secure channel handshake.", e);
-            onError(e);
+            close();
         }
     }
 
@@ -108,17 +114,14 @@ class SecureTransport extends Transport implements SecureChannel.Callback {
 
         // TODO: find a better way to handle incoming requests than a dedicated thread.
         new Thread(() -> {
-            try {
+            while (mShouldProcessRequests) {
-                while (mShouldProcessRequests) {
+                try {
-                    synchronized (mRequestQueue) {
+                    byte[] request = mRequestQueue.take();
-                        byte[] request = mRequestQueue.poll();
+                    mSecureChannel.sendSecureMessage(request);
-                        if (request != null) {
+                } catch (Exception e) {
-                            mSecureChannel.sendSecureMessage(request);
+                    Slog.e(TAG, "Failed to send secure message.", e);
-                        }
+                    close();
-                    }
                 }
-            } catch (IOException e) {
-                onError(e);
             }
         }).start();
     }
@@ -135,13 +138,18 @@ class SecureTransport extends Transport implements SecureChannel.Callback {
         try {
             handleMessage(message, sequence, content);
         } catch (IOException error) {
-            onError(error);
+            // IOException won't be thrown here because a separate thread is handling
+            // the write operations inside onSecureConnection().
         }
     }
 
     @Override
     public void onError(Throwable error) {
-        mShouldProcessRequests = false;
+        Slog.e(TAG, "Secure transport encountered an error.", error);
-        Slog.e(TAG, error.getMessage(), error);
+
+        // If the channel was stopped as a result of the error, then detach itself.
+        if (mSecureChannel.isStopped()) {
+            close();
+        }
     }
 }

services/companion/java/com/android/server/companion/transport/Transport.java

@@ -70,6 +70,8 @@ public abstract class Transport {
      */
     private final Map<Integer, IOnMessageReceivedListener> mListeners;
 
+    private OnTransportClosedListener mOnTransportClosed;
+
     private static boolean isRequest(int message) {
         return (message & 0xFF000000) == 0x63000000;
     }
@@ -120,20 +122,18 @@ public abstract class Transport {
     abstract void stop();
 
     /**
-     * Stop listening to the incoming data and close the streams.
+     * Stop listening to the incoming data and close the streams. If a listener for closed event
+     * is set, then trigger it to assist with its clean-up.
      */
-    abstract void close();
+    void close() {
+        if (mOnTransportClosed != null) {
+            mOnTransportClosed.onClosed(this);
+        }
+    }
 
     protected abstract void sendMessage(int message, int sequence, @NonNull byte[] data)
             throws IOException;
 
-    /**
-     * Send a message.
-     */
-    public void sendMessage(int message, @NonNull byte[] data) throws IOException {
-        sendMessage(message, mNextSequence.incrementAndGet(), data);
-    }
-
     public Future<byte[]> requestForResponse(int message, byte[] data) {
         if (DEBUG) Slog.d(TAG, "Requesting for response");
         final int sequence = mNextSequence.incrementAndGet();
@@ -247,4 +247,14 @@ public abstract class Transport {
             }
         }
     }
+
+    void setOnTransportClosedListener(OnTransportClosedListener callback) {
+        this.mOnTransportClosed = callback;
+    }
+
+    // Interface to pass transport to the transport manager to assist with detachment.
+    @FunctionalInterface
+    interface OnTransportClosedListener {
+        void onClosed(Transport transport);
+    }
 }