diff options
3 files changed, 101 insertions, 2 deletions
diff --git a/errorprone/Android.bp b/errorprone/Android.bp index 016b85510a94..098f4bfa74ac 100644 --- a/errorprone/Android.bp +++ b/errorprone/Android.bp @@ -20,6 +20,4 @@ java_library_host { plugins: [ "//external/dagger2:dagger2-auto-service", ], - - javacflags: ["-verbose"], } diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java new file mode 100644 index 000000000000..48123abd26cb --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java @@ -0,0 +1,80 @@ +/* + * 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.google.errorprone.bugpatterns.android; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +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.throwStatement; +import static com.google.errorprone.matchers.Matchers.variableType; + +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.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.CatchTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; + +import java.util.List; + +/** + * Apps making calls into the system server may end up persisting internal state + * or making security decisions based on the perceived success or failure of a + * call, or any default values returned. For this reason, we want to strongly + * throw when there was trouble with the transaction. + * <p> + * The rethrowFromSystemServer() method is the best-practice way of doing this + * correctly, so that we don't clutter logs with misleading stack traces, and + * this checker verifies that best-practice is used. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkRethrowFromSystem", + summary = "Verifies that system_server calls use rethrowFromSystemServer()", + severity = WARNING) +public final class RethrowFromSystemChecker extends BugChecker implements CatchTreeMatcher { + private static final Matcher<Tree> INSIDE_MANAGER = + enclosingClass(hasAnnotation("android.annotation.SystemService")); + private static final Matcher<VariableTree> REMOTE_EXCEPTION = variableType( + isSameType("android.os.RemoteException")); + private static final Matcher<StatementTree> RETHROW_FROM_SYSTEM = throwStatement( + methodInvocation(instanceMethod().onExactClass("android.os.RemoteException") + .named("rethrowFromSystemServer"))); + + @Override + public Description matchCatch(CatchTree tree, VisitorState state) { + if (INSIDE_MANAGER.matches(tree, state) + && REMOTE_EXCEPTION.matches(tree.getParameter(), state)) { + final List<? extends StatementTree> 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(); + } + } + return Description.NO_MATCH; + } +} diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java index 1ce816c34990..232cf3f0d677 100644 --- a/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java @@ -34,6 +34,27 @@ import com.sun.source.tree.BinaryTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.Tree.Kind; +/** + * Over the years we've had several obscure bugs related to how SDK level + * comparisons are performed, specifically during the window of time where we've + * started distributing the "frankenbuild" to developers. + * <p> + * Consider the case where a framework developer shipping release "R" wants to + * only grant a specific behavior to modern apps; they could write this in two + * different ways: + * <ol> + * <li>if (targetSdkVersion > Build.VERSION_CODES.Q) { + * <li>if (targetSdkVersion >= Build.VERSION_CODES.R) { + * </ol> + * The safer of these two options is (2), which will ensure that developers only + * get the behavior when <em>both</em> the app and the platform agree on the + * specific SDK level having shipped. + * <p> + * Consider the breakage that would happen with option (1) if we started + * shipping APKs that are based on the final R SDK, but are then installed on + * earlier preview releases which still consider R to be CUR_DEVELOPMENT; they'd + * risk crashing due to behaviors that were never part of the official R SDK. + */ @AutoService(BugChecker.class) @BugPattern( name = "AndroidFrameworkTargetSdk", |