OSDN Git Service

Don't use transport binder with the lock held
authorBernardo Rufino <brufino@google.com>
Thu, 18 Jan 2018 15:10:41 +0000 (15:10 +0000)
committerBernardo Rufino <brufino@google.com>
Thu, 18 Jan 2018 19:20:49 +0000 (19:20 +0000)
There was a deadlock around the transport lock. We registered transports
with the transport lock held, this kicked-off transport onCreate()
synchronously, which called TransportManager
updateTransportAttributes(), which tried to acquire mentioned lock but
couldn't. This CL removes the lock for any call to the transport or
operation that triggers a call to the transport (it was
TransportClient.connect() or its variants).

Test: Load GMSCore before fix, boot, register transports, check no ANR
Test: m -j RunFrameworksServicesRoboTests
Test: adb shell bmgr transport -c <transport>, being registered & not
Bug: 72147303
Change-Id: I72ca145d7fb73c0ef29c4aa1b620fea4969481db

services/backup/java/com/android/server/backup/TransportManager.java
services/robotests/src/com/android/server/backup/TransportManagerTest.java

index 30fd25a..5b901ee 100644 (file)
@@ -62,9 +62,17 @@ public class TransportManager {
     private final PackageManager mPackageManager;
     private final Set<ComponentName> mTransportWhitelist;
     private final TransportClientManager mTransportClientManager;
-    private final Object mTransportLock = new Object();
     private OnTransportRegisteredListener mOnTransportRegisteredListener = (c, n) -> {};
 
+    /**
+     * Lock for registered transports and currently selected transport.
+     *
+     * <p><b>Warning:</b> No calls to {@link IBackupTransport} or calls that result in transport
+     * code being executed such as {@link TransportClient#connect(String)}} and its variants should
+     * be made with this lock held, risk of deadlock.
+     */
+    private final Object mTransportLock = new Object();
+
     /** @see #getRegisteredTransportNames() */
     @GuardedBy("mTransportLock")
     private final Map<ComponentName, TransportDescription> mRegisteredTransportsDescriptionMap =
@@ -109,15 +117,16 @@ public class TransportManager {
 
     @WorkerThread
     void onPackageChanged(String packageName, String... components) {
+        // Unfortunately this can't be atomic because we risk a deadlock if
+        // registerTransportsFromPackage() is put inside the synchronized block
+        Set<ComponentName> transportComponents =
+                Stream.of(components)
+                        .map(component -> new ComponentName(packageName, component))
+                        .collect(Collectors.toSet());
         synchronized (mTransportLock) {
-            Set<ComponentName> transportComponents =
-                    Stream.of(components)
-                            .map(component -> new ComponentName(packageName, component))
-                            .collect(Collectors.toSet());
-
             mRegisteredTransportsDescriptionMap.keySet().removeIf(transportComponents::contains);
-            registerTransportsFromPackage(packageName, transportComponents::contains);
         }
+        registerTransportsFromPackage(packageName, transportComponents::contains);
     }
 
     /**
@@ -263,6 +272,9 @@ public class TransportManager {
      * This is called with an internal lock held, ensuring that the transport will remain registered
      * while {@code transportConsumer} is being executed. Don't do heavy operations in {@code
      * transportConsumer}.
+     *
+     * <p><b>Warning:</b> Do NOT make any calls to {@link IBackupTransport} or call any variants of
+     * {@link TransportClient#connect(String)} here, otherwise you risk deadlock.
      */
     public void forEachRegisteredTransport(Consumer<String> transportConsumer) {
         synchronized (mTransportLock) {
@@ -465,20 +477,27 @@ public class TransportManager {
      */
     @WorkerThread
     public int registerAndSelectTransport(ComponentName transportComponent) {
+        // If it's already registered we select and return
         synchronized (mTransportLock) {
-            if (!mRegisteredTransportsDescriptionMap.containsKey(transportComponent)) {
-                int result = registerTransport(transportComponent);
-                if (result != BackupManager.SUCCESS) {
-                    return result;
-                }
+            try {
+                selectTransport(getTransportName(transportComponent));
+                return BackupManager.SUCCESS;
+            } catch (TransportNotRegisteredException e) {
+                // Fall through and release lock
             }
+        }
 
+        // We can't call registerTransport() with the transport lock held
+        int result = registerTransport(transportComponent);
+        if (result != BackupManager.SUCCESS) {
+            return result;
+        }
+        synchronized (mTransportLock) {
             try {
                 selectTransport(getTransportName(transportComponent));
                 return BackupManager.SUCCESS;
             } catch (TransportNotRegisteredException e) {
-                // Shouldn't happen because we are holding the lock
-                Slog.wtf(TAG, "Transport unexpectedly not registered");
+                Slog.wtf(TAG, "Transport got unregistered");
                 return BackupManager.ERROR_TRANSPORT_UNAVAILABLE;
             }
         }
@@ -512,13 +531,11 @@ public class TransportManager {
         if (hosts == null) {
             return;
         }
-        synchronized (mTransportLock) {
-            for (ResolveInfo host : hosts) {
-                ComponentName transportComponent = host.serviceInfo.getComponentName();
-                if (transportComponentFilter.test(transportComponent)
-                        && isTransportTrusted(transportComponent)) {
-                    registerTransport(transportComponent);
-                }
+        for (ResolveInfo host : hosts) {
+            ComponentName transportComponent = host.serviceInfo.getComponentName();
+            if (transportComponentFilter.test(transportComponent)
+                    && isTransportTrusted(transportComponent)) {
+                registerTransport(transportComponent);
             }
         }
     }
@@ -547,22 +564,24 @@ public class TransportManager {
     /**
      * Tries to register transport represented by {@code transportComponent}.
      *
+     * <p><b>Warning:</b> Don't call this with the transport lock held.
+     *
      * @param transportComponent Host of the transport that we want to register.
      * @return One of {@link BackupManager#SUCCESS}, {@link BackupManager#ERROR_TRANSPORT_INVALID}
      *     or {@link BackupManager#ERROR_TRANSPORT_UNAVAILABLE}.
      */
     @WorkerThread
     private int registerTransport(ComponentName transportComponent) {
+        checkCanUseTransport();
+
         if (!isTransportTrusted(transportComponent)) {
             return BackupManager.ERROR_TRANSPORT_INVALID;
         }
 
         String transportString = transportComponent.flattenToShortString();
-
         String callerLogString = "TransportManager.registerTransport()";
         TransportClient transportClient =
                 mTransportClientManager.getTransportClient(transportComponent, callerLogString);
-
         final IBackupTransport transport;
         try {
             transport = transportClient.connectOrThrow(callerLogString);
@@ -593,20 +612,26 @@ public class TransportManager {
     /** If {@link RemoteException} is thrown the transport is guaranteed to not be registered. */
     private void registerTransport(ComponentName transportComponent, IBackupTransport transport)
             throws RemoteException {
+        checkCanUseTransport();
+
+        TransportDescription description =
+                new TransportDescription(
+                        transport.name(),
+                        transport.transportDirName(),
+                        transport.configurationIntent(),
+                        transport.currentDestinationString(),
+                        transport.dataManagementIntent(),
+                        transport.dataManagementLabel());
         synchronized (mTransportLock) {
-            String name = transport.name();
-            TransportDescription description =
-                    new TransportDescription(
-                            name,
-                            transport.transportDirName(),
-                            transport.configurationIntent(),
-                            transport.currentDestinationString(),
-                            transport.dataManagementIntent(),
-                            transport.dataManagementLabel());
             mRegisteredTransportsDescriptionMap.put(transportComponent, description);
         }
     }
 
+    private void checkCanUseTransport() {
+        Preconditions.checkState(
+                !Thread.holdsLock(mTransportLock), "Can't call transport with transport lock held");
+    }
+
     private static Predicate<ComponentName> fromPackageFilter(String packageName) {
         return transportComponent -> packageName.equals(transportComponent.getPackageName());
     }
index cf0bc23..6753d73 100644 (file)
@@ -19,9 +19,13 @@ package com.android.server.backup;
 import static com.android.server.backup.testing.TransportData.genericTransport;
 import static com.android.server.backup.testing.TransportTestUtils.mockTransport;
 import static com.android.server.backup.testing.TransportTestUtils.setUpTransportsForTransportManager;
-
 import static com.google.common.truth.Truth.assertThat;
-
+import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+import static java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toSet;
+import static java.util.stream.Stream.concat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.never;
@@ -31,23 +35,15 @@ import static org.mockito.Mockito.when;
 import static org.robolectric.shadow.api.Shadow.extract;
 import static org.testng.Assert.expectThrows;
 
-import static java.util.Arrays.asList;
-import static java.util.Collections.emptyList;
-import static java.util.Collections.singletonList;
-import static java.util.stream.Collectors.toList;
-import static java.util.stream.Collectors.toSet;
-import static java.util.stream.Stream.concat;
-
 import android.annotation.Nullable;
+import android.app.backup.BackupManager;
 import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
 import android.content.pm.ApplicationInfo;
 import android.content.pm.PackageInfo;
 import android.platform.test.annotations.Presubmit;
-
 import com.android.server.backup.testing.ShadowContextImplForBackup;
-import com.android.server.testing.shadows.FrameworkShadowPackageManager;
 import com.android.server.backup.testing.TransportData;
 import com.android.server.backup.testing.TransportTestUtils.TransportMock;
 import com.android.server.backup.transport.OnTransportRegisteredListener;
@@ -57,7 +53,12 @@ import com.android.server.backup.transport.TransportNotRegisteredException;
 import com.android.server.testing.FrameworkRobolectricTestRunner;
 import com.android.server.testing.SystemLoaderClasses;
 import com.android.server.testing.shadows.FrameworkShadowContextImpl;
-
+import com.android.server.testing.shadows.FrameworkShadowPackageManager;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Stream;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -68,12 +69,6 @@ import org.robolectric.RuntimeEnvironment;
 import org.robolectric.annotation.Config;
 import org.robolectric.shadows.ShadowPackageManager;
 
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Stream;
-
 @RunWith(FrameworkRobolectricTestRunner.class)
 @Config(
     manifest = Config.NONE,
@@ -308,6 +303,43 @@ public class TransportManagerTest {
     }
 
     @Test
+    public void testRegisterAndSelectTransport_whenTransportRegistered() throws Exception {
+        setUpPackage(PACKAGE_A, ApplicationInfo.PRIVATE_FLAG_PRIVILEGED);
+        setUpTransports(mTransportA1);
+        TransportManager transportManager = createTransportManager(null, mTransportA1);
+        transportManager.registerTransports();
+        ComponentName transportComponent = mTransportA1.getTransportComponent();
+
+        int result = transportManager.registerAndSelectTransport(transportComponent);
+
+        assertThat(result).isEqualTo(BackupManager.SUCCESS);
+        assertThat(transportManager.getRegisteredTransportComponents())
+                .asList()
+                .contains(transportComponent);
+        assertThat(transportManager.getCurrentTransportName())
+                .isEqualTo(mTransportA1.transportName);
+    }
+
+    @Test
+    public void testRegisterAndSelectTransport_whenTransportNotRegistered() throws Exception {
+        setUpPackage(PACKAGE_A, ApplicationInfo.PRIVATE_FLAG_PRIVILEGED);
+        setUpTransports(mTransportA1);
+        TransportManager transportManager = createTransportManager(null, mTransportA1);
+        ComponentName transportComponent = mTransportA1.getTransportComponent();
+
+        int result = transportManager.registerAndSelectTransport(transportComponent);
+
+        assertThat(result).isEqualTo(BackupManager.SUCCESS);
+        assertThat(transportManager.getRegisteredTransportComponents())
+                .asList()
+                .contains(transportComponent);
+        assertThat(transportManager.getTransportDirName(mTransportA1.transportName))
+                .isEqualTo(mTransportA1.transportDirName);
+        assertThat(transportManager.getCurrentTransportName())
+                .isEqualTo(mTransportA1.transportName);
+    }
+
+    @Test
     public void testGetCurrentTransportName_whenSelectTransportNotCalled_returnsDefaultTransport()
             throws Exception {
         setUpPackage(PACKAGE_A, ApplicationInfo.PRIVATE_FLAG_PRIVILEGED);