Fix NPE when serializing roles.xml and add nullability annotations.
This change fixes the NPE when serializing roles.xml if package hash
is null, and added the missing nullability annotations in role manager
service to make things clear and let the IDE warn.
Bug: 110557011
Test: manual
Change-Id: I27b5942ff6091414c527d7d337d7d01218e88fb0
diff --git a/services/core/java/com/android/server/role/RoleManagerService.java b/services/core/java/com/android/server/role/RoleManagerService.java
index b065470..b5ad235 100644
--- a/services/core/java/com/android/server/role/RoleManagerService.java
+++ b/services/core/java/com/android/server/role/RoleManagerService.java
@@ -55,6 +55,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.Objects;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
@@ -106,7 +107,7 @@
intentFilter.addAction(Intent.ACTION_USER_REMOVED);
getContext().registerReceiverAsUser(new BroadcastReceiver() {
@Override
- public void onReceive(Context context, Intent intent) {
+ public void onReceive(@NonNull Context context, @NonNull Intent intent) {
if (TextUtils.equals(intent.getAction(), Intent.ACTION_USER_REMOVED)) {
int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, 0);
onRemoveUser(userId);
@@ -129,10 +130,11 @@
userState = getUserStateLocked(userId);
}
String packagesHash = computeComponentStateHash(userId);
- boolean needGrant;
+ String lastGrantPackagesHash;
synchronized (mLock) {
- needGrant = !packagesHash.equals(userState.getLastGrantPackagesHashLocked());
+ lastGrantPackagesHash = userState.getLastGrantPackagesHashLocked();
}
+ boolean needGrant = !Objects.equals(packagesHash, lastGrantPackagesHash);
if (needGrant) {
// Some vital packages state has changed since last role grant
// Run grants again
@@ -144,7 +146,6 @@
public void onSuccess() {
result.complete(null);
}
-
@Override
public void onFailure() {
result.completeExceptionally(new RuntimeException());
@@ -163,7 +164,8 @@
}
}
- private String computeComponentStateHash(int userId) {
+ @Nullable
+ private String computeComponentStateHash(@UserIdInt int userId) {
PackageManagerInternal pm = LocalServices.getService(PackageManagerInternal.class);
ByteArrayOutputStream out = new ByteArrayOutputStream();
@@ -198,8 +200,7 @@
private RoleUserState getUserStateLocked(@UserIdInt int userId) {
RoleUserState userState = mUserStates.get(userId);
if (userState == null) {
- userState = new RoleUserState(userId);
- userState.readSyncLocked();
+ userState = RoleUserState.newInstanceLocked(userId);
mUserStates.put(userId, userState);
}
return userState;
@@ -386,11 +387,11 @@
}
@Override
- public void onShellCommand(FileDescriptor in, FileDescriptor out,
- FileDescriptor err, String[] args, ShellCallback callback,
- ResultReceiver resultReceiver) {
- (new RoleManagerShellCommand(this)).exec(
- this, in, out, err, args, callback, resultReceiver);
+ public void onShellCommand(@Nullable FileDescriptor in, @Nullable FileDescriptor out,
+ @Nullable FileDescriptor err, @NonNull String[] args,
+ @Nullable ShellCallback callback, @NonNull ResultReceiver resultReceiver) {
+ new RoleManagerShellCommand(this).exec(this, in, out, err, args, callback,
+ resultReceiver);
}
}
}
diff --git a/services/core/java/com/android/server/role/RoleManagerShellCommand.java b/services/core/java/com/android/server/role/RoleManagerShellCommand.java
index e1977ef..336b311 100644
--- a/services/core/java/com/android/server/role/RoleManagerShellCommand.java
+++ b/services/core/java/com/android/server/role/RoleManagerShellCommand.java
@@ -16,6 +16,8 @@
package com.android.server.role;
+import android.annotation.NonNull;
+import android.annotation.Nullable;
import android.app.role.IRoleManager;
import android.app.role.IRoleManagerCallback;
import android.os.RemoteException;
@@ -27,13 +29,17 @@
import java.util.concurrent.TimeUnit;
class RoleManagerShellCommand extends ShellCommand {
+
+ @NonNull
private final IRoleManager mRoleManager;
- RoleManagerShellCommand(IRoleManager roleManager) {
+ RoleManagerShellCommand(@NonNull IRoleManager roleManager) {
mRoleManager = roleManager;
}
private class Callback extends IRoleManagerCallback.Stub {
+
+ @NonNull
private final CompletableFuture<Void> mResult = new CompletableFuture<>();
public int waitForResult() {
@@ -58,7 +64,7 @@
}
@Override
- public int onCommand(String cmd) {
+ public int onCommand(@Nullable String cmd) {
if (cmd == null) {
return handleDefaultCommands(cmd);
}
diff --git a/services/core/java/com/android/server/role/RoleUserState.java b/services/core/java/com/android/server/role/RoleUserState.java
index f218d3a..3e3e156 100644
--- a/services/core/java/com/android/server/role/RoleUserState.java
+++ b/services/core/java/com/android/server/role/RoleUserState.java
@@ -47,6 +47,7 @@
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.List;
+import java.util.Objects;
/**
* Stores the state of roles for a user.
@@ -70,25 +71,41 @@
private final int mUserId;
@GuardedBy("RoleManagerService.mLock")
- private int mVersion;
+ private int mVersion = VERSION_UNDEFINED;
@GuardedBy("RoleManagerService.mLock")
- private String mLastGrantPackagesHash = null;
+ @Nullable
+ private String mLastGrantPackagesHash;
/**
* Maps role names to its holders' package names. The values should never be null.
*/
@GuardedBy("RoleManagerService.mLock")
- @Nullable
- private ArrayMap<String, ArraySet<String>> mRoles = null;
+ @NonNull
+ private ArrayMap<String, ArraySet<String>> mRoles = new ArrayMap<>();
@GuardedBy("RoleManagerService.mLock")
private boolean mDestroyed;
+ @NonNull
private final Handler mWriteHandler = new Handler(BackgroundThread.getHandler().getLooper());
- public RoleUserState(@UserIdInt int userId) {
+ private RoleUserState(@UserIdInt int userId) {
mUserId = userId;
+
+ readSyncLocked();
+ }
+
+ /**
+ * Create a new instance of user state, and read its state from disk if previously persisted.
+ *
+ * @param userId the user id for the new user state
+ *
+ * @return the new user state
+ */
+ @GuardedBy("RoleManagerService.mLock")
+ public static RoleUserState newInstanceLocked(@UserIdInt int userId) {
+ return new RoleUserState(userId);
}
/**
@@ -116,7 +133,9 @@
}
/**
- * Get the hash representing the state of packages during the last time initial grants was run
+ * Get the hash representing the state of packages during the last time initial grants was run.
+ *
+ * @return the hash representing the state of packages
*/
@GuardedBy("RoleManagerService.mLock")
public String getLastGrantPackagesHashLocked() {
@@ -124,10 +143,16 @@
}
/**
- * Set the hash representing the state of packages during the last time initial grants was run
+ * Set the hash representing the state of packages during the last time initial grants was run.
+ *
+ * @param lastGrantPackagesHash the hash representing the state of packages
*/
@GuardedBy("RoleManagerService.mLock")
- public void setLastGrantPackagesHashLocked(String lastGrantPackagesHash) {
+ public void setLastGrantPackagesHashLocked(@Nullable String lastGrantPackagesHash) {
+ throwIfDestroyedLocked();
+ if (Objects.equals(mLastGrantPackagesHash, lastGrantPackagesHash)) {
+ return;
+ }
mLastGrantPackagesHash = lastGrantPackagesHash;
writeAsyncLocked();
}
@@ -250,9 +275,9 @@
* Schedule writing the state to file.
*/
@GuardedBy("RoleManagerService.mLock")
- void writeAsyncLocked() {
+ private void writeAsyncLocked() {
throwIfDestroyedLocked();
- int version = mVersion;
+
ArrayMap<String, ArraySet<String>> roles = new ArrayMap<>();
for (int i = 0, size = CollectionUtils.size(mRoles); i < size; ++i) {
String roleName = mRoles.keyAt(i);
@@ -260,15 +285,16 @@
roleHolders = new ArraySet<>(roleHolders);
roles.put(roleName, roleHolders);
}
+
mWriteHandler.removeCallbacksAndMessages(null);
// TODO: Throttle writes.
- mWriteHandler.sendMessage(PooledLambda.obtainMessage(
- RoleUserState::writeSync, this, version, roles, mLastGrantPackagesHash));
+ mWriteHandler.sendMessage(PooledLambda.obtainMessage(RoleUserState::writeSync, this,
+ mVersion, mLastGrantPackagesHash, roles));
}
@WorkerThread
- private void writeSync(int version, @NonNull ArrayMap<String, ArraySet<String>> roles,
- String packagesHash) {
+ private void writeSync(int version, @Nullable String packagesHash,
+ @NonNull ArrayMap<String, ArraySet<String>> roles) {
AtomicFile atomicFile = new AtomicFile(getFile(mUserId), "roles-" + mUserId);
FileOutputStream out = null;
try {
@@ -280,7 +306,7 @@
"http://xmlpull.org/v1/doc/features.html#indent-output", true);
serializer.startDocument(null, true);
- serializeRoles(serializer, version, roles, packagesHash);
+ serializeRoles(serializer, version, packagesHash, roles);
serializer.endDocument();
atomicFile.finishWrite(out);
@@ -296,19 +322,26 @@
@WorkerThread
private void serializeRoles(@NonNull XmlSerializer serializer, int version,
- @NonNull ArrayMap<String, ArraySet<String>> roles, String packagesHash)
+ @Nullable String packagesHash, @NonNull ArrayMap<String, ArraySet<String>> roles)
throws IOException {
serializer.startTag(null, TAG_ROLES);
+
serializer.attribute(null, ATTRIBUTE_VERSION, Integer.toString(version));
- serializer.attribute(null, ATTRIBUTE_PACKAGES_HASH, packagesHash);
+
+ if (packagesHash != null) {
+ serializer.attribute(null, ATTRIBUTE_PACKAGES_HASH, packagesHash);
+ }
+
for (int i = 0, size = roles.size(); i < size; ++i) {
String roleName = roles.keyAt(i);
ArraySet<String> roleHolders = roles.valueAt(i);
+
serializer.startTag(null, TAG_ROLE);
serializer.attribute(null, ATTRIBUTE_NAME, roleName);
serializeRoleHolders(serializer, roleHolders);
serializer.endTag(null, TAG_ROLE);
}
+
serializer.endTag(null, TAG_ROLES);
}
@@ -317,6 +350,7 @@
@NonNull ArraySet<String> roleHolders) throws IOException {
for (int i = 0, size = roleHolders.size(); i < size; ++i) {
String roleHolder = roleHolders.valueAt(i);
+
serializer.startTag(null, TAG_HOLDER);
serializer.attribute(null, ATTRIBUTE_NAME, roleHolder);
serializer.endTag(null, TAG_HOLDER);
@@ -327,11 +361,7 @@
* Read the state from file.
*/
@GuardedBy("RoleManagerService.mLock")
- public void readSyncLocked() {
- if (mRoles != null) {
- throw new IllegalStateException("This RoleUserState has already read the roles.xml");
- }
-
+ private void readSyncLocked() {
File file = getFile(mUserId);
try (FileInputStream in = new AtomicFile(file).openRead()) {
XmlPullParser parser = Xml.newPullParser();
@@ -339,8 +369,6 @@
parseXmlLocked(parser);
} catch (FileNotFoundException e) {
Slog.i(LOG_TAG, "roles.xml not found");
- mRoles = new ArrayMap<>();
- mVersion = VERSION_UNDEFINED;
} catch (XmlPullParserException | IOException e) {
throw new IllegalStateException("Failed to parse roles.xml: " + file, e);
}
@@ -362,13 +390,14 @@
return;
}
}
+ Slog.w(LOG_TAG, "Missing <" + TAG_ROLES + "> in roles.xml");
}
private void parseRolesLocked(@NonNull XmlPullParser parser) throws IOException,
XmlPullParserException {
mVersion = Integer.parseInt(parser.getAttributeValue(null, ATTRIBUTE_VERSION));
mLastGrantPackagesHash = parser.getAttributeValue(null, ATTRIBUTE_PACKAGES_HASH);
- mRoles = new ArrayMap<>();
+ mRoles.clear();
int type;
int depth;