From c095882faafe1e10754bd557db05e6c5b2635ff8 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 12:52:42 -0600 Subject: [PATCH 1/2] 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 +++++++++++++++---- .../android/RethrowFromSystemCheckerTest.java | 23 +++++++++ .../internal/telephony/ITelephony.java | 23 +++++++++ 3 files changed, 87 insertions(+), 10 deletions(-) create mode 100644 errorprone/tests/res/com/android/internal/telephony/ITelephony.java diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java index 48123abd26cb4..130b256e66226 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); + } + }; + } } diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.java index 32efbf206a456..0943bd65c06ff 100644 --- a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.java +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.java @@ -105,4 +105,27 @@ public class RethrowFromSystemCheckerTest { "}") .doTest(); } + + @Test + public void testTelephony() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.java") + .addSourceFile("/com/android/internal/telephony/ITelephony.java") + .addSourceFile("/android/os/IInterface.java") + .addSourceFile("/android/os/RemoteException.java") + .addSourceLines("TelephonyManager.java", + "import android.annotation.SystemService;", + "import com.android.internal.telephony.ITelephony;", + "import android.os.RemoteException;", + "@SystemService(\"telephony\") public class TelephonyManager {", + " ITelephony mService;", + " void bar() {", + " try {", + " mService.bar();", + " } catch (RemoteException ignored) {", + " }", + " }", + "}") + .doTest(); + } } diff --git a/errorprone/tests/res/com/android/internal/telephony/ITelephony.java b/errorprone/tests/res/com/android/internal/telephony/ITelephony.java new file mode 100644 index 0000000000000..61c4dd561b0b2 --- /dev/null +++ b/errorprone/tests/res/com/android/internal/telephony/ITelephony.java @@ -0,0 +1,23 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.internal.telephony; + +import android.os.RemoteException; + +public interface ITelephony extends android.os.IInterface { + public void bar() throws RemoteException; +} From 74da287703f26d9d52ed02aabeb1815158194bf3 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 13:10:28 -0600 Subject: [PATCH 2/2] 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 --- .../android/ContextUserIdChecker.java | 10 +++- .../android/ContextUserIdCheckerTest.java | 52 +++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java index 4f1af3e9bea2a..3a1bc1eeb9aea 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 c0b8cd745afcf..c8772306a59b3 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(); + } }