Guard against incorrect ID assignment in CdmService

Do not assume that all previously assigned association IDs were assigned
correctly in regard to ranges of associations ID allocated to each user
when generating an association ID for a new association in
CompanionDeviceManagerService.getNewAssociationIdForPackage().

Also Slog.e() when detect such errors in existing Associations as well
as when detect ID conflicts in AssociationStoreImpl.addAssociation().

Bug: 224736262
Bug: 223995714
Test: atest CtsCompanionDeviceManagerCoreTestCases
Change-Id: Ibe14cfda9718b3e473f0f157bbfef2f7051819c2
This commit is contained in:
Sergey Nikolaienkov
2022-04-08 13:47:41 +02:00
parent 65d4e08971
commit 8228658b69
3 changed files with 31 additions and 4 deletions

View File

@@ -23,6 +23,7 @@ import android.annotation.UserIdInt;
import android.companion.AssociationInfo; import android.companion.AssociationInfo;
import android.net.MacAddress; import android.net.MacAddress;
import android.util.Log; import android.util.Log;
import android.util.Slog;
import android.util.SparseArray; import android.util.SparseArray;
import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.GuardedBy;
@@ -80,7 +81,7 @@ class AssociationStoreImpl implements AssociationStore {
synchronized (mLock) { synchronized (mLock) {
if (mIdMap.containsKey(id)) { if (mIdMap.containsKey(id)) {
if (DEBUG) Log.w(TAG, "Association already stored."); Slog.e(TAG, "Association with id " + id + " already exists.");
return; return;
} }
mIdMap.put(id, association); mIdMap.put(id, association);

View File

@@ -813,7 +813,13 @@ public class CompanionDeviceManagerService extends SystemService {
synchronized (mPreviouslyUsedIds) { synchronized (mPreviouslyUsedIds) {
// First: collect all IDs currently in use for this user's Associations. // First: collect all IDs currently in use for this user's Associations.
final SparseBooleanArray usedIds = new SparseBooleanArray(); final SparseBooleanArray usedIds = new SparseBooleanArray();
for (AssociationInfo it : mAssociationStore.getAssociationsForUser(userId)) {
// We should really only be checking associations for the given user (i.e.:
// mAssociationStore.getAssociationsForUser(userId)), BUT in the past we've got in a
// state where association IDs were not assigned correctly in regard to
// user-to-association-ids-range (e.g. associations with IDs from 1 to 100,000 should
// always belong to u0), so let's check all the associations.
for (AssociationInfo it : mAssociationStore.getAssociations()) {
usedIds.put(it.getId(), true); usedIds.put(it.getId(), true);
} }

View File

@@ -25,6 +25,8 @@ import static com.android.internal.util.XmlUtils.writeBooleanAttribute;
import static com.android.internal.util.XmlUtils.writeIntAttribute; import static com.android.internal.util.XmlUtils.writeIntAttribute;
import static com.android.internal.util.XmlUtils.writeLongAttribute; import static com.android.internal.util.XmlUtils.writeLongAttribute;
import static com.android.internal.util.XmlUtils.writeStringAttribute; import static com.android.internal.util.XmlUtils.writeStringAttribute;
import static com.android.server.companion.CompanionDeviceManagerService.getFirstAssociationIdForUser;
import static com.android.server.companion.CompanionDeviceManagerService.getLastAssociationIdForUser;
import static com.android.server.companion.DataStoreUtils.createStorageFileForUser; import static com.android.server.companion.DataStoreUtils.createStorageFileForUser;
import static com.android.server.companion.DataStoreUtils.isEndOfTag; import static com.android.server.companion.DataStoreUtils.isEndOfTag;
import static com.android.server.companion.DataStoreUtils.isStartOfTag; import static com.android.server.companion.DataStoreUtils.isStartOfTag;
@@ -194,7 +196,25 @@ final class PersistentDataStore {
// Associations for all users are stored in a single "flat" set: so we read directly // Associations for all users are stored in a single "flat" set: so we read directly
// into it. // into it.
readStateForUser(userId, allAssociationsOut, previouslyUsedIds); final Set<AssociationInfo> associationsForUser = new HashSet<>();
readStateForUser(userId, associationsForUser, previouslyUsedIds);
// Go through all the associations for the user and check if their IDs are within
// the allowed range (for the user).
final int firstAllowedId = getFirstAssociationIdForUser(userId);
final int lastAllowedId = getLastAssociationIdForUser(userId);
for (AssociationInfo association : associationsForUser) {
final int id = association.getId();
if (id < firstAllowedId || id > lastAllowedId) {
Slog.e(TAG, "Wrong association ID assignment: " + id + ". "
+ "Association belongs to u" + userId + " and thus its ID should be "
+ "within [" + firstAllowedId + ", " + lastAllowedId + "] range.");
// TODO(b/224736262): try fixing (re-assigning) the ID?
}
}
// Add user's association to the "output" set.
allAssociationsOut.addAll(associationsForUser);
// Save previously used IDs for this user into the "out" structure. // Save previously used IDs for this user into the "out" structure.
previouslyUsedIdsPerUserOut.append(userId, previouslyUsedIds); previouslyUsedIdsPerUserOut.append(userId, previouslyUsedIds);
@@ -369,7 +389,7 @@ final class PersistentDataStore {
// existing ones from the backup files. And the fact that we are reading from a V0 file, // existing ones from the backup files. And the fact that we are reading from a V0 file,
// means that CDM hasn't assigned any IDs yet, so we can just start from the first available // means that CDM hasn't assigned any IDs yet, so we can just start from the first available
// id for each user (eg. 1 for user 0; 100 001 - for user 1; 200 001 - for user 2; etc). // id for each user (eg. 1 for user 0; 100 001 - for user 1; 200 001 - for user 2; etc).
int associationId = CompanionDeviceManagerService.getFirstAssociationIdForUser(userId); int associationId = getFirstAssociationIdForUser(userId);
while (true) { while (true) {
parser.nextTag(); parser.nextTag();
if (isEndOfTag(parser, XML_TAG_ASSOCIATIONS)) break; if (isEndOfTag(parser, XML_TAG_ASSOCIATIONS)) break;