From 8e34e6352cce537669b366d456e1450979929d22 Mon Sep 17 00:00:00 2001 From: Maurice Lam Date: Fri, 11 Feb 2022 22:14:43 +0000 Subject: [PATCH] Fix secondary user test for VirtualDevice - Clear binder identity before calling getPackageUid - VirtualDeviceImpl uses the owner's user ID to create its Context - For virtual camera, create a different CameraAccessController for each user Fixes: 218790107 Test: (On secondary user) atest CtsVirtualDevicesTestCases Change-Id: I63027ab58854f11faa4e2b20da202886dd10d2f3 --- .../virtual/CameraAccessController.java | 39 +++++++++++----- .../companion/virtual/PermissionUtils.java | 16 ++++--- .../companion/virtual/VirtualDeviceImpl.java | 3 +- .../virtual/VirtualDeviceManagerService.java | 45 ++++++++++++++----- .../tests/servicestests/AndroidManifest.xml | 2 + .../VirtualDeviceManagerServiceTest.java | 2 + 6 files changed, 80 insertions(+), 27 deletions(-) diff --git a/services/companion/java/com/android/server/companion/virtual/CameraAccessController.java b/services/companion/java/com/android/server/companion/virtual/CameraAccessController.java index 2c42c910b9ab2..adc8459de6588 100644 --- a/services/companion/java/com/android/server/companion/virtual/CameraAccessController.java +++ b/services/companion/java/com/android/server/companion/virtual/CameraAccessController.java @@ -33,17 +33,19 @@ import com.android.internal.annotations.GuardedBy; /** * Handles blocking access to the camera for apps running on virtual devices. */ -class CameraAccessController extends CameraManager.AvailabilityCallback { +class CameraAccessController extends CameraManager.AvailabilityCallback implements AutoCloseable { private static final String TAG = "CameraAccessController"; private final Object mLock = new Object(); private final Context mContext; - private VirtualDeviceManagerInternal mVirtualDeviceManagerInternal; - CameraAccessBlockedCallback mBlockedCallback; - private CameraManager mCameraManager; - private boolean mListeningForCameraEvents; - private PackageManager mPackageManager; + private final VirtualDeviceManagerInternal mVirtualDeviceManagerInternal; + private final CameraAccessBlockedCallback mBlockedCallback; + private final CameraManager mCameraManager; + private final PackageManager mPackageManager; + + @GuardedBy("mLock") + private int mObserverCount = 0; @GuardedBy("mLock") private ArrayMap mPackageToSessionData = new ArrayMap<>(); @@ -77,23 +79,38 @@ class CameraAccessController extends CameraManager.AvailabilityCallback { */ public void startObservingIfNeeded() { synchronized (mLock) { - if (!mListeningForCameraEvents) { + if (mObserverCount == 0) { mCameraManager.registerAvailabilityCallback(mContext.getMainExecutor(), this); - mListeningForCameraEvents = true; } + mObserverCount++; } } /** * Stop watching for camera access. */ - public void stopObserving() { + public void stopObservingIfNeeded() { synchronized (mLock) { - mCameraManager.unregisterAvailabilityCallback(this); - mListeningForCameraEvents = false; + mObserverCount--; + if (mObserverCount <= 0) { + close(); + } } } + + @Override + public void close() { + synchronized (mLock) { + if (mObserverCount < 0) { + Slog.wtf(TAG, "Unexpected negative mObserverCount: " + mObserverCount); + } else if (mObserverCount > 0) { + Slog.w(TAG, "Unexpected close with observers remaining: " + mObserverCount); + } + } + mCameraManager.unregisterAvailabilityCallback(this); + } + @Override public void onCameraOpened(@NonNull String cameraId, @NonNull String packageName) { synchronized (mLock) { diff --git a/services/companion/java/com/android/server/companion/virtual/PermissionUtils.java b/services/companion/java/com/android/server/companion/virtual/PermissionUtils.java index c397ea2997a4f..77b880f6e62f0 100644 --- a/services/companion/java/com/android/server/companion/virtual/PermissionUtils.java +++ b/services/companion/java/com/android/server/companion/virtual/PermissionUtils.java @@ -18,6 +18,8 @@ package com.android.server.companion.virtual; import android.content.Context; import android.content.pm.PackageManager; +import android.os.Binder; +import android.os.UserHandle; import android.util.Slog; /** @@ -32,13 +34,15 @@ class PermissionUtils { * * @param context the context * @param callingPackage the calling application package name - * @param callingUid the calling application uid - * @return {@code true} if the package name matches the calling app uid, {@code false} otherwise + * @return {@code true} if the package name matches {@link Binder#getCallingUid()}, or + * {@code false} otherwise */ - public static boolean validatePackageName(Context context, String callingPackage, - int callingUid) { + public static boolean validateCallingPackageName(Context context, String callingPackage) { + final int callingUid = Binder.getCallingUid(); + final long token = Binder.clearCallingIdentity(); try { - int packageUid = context.getPackageManager().getPackageUid(callingPackage, 0); + int packageUid = context.getPackageManager() + .getPackageUidAsUser(callingPackage, UserHandle.getUserId(callingUid)); if (packageUid != callingUid) { Slog.e(LOG_TAG, "validatePackageName: App with package name " + callingPackage + " is UID " + packageUid + " but caller is " + callingUid); @@ -48,6 +52,8 @@ class PermissionUtils { Slog.e(LOG_TAG, "validatePackageName: App with package name " + callingPackage + " does not exist"); return false; + } finally { + Binder.restoreCallingIdentity(token); } return true; } diff --git a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java index c0a904fe3d9a6..e248f678d028b 100644 --- a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java +++ b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java @@ -140,7 +140,8 @@ final class VirtualDeviceImpl extends IVirtualDevice.Stub int ownerUid, InputController inputController, OnDeviceCloseListener listener, PendingTrampolineCallback pendingTrampolineCallback, IVirtualDeviceActivityListener activityListener, VirtualDeviceParams params) { - mContext = context; + UserHandle ownerUserHandle = UserHandle.getUserHandleForUid(ownerUid); + mContext = context.createContextAsUser(ownerUserHandle, 0); mAssociationInfo = associationInfo; mPendingTrampolineCallback = pendingTrampolineCallback; mActivityListener = activityListener; diff --git a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java index c7d8daac606b5..9f252d744144d 100644 --- a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java +++ b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java @@ -36,6 +36,7 @@ import android.os.IBinder; import android.os.Looper; import android.os.Parcel; import android.os.RemoteException; +import android.os.UserHandle; import android.util.ExceptionUtils; import android.util.Slog; import android.util.SparseArray; @@ -66,7 +67,12 @@ public class VirtualDeviceManagerService extends SystemService { private VirtualDeviceManagerInternal mLocalService; private final Handler mHandler = new Handler(Looper.getMainLooper()); private final PendingTrampolineMap mPendingTrampolines = new PendingTrampolineMap(mHandler); - private final CameraAccessController mCameraAccessController; + /** + * Mapping from user IDs to CameraAccessControllers. + */ + @GuardedBy("mVirtualDeviceManagerLock") + private final SparseArray mCameraAccessControllers = + new SparseArray<>(); /** * Mapping from CDM association IDs to virtual devices. Only one virtual device is allowed for @@ -94,8 +100,6 @@ public class VirtualDeviceManagerService extends SystemService { super(context); mImpl = new VirtualDeviceManagerImpl(); mLocalService = new LocalService(); - mCameraAccessController = new CameraAccessController(getContext(), mLocalService, - this::onCameraAccessBlocked); } private final ActivityInterceptorCallback mActivityInterceptorCallback = @@ -144,16 +148,19 @@ public class VirtualDeviceManagerService extends SystemService { @Override public void onUserStarting(@NonNull TargetUser user) { super.onUserStarting(user); + Context userContext = getContext().createContextAsUser(user.getUserHandle(), 0); synchronized (mVirtualDeviceManagerLock) { - final CompanionDeviceManager cdm = getContext() - .createContextAsUser(user.getUserHandle(), 0) - .getSystemService(CompanionDeviceManager.class); + final CompanionDeviceManager cdm = + userContext.getSystemService(CompanionDeviceManager.class); final int userId = user.getUserIdentifier(); mAllAssociations.put(userId, cdm.getAllAssociations()); OnAssociationsChangedListener listener = associations -> mAllAssociations.put(userId, associations); mOnAssociationsChangedListeners.put(userId, listener); cdm.addOnAssociationsChangedListener(Runnable::run, listener); + CameraAccessController cameraAccessController = new CameraAccessController( + userContext, mLocalService, this::onCameraAccessBlocked); + mCameraAccessControllers.put(user.getUserIdentifier(), cameraAccessController); } } @@ -171,6 +178,14 @@ public class VirtualDeviceManagerService extends SystemService { cdm.removeOnAssociationsChangedListener(listener); mOnAssociationsChangedListeners.remove(userId); } + CameraAccessController cameraAccessController = mCameraAccessControllers.get( + user.getUserIdentifier()); + if (cameraAccessController != null) { + cameraAccessController.close(); + mCameraAccessControllers.remove(user.getUserIdentifier()); + } else { + Slog.w(TAG, "Cannot unregister cameraAccessController for user " + user); + } } } @@ -198,7 +213,7 @@ public class VirtualDeviceManagerService extends SystemService { android.Manifest.permission.CREATE_VIRTUAL_DEVICE, "createVirtualDevice"); final int callingUid = getCallingUid(); - if (!PermissionUtils.validatePackageName(getContext(), packageName, callingUid)) { + if (!PermissionUtils.validateCallingPackageName(getContext(), packageName)) { throw new SecurityException( "Package name " + packageName + " does not belong to calling uid " + callingUid); @@ -213,6 +228,9 @@ public class VirtualDeviceManagerService extends SystemService { "Virtual device for association ID " + associationId + " already exists"); } + final int userId = UserHandle.getUserId(callingUid); + final CameraAccessController cameraAccessController = + mCameraAccessControllers.get(userId); VirtualDeviceImpl virtualDevice = new VirtualDeviceImpl(getContext(), associationInfo, token, callingUid, new VirtualDeviceImpl.OnDeviceCloseListener() { @@ -220,14 +238,21 @@ public class VirtualDeviceManagerService extends SystemService { public void onClose(int associationId) { synchronized (mVirtualDeviceManagerLock) { mVirtualDevices.remove(associationId); - if (mVirtualDevices.size() == 0) { - mCameraAccessController.stopObserving(); + if (cameraAccessController != null) { + cameraAccessController.stopObservingIfNeeded(); + } else { + Slog.w(TAG, "cameraAccessController not found for user " + + userId); } } } }, this, activityListener, params); - mCameraAccessController.startObservingIfNeeded(); + if (cameraAccessController != null) { + cameraAccessController.startObservingIfNeeded(); + } else { + Slog.w(TAG, "cameraAccessController not found for user " + userId); + } mVirtualDevices.put(associationInfo.getId(), virtualDevice); return virtualDevice; } diff --git a/services/tests/servicestests/AndroidManifest.xml b/services/tests/servicestests/AndroidManifest.xml index 53cab9ed80cf5..a6194df18217c 100644 --- a/services/tests/servicestests/AndroidManifest.xml +++ b/services/tests/servicestests/AndroidManifest.xml @@ -98,6 +98,8 @@ + + diff --git a/services/tests/servicestests/src/com/android/server/companion/virtual/VirtualDeviceManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/companion/virtual/VirtualDeviceManagerServiceTest.java index 9aac81c34edb9..7b921ababf36b 100644 --- a/services/tests/servicestests/src/com/android/server/companion/virtual/VirtualDeviceManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/companion/virtual/VirtualDeviceManagerServiceTest.java @@ -58,6 +58,7 @@ import android.os.IBinder; import android.os.IPowerManager; import android.os.IThermalService; import android.os.PowerManager; +import android.os.Process; import android.os.RemoteException; import android.os.WorkSource; import android.platform.test.annotations.Presubmit; @@ -136,6 +137,7 @@ public class VirtualDeviceManagerServiceTest { LocalServices.addService(DisplayManagerInternal.class, mDisplayManagerInternalMock); mContext = Mockito.spy(new ContextWrapper(InstrumentationRegistry.getTargetContext())); + doReturn(mContext).when(mContext).createContextAsUser(eq(Process.myUserHandle()), anyInt()); doNothing().when(mContext).enforceCallingOrSelfPermission( eq(Manifest.permission.CREATE_VIRTUAL_DEVICE), anyString()); when(mContext.getSystemService(Context.DEVICE_POLICY_SERVICE)).thenReturn(