Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8228658b authored by Sergey Nikolaienkov's avatar Sergey Nikolaienkov
Browse files

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
parent 65d4e089

services/companion/java/com/android/server/companion/AssociationStoreImpl.java

+2 −1
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ import android.annotation.UserIdInt; 
import android.companion.AssociationInfo;

import android.net.MacAddress;

import android.util.Log;

import android.util.Slog;

import android.util.SparseArray;



import com.android.internal.annotations.GuardedBy;
@@ -80,7 +81,7 @@ class AssociationStoreImpl implements AssociationStore { 


        synchronized (mLock) {

            if (mIdMap.containsKey(id)) {

                if (DEBUG) Log.w(TAG, "Association already stored.");

                Slog.e(TAG, "Association with id " + id + " already exists.");

                return;

            }

            mIdMap.put(id, association);

services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java

+7 −1
Original line number Diff line number Diff line
@@ -813,7 +813,13 @@ public class CompanionDeviceManagerService extends SystemService { 
        synchronized (mPreviouslyUsedIds) {

            // First: collect all IDs currently in use for this user's Associations.

            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);

            }

services/companion/java/com/android/server/companion/PersistentDataStore.java

+22 −2
Original line number Diff line number Diff line
@@ -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.writeLongAttribute;

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.isEndOfTag;

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

            // 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.

            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,

        // 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).

        int associationId = CompanionDeviceManagerService.getFirstAssociationIdForUser(userId);

        int associationId = getFirstAssociationIdForUser(userId);

        while (true) {

            parser.nextTag();

            if (isEndOfTag(parser, XML_TAG_ASSOCIATIONS)) break;