From a84ad2b349d69c2e159361cca6445c55e06b5a59 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Wed, 24 Jun 2020 17:52:55 -0600 Subject: [PATCH] Slight relaxing of ContextUserIdChecker. Several managers keep an "int mUserId" field which is assigned from Context.getUserId(), so Binder calls referencing that field are okay. Also shift to borrowing the "flavor" logic for detecting userId parameters consistently. Bug: 115654727, 159626156 Test: atest error_prone_android_framework_test Change-Id: I9841fdf16f34c08b113e689e74b94f1ede839e2c --- .../android/ContextUserIdChecker.java | 42 +++++++++---------- .../bugpatterns/android/UidChecker.java | 8 ++-- .../android/ContextUserIdCheckerTest.java | 13 ++++++ .../tests/res/android/os/UserHandle.java | 4 ++ 4 files changed, 40 insertions(+), 27 deletions(-) diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java index 7f2cce6ea7f05..4f1af3e9bea2a 100644 --- a/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java @@ -17,6 +17,7 @@ package com.google.errorprone.bugpatterns.android; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.bugpatterns.android.UidChecker.getFlavor; import static com.google.errorprone.matchers.Matchers.enclosingClass; import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.instanceMethod; @@ -27,17 +28,17 @@ import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.android.UidChecker.Flavor; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol.VarSymbol; import java.util.List; -import java.util.Locale; -import java.util.function.Predicate; /** * To avoid an explosion of {@code startActivityForUser} style methods, we've @@ -62,39 +63,34 @@ public final class ContextUserIdChecker extends BugChecker implements MethodInvo private static final Matcher GET_USER_ID_CALL = methodInvocation( instanceMethod().onDescendantOf("android.content.Context").named("getUserId")); + private static final Matcher USER_ID_FIELD = new Matcher() { + @Override + public boolean matches(ExpressionTree tree, VisitorState state) { + if (tree instanceof IdentifierTree) { + return "mUserId".equals((((IdentifierTree) tree).getName().toString())); + } + return false; + } + }; + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (INSIDE_MANAGER.matches(tree, state) && BINDER_CALL.matches(tree, state)) { final List vars = ASTHelpers.getSymbol(tree).params(); for (int i = 0; i < vars.size(); i++) { - if (USER_ID_VAR.test(vars.get(i)) && - !GET_USER_ID_CALL.matches(tree.getArguments().get(i), state)) { + final Flavor varFlavor = getFlavor(vars.get(i)); + final ExpressionTree arg = tree.getArguments().get(i); + if (varFlavor == Flavor.USER_ID && + !GET_USER_ID_CALL.matches(arg, state) && + !USER_ID_FIELD.matches(arg, state)) { return buildDescription(tree) .setMessage("Must pass Context.getUserId() as user ID" - + "to enable createContextAsUser()") + + " to enable createContextAsUser()") .build(); } } } return Description.NO_MATCH; } - - private static final UserIdMatcher USER_ID_VAR = new UserIdMatcher(); - - private static class UserIdMatcher implements Predicate { - @Override - public boolean test(VarSymbol t) { - if ("int".equals(t.type.toString())) { - switch (t.name.toString().toLowerCase(Locale.ROOT)) { - case "user": - case "userid": - case "userhandle": - case "user_id": - return true; - } - } - return false; - } - } } diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.java index 533586f65c619..3ffb8ea407894 100644 --- a/errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.java +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.java @@ -63,7 +63,7 @@ public final class UidChecker extends BugChecker implements MethodInvocationTree return Description.NO_MATCH; } - private static enum Flavor { + public static enum Flavor { UNKNOWN(null), PID(Pattern.compile("(^pid$|Pid$)")), UID(Pattern.compile("(^uid$|Uid$)")), @@ -78,7 +78,7 @@ public final class UidChecker extends BugChecker implements MethodInvocationTree } } - private static Flavor getFlavor(String name) { + public static Flavor getFlavor(String name) { for (Flavor f : Flavor.values()) { if (f.matches(name)) { return f; @@ -87,7 +87,7 @@ public final class UidChecker extends BugChecker implements MethodInvocationTree return Flavor.UNKNOWN; } - private static Flavor getFlavor(VarSymbol symbol) { + public static Flavor getFlavor(VarSymbol symbol) { final String type = symbol.type.toString(); if ("int".equals(type)) { return getFlavor(symbol.name.toString()); @@ -95,7 +95,7 @@ public final class UidChecker extends BugChecker implements MethodInvocationTree return Flavor.UNKNOWN; } - private static Flavor getFlavor(ExpressionTree tree) { + public static Flavor getFlavor(ExpressionTree tree) { if (tree instanceof IdentifierTree) { return getFlavor(((IdentifierTree) tree).getName().toString()); } else if (tree instanceof MemberSelectTree) { diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java index 46a24d16f35a4..c0b8cd745afcf 100644 --- a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java @@ -49,9 +49,16 @@ public class ContextUserIdCheckerTest { "@SystemService(\"foo\") public class FooManager {", " Context mContext;", " IFooService mService;", + " final int mUserId;", + " FooManager(Context context) {", + " mUserId = mContext.getUserId();", + " }", " void bar() throws RemoteException {", " mService.baz(null, mContext.getUserId());", " }", + " void baz() throws RemoteException {", + " mService.baz(null, mUserId);", + " }", "}") .doTest(); } @@ -63,11 +70,13 @@ public class ContextUserIdCheckerTest { .addSourceFile("/android/content/Context.java") .addSourceFile("/android/foo/IFooService.java") .addSourceFile("/android/os/IInterface.java") + .addSourceFile("/android/os/UserHandle.java") .addSourceFile("/android/os/RemoteException.java") .addSourceLines("FooManager.java", "import android.annotation.SystemService;", "import android.content.Context;", "import android.foo.IFooService;", + "import android.os.UserHandle;", "import android.os.RemoteException;", "@SystemService(\"foo\") public class FooManager {", " Context mContext;", @@ -76,6 +85,10 @@ public class ContextUserIdCheckerTest { " // BUG: Diagnostic contains:", " mService.baz(null, 0);", " }", + " void baz() throws RemoteException {", + " // BUG: Diagnostic contains:", + " mService.baz(null, UserHandle.myUserId());", + " }", "}") .doTest(); } diff --git a/errorprone/tests/res/android/os/UserHandle.java b/errorprone/tests/res/android/os/UserHandle.java index a05fb9e42efa0..c58d661fad08e 100644 --- a/errorprone/tests/res/android/os/UserHandle.java +++ b/errorprone/tests/res/android/os/UserHandle.java @@ -28,4 +28,8 @@ public class UserHandle { public static int getUid(int userId, int appId) { throw new UnsupportedOperationException(); } + + public static int myUserId() { + throw new UnsupportedOperationException(); + } }