Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge "Don't use transport binder with the lock held"

Browse Source
This commit is contained in:
TreeHugger Robot
2018-01-18 23:04:06 +00:00
committed by Android (Google) Code Review
parent 4604c41114 070081bc4f
commit 8f09d6ee8f
2 changed files with 109 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

93
services/backup/java/com/android/server/backup/TransportManager.java
View 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) {
// Shouldn't happen because we are holding the lock
Slog.wtf(TAG, "Transport unexpectedly not registered");
// 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) {
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());
}

68
services/robotests/src/com/android/server/backup/TransportManagerTest.java
View 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,
@@ -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 {