From 7a907dbe46fd15b808bddaf2d191b32555047869 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Fri, 7 Aug 2020 09:50:28 -0700 Subject: [PATCH] Refactored user switch workflow to decouple UserInfo from TargetUser. UserController calls SystemServiceManager on some stages of the user switch lifecycle, and SystemServiceManager calls the respective callbacks in the SystemService objects it manage. Prior to Android 11, these callbacks would take a @UserIdInt userid, but now they take a TargetUser (which in turns contains a Userinfo). So, currently, UserController calls SystemServiceManager passing the userid, and SystemServiceManager gets the UserInfo from UserManagerService. That works fine most of the time, but there might be some race conditions when a user is removed that can crash the system when such user is stopped (because UserManagerService doesn't have its UserInfo anymore). This CL fixes this issue by refactoring the user switch workflow so: - TargetUser doesn't hold a reference to the UserInfo anymore, but offer the methods needed by SystemService implementations. - SystemServiceManager keeps track of the TargetUser objects used in the callbacks. Test: manual verification Fixes: 159831354 Change-Id: If68ab1b5e247c7389c8cac16432d8b8809507e86 --- .../autofill/AutofillManagerService.java | 2 +- .../ContentCaptureManagerService.java | 2 +- .../java/android/os/UserManagerInternal.java | 3 +- .../com/android/server/SystemService.java | 73 +++++++++++++++---- .../android/server/SystemServiceManager.java | 50 ++++++++++--- .../com/android/server/am/UserController.java | 4 +- .../android/server/pm/UserManagerService.java | 19 ++++- .../server/usage/UsageStatsService.java | 15 ++-- .../VoiceInteractionManagerService.java | 10 +-- 9 files changed, 131 insertions(+), 47 deletions(-) diff --git a/services/autofill/java/com/android/server/autofill/AutofillManagerService.java b/services/autofill/java/com/android/server/autofill/AutofillManagerService.java index 663fd6259fd44..ad85784ca066c 100644 --- a/services/autofill/java/com/android/server/autofill/AutofillManagerService.java +++ b/services/autofill/java/com/android/server/autofill/AutofillManagerService.java @@ -360,7 +360,7 @@ public final class AutofillManagerService @Override // from SystemService public boolean isUserSupported(TargetUser user) { - return user.getUserInfo().isFull() || user.getUserInfo().isManagedProfile(); + return user.isFull() || user.isManagedProfile(); } @Override // from SystemService diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java index ea94ad0b3c202..e742015916e7d 100644 --- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java +++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java @@ -219,7 +219,7 @@ public final class ContentCaptureManagerService extends @Override // from SystemService public boolean isUserSupported(TargetUser user) { - return user.getUserInfo().isFull() || user.getUserInfo().isManagedProfile(); + return user.isFull() || user.isManagedProfile(); } @Override // from SystemService diff --git a/services/core/java/android/os/UserManagerInternal.java b/services/core/java/android/os/UserManagerInternal.java index fbe8c04bd59cd..61e8128bd5106 100644 --- a/services/core/java/android/os/UserManagerInternal.java +++ b/services/core/java/android/os/UserManagerInternal.java @@ -262,8 +262,7 @@ public abstract class UserManagerInternal { public abstract boolean hasUserRestriction(String restriction, int userId); /** - * Gets an {@link UserInfo} for the given {@code userId}, or {@code null} if not - * found. + * Gets a {@link UserInfo} for the given {@code userId}, or {@code null} if not found. */ public abstract @Nullable UserInfo getUserInfo(@UserIdInt int userId); diff --git a/services/core/java/com/android/server/SystemService.java b/services/core/java/com/android/server/SystemService.java index 1496e926b95bc..84d01ec3598de 100644 --- a/services/core/java/com/android/server/SystemService.java +++ b/services/core/java/com/android/server/SystemService.java @@ -23,6 +23,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.SystemApi; import android.annotation.SystemApi.Client; +import android.annotation.UserIdInt; import android.app.ActivityThread; import android.content.Context; import android.content.pm.UserInfo; @@ -131,45 +132,89 @@ public abstract class SystemService { */ @SystemApi(client = Client.SYSTEM_SERVER) public static final class TargetUser { - @NonNull - private final UserInfo mUserInfo; + + // NOTE: attributes below must be immutable while ther user is running (i.e., from the + // moment it's started until after it's shutdown). + private final @UserIdInt int mUserId; + private final boolean mFull; + private final boolean mManagedProfile; + private final boolean mPreCreated; /** @hide */ public TargetUser(@NonNull UserInfo userInfo) { - mUserInfo = userInfo; + mUserId = userInfo.id; + mFull = userInfo.isFull(); + mManagedProfile = userInfo.isManagedProfile(); + mPreCreated = userInfo.preCreated; } /** - * @return The information about the user. NOTE: this is a "live" object - * referenced by {@link UserManagerService} and hence should not be modified. + * Checks if the target user is {@link UserInfo#isFull() full}. * * @hide */ - @NonNull - public UserInfo getUserInfo() { - return mUserInfo; + public boolean isFull() { + return mFull; } /** - * @return the target {@link UserHandle}. + * Checks if the target user is a managed profile. + * + * @hide + */ + public boolean isManagedProfile() { + return mManagedProfile; + } + + /** + * Checks if the target user is a pre-created user. + * + * @hide + */ + public boolean isPreCreated() { + return mPreCreated; + } + + /** + * Gets the target user's {@link UserHandle}. */ @NonNull public UserHandle getUserHandle() { - return mUserInfo.getUserHandle(); + return UserHandle.of(mUserId); } /** - * @return the integer user id + * Gets the target user's id. * * @hide */ - public int getUserIdentifier() { - return mUserInfo.id; + public @UserIdInt int getUserIdentifier() { + return mUserId; } @Override public String toString() { - return Integer.toString(getUserIdentifier()); + return Integer.toString(mUserId); + } + + /** + * @hide + */ + public void dump(@NonNull StringBuilder builder) { + builder.append(getUserIdentifier()); + + if (!isFull() && !isManagedProfile()) return; + + builder.append('('); + boolean addComma = false; + if (isFull()) { + builder.append("full"); + } + if (isManagedProfile()) { + if (addComma) builder.append(','); + builder.append("mp"); + } + builder.append(')'); } } diff --git a/services/core/java/com/android/server/SystemServiceManager.java b/services/core/java/com/android/server/SystemServiceManager.java index 74bb7d7e90f18..df966c1ea6ac4 100644 --- a/services/core/java/com/android/server/SystemServiceManager.java +++ b/services/core/java/com/android/server/SystemServiceManager.java @@ -27,7 +27,10 @@ import android.os.UserHandle; import android.os.UserManagerInternal; import android.util.ArrayMap; import android.util.Slog; +import android.util.SparseArray; +import com.android.internal.annotations.GuardedBy; +import com.android.internal.util.Preconditions; import com.android.server.SystemService.TargetUser; import com.android.server.utils.TimingsTraceAndSlog; @@ -74,6 +77,13 @@ public class SystemServiceManager { private UserManagerInternal mUserManagerInternal; + /** + * Map of started {@link TargetUser TargetUsers} by user id; users are added on start and + * removed after they're completely shut down. + */ + @GuardedBy("mTargetUsers") + private final SparseArray mTargetUsers = new SparseArray<>(); + SystemServiceManager(Context context) { mContext = context; } @@ -240,21 +250,26 @@ public class SystemServiceManager { mUserManagerInternal = LocalServices.getService(UserManagerInternal.class); } - private @NonNull UserInfo getUserInfo(@UserIdInt int userHandle) { - if (mUserManagerInternal == null) { - throw new IllegalStateException("mUserManagerInternal not set yet"); + private @NonNull TargetUser getTargetUser(@UserIdInt int userHandle) { + final TargetUser targetUser; + synchronized (mTargetUsers) { + targetUser = mTargetUsers.get(userHandle); } - final UserInfo userInfo = mUserManagerInternal.getUserInfo(userHandle); - if (userInfo == null) { - throw new IllegalStateException("No UserInfo for " + userHandle); - } - return userInfo; + Preconditions.checkState(targetUser != null, "No TargetUser for " + userHandle); + return targetUser; } /** * Starts the given user. */ public void startUser(final @NonNull TimingsTraceAndSlog t, final @UserIdInt int userHandle) { + // Create cached TargetUser + final UserInfo userInfo = mUserManagerInternal.getUserInfo(userHandle); + Preconditions.checkState(userInfo != null, "No UserInfo for " + userHandle); + synchronized (mTargetUsers) { + mTargetUsers.put(userHandle, new TargetUser(userInfo)); + } + onUser(t, START, userHandle); } @@ -291,6 +306,11 @@ public class SystemServiceManager { */ public void cleanupUser(final @UserIdInt int userHandle) { onUser(CLEANUP, userHandle); + + // Remove cached TargetUser + synchronized (mTargetUsers) { + mTargetUsers.remove(userHandle); + } } private void onUser(@NonNull String onWhat, @UserIdInt int userHandle) { @@ -306,9 +326,9 @@ public class SystemServiceManager { @UserIdInt int curUserId, @UserIdInt int prevUserId) { t.traceBegin("ssm." + onWhat + "User-" + curUserId); Slog.i(TAG, "Calling on" + onWhat + "User " + curUserId); - final TargetUser curUser = new TargetUser(getUserInfo(curUserId)); + final TargetUser curUser = getTargetUser(curUserId); final TargetUser prevUser = prevUserId == UserHandle.USER_NULL ? null - : new TargetUser(getUserInfo(prevUserId)); + : getTargetUser(prevUserId); final int serviceLen = mServices.size(); for (int i = 0; i < serviceLen; i++) { final SystemService service = mServices.get(i); @@ -437,7 +457,7 @@ public class SystemServiceManager { */ public void dump() { StringBuilder builder = new StringBuilder(); - builder.append("Current phase: ").append(mCurrentPhase).append("\n"); + builder.append("Current phase: ").append(mCurrentPhase).append('\n'); builder.append("Services:\n"); final int startedLen = mServices.size(); for (int i = 0; i < startedLen; i++) { @@ -447,6 +467,14 @@ public class SystemServiceManager { .append("\n"); } + builder.append("Target users: "); + final int targetUsersSize = mTargetUsers.size(); + for (int i = 0; i < targetUsersSize; i++) { + mTargetUsers.valueAt(i).dump(builder); + if (i != targetUsersSize - 1) builder.append(','); + } + builder.append('\n'); + Slog.e(TAG, builder.toString()); } } diff --git a/services/core/java/com/android/server/am/UserController.java b/services/core/java/com/android/server/am/UserController.java index 7420e0a6e8283..83bf28e2d3bae 100644 --- a/services/core/java/com/android/server/am/UserController.java +++ b/services/core/java/com/android/server/am/UserController.java @@ -988,6 +988,8 @@ class UserController implements Handler.Callback { final ArrayList stopCallbacks; final ArrayList keyEvictedCallbacks; int userIdToLock = userId; + // Must get a reference to UserInfo before it's removed + final UserInfo userInfo = getUserInfo(userId); synchronized (mLock) { stopCallbacks = new ArrayList<>(uss.mStopCallbacks); keyEvictedCallbacks = new ArrayList<>(uss.mKeyEvictedCallbacks); @@ -1030,8 +1032,8 @@ class UserController implements Handler.Callback { if (stopped) { mInjector.systemServiceManagerCleanupUser(userId); mInjector.stackSupervisorRemoveUser(userId); + // Remove the user if it is ephemeral. - UserInfo userInfo = getUserInfo(userId); if (userInfo.isEphemeral() && !userInfo.preCreated) { mInjector.getUserManager().removeUserEvenWhenDisallowed(userId); } diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index f66b4ee5a1a73..d137fd05f793f 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -499,20 +499,23 @@ public class UserManagerService extends IUserManager.Stub { states = new SparseIntArray(); invalidateIsUserUnlockedCache(); } - public int get(int userId) { + public int get(@UserIdInt int userId) { return states.get(userId); } - public int get(int userId, int fallback) { + public int get(@UserIdInt int userId, int fallback) { return states.indexOfKey(userId) >= 0 ? states.get(userId) : fallback; } - public void put(int userId, int state) { + public void put(@UserIdInt int userId, int state) { states.put(userId, state); invalidateIsUserUnlockedCache(); } - public void delete(int userId) { + public void delete(@UserIdInt int userId) { states.delete(userId); invalidateIsUserUnlockedCache(); } + public boolean has(@UserIdInt int userId) { + return states.get(userId, UserHandle.USER_NULL) != UserHandle.USER_NULL; + } @Override public String toString() { return states.toString(); @@ -3561,6 +3564,13 @@ public class UserManagerService extends IUserManager.Stub { if (preCreatedUserData == null) { return null; } + synchronized (mUserStates) { + if (mUserStates.has(preCreatedUserData.info.id)) { + Slog.w(LOG_TAG, "Cannot reuse pre-created user " + + preCreatedUserData.info.id + " because it didn't stop yet"); + return null; + } + } final UserInfo preCreatedUser = preCreatedUserData.info; final int newFlags = preCreatedUser.flags | flags; if (!checkUserTypeConsistency(newFlags)) { @@ -5225,6 +5235,7 @@ public class UserManagerService extends IUserManager.Stub { return userData == null ? null : userData.info; } + @Override public @NonNull UserInfo[] getUserInfos() { synchronized (mUsersLock) { int userSize = mUsers.size(); diff --git a/services/usage/java/com/android/server/usage/UsageStatsService.java b/services/usage/java/com/android/server/usage/UsageStatsService.java index 78556ef41edb0..1e5d92b270d21 100644 --- a/services/usage/java/com/android/server/usage/UsageStatsService.java +++ b/services/usage/java/com/android/server/usage/UsageStatsService.java @@ -61,7 +61,6 @@ import android.content.pm.PackageManager; import android.content.pm.PackageManagerInternal; import android.content.pm.ParceledListSlice; import android.content.pm.ShortcutServiceInternal; -import android.content.pm.UserInfo; import android.content.res.Configuration; import android.os.Binder; import android.os.Environment; @@ -306,25 +305,25 @@ public class UsageStatsService extends SystemService implements @Override public void onUserStopping(@NonNull TargetUser user) { - final UserInfo userInfo = user.getUserInfo(); + final int userId = user.getUserIdentifier(); synchronized (mLock) { // User was started but never unlocked so no need to report a user stopped event - if (!mUserUnlockedStates.get(userInfo.id)) { - persistPendingEventsLocked(userInfo.id); + if (!mUserUnlockedStates.get(userId)) { + persistPendingEventsLocked(userId); return; } // Report a user stopped event before persisting all stats to disk via the user service final Event event = new Event(USER_STOPPED, SystemClock.elapsedRealtime()); event.mPackage = Event.DEVICE_EVENT_PACKAGE_NAME; - reportEvent(event, userInfo.id); - final UserUsageStatsService userService = mUserState.get(userInfo.id); + reportEvent(event, userId); + final UserUsageStatsService userService = mUserState.get(userId); if (userService != null) { userService.userStopped(); } - mUserUnlockedStates.put(userInfo.id, false); - mUserState.put(userInfo.id, null); // release the service (mainly for GC) + mUserUnlockedStates.put(userId, false); + mUserState.put(userId, null); // release the service (mainly for GC) } } diff --git a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java index 9ff44eba8fa0e..a2215ceed36af 100644 --- a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java +++ b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java @@ -164,13 +164,13 @@ public class VoiceInteractionManagerService extends SystemService { } } - private boolean isSupported(UserInfo user) { + @Override + public boolean isUserSupported(@NonNull TargetUser user) { return user.isFull(); } - @Override - public boolean isUserSupported(TargetUser user) { - return isSupported(user.getUserInfo()); + private boolean isUserSupported(@NonNull UserInfo user) { + return user.isFull(); } @Override @@ -461,7 +461,7 @@ public class VoiceInteractionManagerService extends SystemService { private void setCurrentUserLocked(@UserIdInt int userHandle) { mCurUser = userHandle; final UserInfo userInfo = mUserManagerInternal.getUserInfo(mCurUser); - mCurUserSupported = isSupported(userInfo); + mCurUserSupported = isUserSupported(userInfo); } public void switchUser(@UserIdInt int userHandle) {