From 070081bc4f3ed1863a917a5c2ce6aa8b813e7649 Mon Sep 17 00:00:00 2001 From: Bernardo Rufino Date: Thu, 18 Jan 2018 15:10:41 +0000 Subject: Don't use transport binder with the lock held 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 , being registered & not Bug: 72147303 Change-Id: I72ca145d7fb73c0ef29c4aa1b620fea4969481db --- .../android/server/backup/TransportManager.java | 89 ++++++++++++++-------- .../server/backup/TransportManagerTest.java | 68 ++++++++++++----- 2 files changed, 107 insertions(+), 50 deletions(-) diff --git a/services/backup/java/com/android/server/backup/TransportManager.java b/services/backup/java/com/android/server/backup/TransportManager.java index 30fd25a92484..5b901ee2b3da 100644 --- a/services/backup/java/com/android/server/backup/TransportManager.java +++ b/services/backup/java/com/android/server/backup/TransportManager.java @@ -62,9 +62,17 @@ public class TransportManager { private final PackageManager mPackageManager; private final Set mTransportWhitelist; private final TransportClientManager mTransportClientManager; - private final Object mTransportLock = new Object(); private OnTransportRegisteredListener mOnTransportRegisteredListener = (c, n) -> {}; + /** + * Lock for registered transports and currently selected transport. + * + *

Warning: 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 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 transportComponents = + Stream.of(components) + .map(component -> new ComponentName(packageName, component)) + .collect(Collectors.toSet()); synchronized (mTransportLock) { - Set 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}. + * + *

Warning: 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 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}. * + *

Warning: 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 fromPackageFilter(String packageName) { return transportComponent -> packageName.equals(transportComponent.getPackageName()); } diff --git a/services/robotests/src/com/android/server/backup/TransportManagerTest.java b/services/robotests/src/com/android/server/backup/TransportManagerTest.java index cf0bc235dc10..6753d73590d3 100644 --- a/services/robotests/src/com/android/server/backup/TransportManagerTest.java +++ b/services/robotests/src/com/android/server/backup/TransportManagerTest.java @@ -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, @@ -307,6 +302,43 @@ public class TransportManagerTest { .onTransportRegistered(mTransportA2.transportName, mTransportA2.transportDirName); } + @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 { -- cgit v1.2.3-59-g8ed1b