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

Make ManagedServices more thread-safe

Browse Source 
Added @GuardedBy annotations to all collection fields (according to their existing usage) plus a few synchronized blocks that were missing. ErrorProne now reports zero "GuardedBy" issues on this class.

Also added a test for a specific case that was found in the wild.

Fixes: 216508294
Test: atest
Change-Id: Ie221e467fd6fc8b510bc9b22c15abc242653c4b3
This commit is contained in:
Matías Hernández
2023-04-18 17:43:05 +02:00
parent 844c5aec48
commit 9aa84aeffc
2 changed files with 116 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

111
services/core/java/com/android/server/notification/ManagedServices.java
View File

@@ -140,34 +140,39 @@ abstract public class ManagedServices {
* The services that have been bound by us. If the service is also connected, it will also
* be in {@link #mServices}.
*/
@GuardedBy("mMutex")
private final ArrayList<Pair<ComponentName, Integer>> mServicesBound = new ArrayList<>();
@GuardedBy("mMutex")
private final ArraySet<Pair<ComponentName, Integer>> mServicesRebinding = new ArraySet<>();
// we need these packages to be protected because classes that inherit from it need to see it
protected final Object mDefaultsLock = new Object();
@GuardedBy("mDefaultsLock")
protected final ArraySet<ComponentName> mDefaultComponents = new ArraySet<>();
@GuardedBy("mDefaultsLock")
protected final ArraySet<String> mDefaultPackages = new ArraySet<>();
// lists the component names of all enabled (and therefore potentially connected)
// app services for current profiles.
private ArraySet<ComponentName> mEnabledServicesForCurrentProfiles
= new ArraySet<>();
@GuardedBy("mMutex")
private final ArraySet<ComponentName> mEnabledServicesForCurrentProfiles = new ArraySet<>();
// Just the packages from mEnabledServicesForCurrentProfiles
private ArraySet<String> mEnabledServicesPackageNames = new ArraySet<>();
@GuardedBy("mMutex")
private final ArraySet<String> mEnabledServicesPackageNames = new ArraySet<>();
// Per user id, list of enabled packages that have nevertheless asked not to be run
private final android.util.SparseSetArray<ComponentName> mSnoozing =
new android.util.SparseSetArray<>();
@GuardedBy("mSnoozing")
private final SparseSetArray<ComponentName> mSnoozing = new SparseSetArray<>();
// List of approved packages or components (by user, then by primary/secondary) that are
// allowed to be bound as managed services. A package or component appearing in this list does
// not mean that we are currently bound to said package/component.
@GuardedBy("mApproved")
protected final ArrayMap<Integer, ArrayMap<Boolean, ArraySet<String>>> mApproved =
new ArrayMap<>();
// List of packages or components (by user) that are configured to be enabled/disabled
// explicitly by the user
@GuardedBy("mApproved")
protected ArrayMap<Integer, ArraySet<String>> mUserSetServices = new ArrayMap<>();
@GuardedBy("mApproved")
protected ArrayMap<Integer, Boolean> mIsUserChanged = new ArrayMap<>();
// True if approved services are stored in xml, not settings.
@@ -262,20 +267,18 @@ abstract public class ManagedServices {
@NonNull
ArrayMap<Boolean, ArrayList<ComponentName>> resetComponents(String packageName, int userId) {
// components that we want to enable
ArrayList<ComponentName> componentsToEnable =
new ArrayList<>(mDefaultComponents.size());
ArrayList<ComponentName> componentsToEnable;
// components that were removed
ArrayList<ComponentName> disabledComponents =
new ArrayList<>(mDefaultComponents.size());
ArrayList<ComponentName> disabledComponents;
// all components that are enabled now
ArraySet<ComponentName> enabledComponents =
new ArraySet<>(getAllowedComponents(userId));
ArraySet<ComponentName> enabledComponents = new ArraySet<>(getAllowedComponents(userId));
boolean changed = false;
synchronized (mDefaultsLock) {
componentsToEnable = new ArrayList<>(mDefaultComponents.size());
disabledComponents = new ArrayList<>(mDefaultComponents.size());
// record all components that are enabled but should not be by default
for (int i = 0; i < mDefaultComponents.size() && enabledComponents.size() > 0; i++) {
ComponentName currentDefault = mDefaultComponents.valueAt(i);
@@ -374,14 +377,14 @@ abstract public class ManagedServices {
}
}
pw.println(" All " + getCaption() + "s (" + mEnabledServicesForCurrentProfiles.size()
+ ") enabled for current profiles:");
for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
if (filter != null && !filter.matches(cmpt)) continue;
pw.println(" " + cmpt);
}
synchronized (mMutex) {
pw.println(" All " + getCaption() + "s (" + mEnabledServicesForCurrentProfiles.size()
+ ") enabled for current profiles:");
for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
if (filter != null && !filter.matches(cmpt)) continue;
pw.println(" " + cmpt);
}
pw.println(" Live " + getCaption() + "s (" + mServices.size() + "):");
for (ManagedServiceInfo info : mServices) {
if (filter != null && !filter.matches(info.component)) continue;
@@ -434,12 +437,12 @@ abstract public class ManagedServices {
}
}
for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
if (filter != null && !filter.matches(cmpt)) continue;
cmpt.dumpDebug(proto, ManagedServicesProto.ENABLED);
}
synchronized (mMutex) {
for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
if (filter != null && !filter.matches(cmpt)) continue;
cmpt.dumpDebug(proto, ManagedServicesProto.ENABLED);
}
for (ManagedServiceInfo info : mServices) {
if (filter != null && !filter.matches(info.component)) continue;
info.dumpDebug(proto, ManagedServicesProto.LIVE_SERVICES, this);
@@ -677,7 +680,9 @@ abstract public class ManagedServices {
if (isUserChanged == null) { //NLS
userSetComponent = TextUtils.emptyIfNull(userSetComponent);
} else { //NAS
mIsUserChanged.put(resolvedUserId, Boolean.valueOf(isUserChanged));
synchronized (mApproved) {
mIsUserChanged.put(resolvedUserId, Boolean.valueOf(isUserChanged));
}
userSetComponent = Boolean.valueOf(isUserChanged) ? approved : "";
}
} else {
@@ -688,7 +693,9 @@ abstract public class ManagedServices {
if (isUserChanged_Old != null && Boolean.valueOf(isUserChanged_Old)) {
//user_set = true
userSetComponent = approved;
mIsUserChanged.put(resolvedUserId, true);
synchronized (mApproved) {
mIsUserChanged.put(resolvedUserId, true);
}
needUpgradeUserset = false;
} else {
userSetComponent = "";
@@ -724,8 +731,11 @@ abstract public class ManagedServices {
}
void upgradeDefaultsXmlVersion() {
// check if any defaults are loaded
int defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
int defaultsSize;
synchronized (mDefaultsLock) {
// check if any defaults are loaded
defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
}
if (defaultsSize == 0) {
// load defaults from current allowed
if (this.mApprovalLevel == APPROVAL_BY_COMPONENT) {
@@ -741,8 +751,10 @@ abstract public class ManagedServices {
}
}
}
synchronized (mDefaultsLock) {
defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
}
// if no defaults are loaded, then load from config
defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
if (defaultsSize == 0) {
loadDefaultsFromConfig();
}
@@ -806,7 +818,9 @@ abstract public class ManagedServices {
}
protected boolean isComponentEnabledForPackage(String pkg) {
return mEnabledServicesPackageNames.contains(pkg);
synchronized (mMutex) {
return mEnabledServicesPackageNames.contains(pkg);
}
}
protected void setPackageOrComponentEnabled(String pkgOrComponent, int userId,
@@ -959,9 +973,13 @@ abstract public class ManagedServices {
}
public void onPackagesChanged(boolean removingPackage, String[] pkgList, int[] uidList) {
if (DEBUG) Slog.d(TAG, "onPackagesChanged removingPackage=" + removingPackage
+ " pkgList=" + (pkgList == null ? null : Arrays.asList(pkgList))
+ " mEnabledServicesPackageNames=" + mEnabledServicesPackageNames);
if (DEBUG) {
synchronized (mMutex) {
Slog.d(TAG, "onPackagesChanged removingPackage=" + removingPackage
+ " pkgList=" + (pkgList == null ? null : Arrays.asList(pkgList))
+ " mEnabledServicesPackageNames=" + mEnabledServicesPackageNames);
}
}
if (pkgList != null && (pkgList.length > 0)) {
boolean anyServicesInvolved = false;
@@ -975,7 +993,7 @@ abstract public class ManagedServices {
}
}
for (String pkgName : pkgList) {
if (mEnabledServicesPackageNames.contains(pkgName)) {
if (isComponentEnabledForPackage(pkgName)) {
anyServicesInvolved = true;
}
if (uidList != null && uidList.length > 0) {
@@ -1299,9 +1317,11 @@ abstract public class ManagedServices {
}
final Set<ComponentName> add = new HashSet<>(userComponents);
ArraySet<ComponentName> snoozed = mSnoozing.get(userId);
if (snoozed != null) {
add.removeAll(snoozed);
synchronized (mSnoozing) {
ArraySet<ComponentName> snoozed = mSnoozing.get(userId);
if (snoozed != null) {
add.removeAll(snoozed);
}
}
componentsToBind.put(userId, add);
@@ -1605,9 +1625,12 @@ abstract public class ManagedServices {
}
}
@VisibleForTesting
boolean isBound(ComponentName cn, int userId) {
final Pair<ComponentName, Integer> servicesBindingTag = Pair.create(cn, userId);
return mServicesBound.contains(servicesBindingTag);
synchronized (mMutex) {
return mServicesBound.contains(servicesBindingTag);
}
}
protected boolean isBoundOrRebinding(final ComponentName cn, final int userId) {
@@ -1833,7 +1856,9 @@ abstract public class ManagedServices {
public boolean isEnabledForCurrentProfiles() {
if (this.isSystem) return true;
if (this.connection == null) return false;
return mEnabledServicesForCurrentProfiles.contains(this.component);
synchronized (mMutex) {
return mEnabledServicesForCurrentProfiles.contains(this.component);
}
}
/**
@@ -1877,7 +1902,9 @@ abstract public class ManagedServices {
/** convenience method for looking in mEnabledServicesForCurrentProfiles */
public boolean isComponentEnabledForCurrentProfiles(ComponentName component) {
return mEnabledServicesForCurrentProfiles.contains(component);
synchronized (mMutex) {
return mEnabledServicesForCurrentProfiles.contains(component);
}
}
public static class UserProfiles {

47
services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java
View File

@@ -96,6 +96,7 @@ import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
public class ManagedServicesTest extends UiServiceTestCase {
@@ -1920,6 +1921,18 @@ public class ManagedServicesTest extends UiServiceTestCase {
assertTrue(service.isBound(cn_disallowed, 0));
}
@Test
public void isComponentEnabledForCurrentProfiles_isThreadSafe() throws InterruptedException {
for (UserInfo userInfo : mUm.getUsers()) {
mService.addApprovedList("pkg1/cmp1:pkg2/cmp2:pkg3/cmp3", userInfo.id, true);
}
testThreadSafety(() -> {
mService.rebindServices(false, 0);
assertThat(mService.isComponentEnabledForCurrentProfiles(
new ComponentName("pkg1", "cmp1"))).isTrue();
}, 20, 30);
}
private void mockServiceInfoWithMetaData(List<ComponentName> componentNames,
ManagedServices service, ArrayMap<ComponentName, Bundle> metaDatas)
throws RemoteException {
@@ -2298,4 +2311,38 @@ public class ManagedServicesTest extends UiServiceTestCase {
return false;
}
}
/**
* Helper method to test the thread safety of some operations.
*
* <p>Runs the supplied {@code operationToTest}, {@code nRunsPerThread} times,
* concurrently using {@code nThreads} threads, and waits for all of them to finish.
*/
private static void testThreadSafety(Runnable operationToTest, int nThreads,
int nRunsPerThread) throws InterruptedException {
final CountDownLatch startLatch = new CountDownLatch(1);
final CountDownLatch doneLatch = new CountDownLatch(nThreads);
for (int i = 0; i < nThreads; i++) {
Runnable threadRunnable = () -> {
try {
startLatch.await();
for (int j = 0; j < nRunsPerThread; j++) {
operationToTest.run();
}
} catch (InterruptedException e) {
e.printStackTrace();
} finally {
doneLatch.countDown();
}
};
new Thread(threadRunnable, "Test Thread #" + i).start();
}
// Ready set go
startLatch.countDown();
// Wait for all test threads to be done.
doneLatch.await();
}
}