From c095882faafe1e10754bd557db05e6c5b2635ff8 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 12:52:42 -0600 Subject: Exclude Telephony Binder interfaces. Purposefully exclude telephony Binder interfaces, since we know they always run under the separate AID_RADIO. Bug: 155703208 Test: atest error_prone_android_framework_test Change-Id: I3ce87caeb2abe3a7ca01ce10560d02b499ece07d --- .../android/RethrowFromSystemChecker.java | 51 +++++++++++++++++----- 1 file changed, 41 insertions(+), 10 deletions(-) (limited to 'errorprone/java') diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java index 48123abd26cb..130b256e6622 100644 --- a/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java @@ -17,11 +17,14 @@ package com.google.errorprone.bugpatterns.android; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.contains; import static com.google.errorprone.matchers.Matchers.enclosingClass; import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.isSameType; import static com.google.errorprone.matchers.Matchers.methodInvocation; +import static com.google.errorprone.matchers.Matchers.not; import static com.google.errorprone.matchers.Matchers.throwStatement; import static com.google.errorprone.matchers.Matchers.variableType; @@ -29,13 +32,17 @@ import com.google.auto.service.AutoService; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.CatchTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.TryTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.predicates.TypePredicate; import com.sun.source.tree.CatchTree; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.TryTree; import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Type; import java.util.List; @@ -54,9 +61,17 @@ import java.util.List; name = "AndroidFrameworkRethrowFromSystem", summary = "Verifies that system_server calls use rethrowFromSystemServer()", severity = WARNING) -public final class RethrowFromSystemChecker extends BugChecker implements CatchTreeMatcher { +public final class RethrowFromSystemChecker extends BugChecker implements TryTreeMatcher { private static final Matcher INSIDE_MANAGER = enclosingClass(hasAnnotation("android.annotation.SystemService")); + + // Purposefully exclude telephony Binder interfaces, since we know they + // always run under the separate AID_RADIO + private static final Matcher SYSTEM_BINDER_CALL = methodInvocation(allOf( + instanceMethod().onDescendantOf("android.os.IInterface").withAnyName(), + not(instanceMethod().onClass(inPackage("com.android.internal.telephony"))), + not(instanceMethod().onClass(inPackage("com.android.internal.telecom"))))); + private static final Matcher REMOTE_EXCEPTION = variableType( isSameType("android.os.RemoteException")); private static final Matcher RETHROW_FROM_SYSTEM = throwStatement( @@ -64,17 +79,33 @@ public final class RethrowFromSystemChecker extends BugChecker implements CatchT .named("rethrowFromSystemServer"))); @Override - public Description matchCatch(CatchTree tree, VisitorState state) { + public Description matchTry(TryTree tree, VisitorState state) { if (INSIDE_MANAGER.matches(tree, state) - && REMOTE_EXCEPTION.matches(tree.getParameter(), state)) { - final List statements = tree.getBlock().getStatements(); - if (statements.size() != 1 || !RETHROW_FROM_SYSTEM.matches(statements.get(0), state)) { - return buildDescription(tree) - .setMessage("Must contain single " - + "'throw e.rethrowFromSystemServer()' statement") - .build(); + && contains(ExpressionTree.class, SYSTEM_BINDER_CALL) + .matches(tree.getBlock(), state)) { + for (CatchTree catchTree : tree.getCatches()) { + if (REMOTE_EXCEPTION.matches(catchTree.getParameter(), state)) { + final List statements = catchTree.getBlock() + .getStatements(); + if (statements.size() != 1 + || !RETHROW_FROM_SYSTEM.matches(statements.get(0), state)) { + return buildDescription(catchTree) + .setMessage("Must contain single " + + "'throw e.rethrowFromSystemServer()' statement") + .build(); + } + } } } return Description.NO_MATCH; } + + private static TypePredicate inPackage(final String filter) { + return new TypePredicate() { + @Override + public boolean apply(Type type, VisitorState state) { + return type.tsym.packge().fullname.toString().startsWith(filter); + } + }; + } } -- cgit v1.2.3-59-g8ed1b From 74da287703f26d9d52ed02aabeb1815158194bf3 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 13:10:28 -0600 Subject: Slight relaxing of Context.getUserId() checks. Two specific managers have their own strategies using indirection to obtain Context.getUserId(), and we're willing to special case these to reduce warning noise. Bug: 115654727, 159626156 Test: atest error_prone_android_framework_test Change-Id: I290b74f7b6ec17c647f1cbb9ea7995f3fd56b132 --- .../bugpatterns/android/ContextUserIdChecker.java | 10 ++++- .../android/ContextUserIdCheckerTest.java | 52 ++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) (limited to 'errorprone/java') diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java index 4f1af3e9bea2..3a1bc1eeb9ae 100644 --- a/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java @@ -18,6 +18,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.anyOf; import static com.google.errorprone.matchers.Matchers.enclosingClass; import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.instanceMethod; @@ -60,8 +61,13 @@ public final class ContextUserIdChecker extends BugChecker implements MethodInvo private static final Matcher BINDER_CALL = methodInvocation( instanceMethod().onDescendantOf("android.os.IInterface").withAnyName()); - private static final Matcher GET_USER_ID_CALL = methodInvocation( - instanceMethod().onDescendantOf("android.content.Context").named("getUserId")); + private static final Matcher GET_USER_ID_CALL = methodInvocation(anyOf( + instanceMethod().onExactClass("android.app.admin.DevicePolicyManager") + .named("myUserId"), + instanceMethod().onExactClass("android.content.pm.ShortcutManager") + .named("injectMyUserId"), + instanceMethod().onDescendantOf("android.content.Context") + .named("getUserId"))); private static final Matcher USER_ID_FIELD = new Matcher() { @Override 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 c0b8cd745afc..c8772306a59b 100644 --- a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java @@ -92,4 +92,56 @@ public class ContextUserIdCheckerTest { "}") .doTest(); } + + @Test + public void testDevicePolicyManager() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.java") + .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("DevicePolicyManager.java", + "package android.app.admin;", + "import android.annotation.SystemService;", + "import android.content.Context;", + "import android.foo.IFooService;", + "import android.os.UserHandle;", + "import android.os.RemoteException;", + "@SystemService(\"dp\") public class DevicePolicyManager {", + " IFooService mService;", + " int myUserId() { return 0; }", + " void bar() throws RemoteException {", + " mService.baz(null, myUserId());", + " }", + "}") + .doTest(); + } + + @Test + public void testShortcutManager() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.java") + .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("ShortcutManager.java", + "package android.content.pm;", + "import android.annotation.SystemService;", + "import android.content.Context;", + "import android.foo.IFooService;", + "import android.os.UserHandle;", + "import android.os.RemoteException;", + "@SystemService(\"shortcut\") public class ShortcutManager {", + " IFooService mService;", + " int injectMyUserId() { return 0; }", + " void bar() throws RemoteException {", + " mService.baz(null, injectMyUserId());", + " }", + "}") + .doTest(); + } } -- cgit v1.2.3-59-g8ed1b