From 4d1d7b56cdb00e0f4706a585ab244db868c37f4e Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 4 May 2020 15:31:07 -0600 Subject: [PATCH] Add custom Error Prone check for SDK comparisons. 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. 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: 1. if (targetSdkVersion > Build.VERSION_CODES.Q) { 2. if (targetSdkVersion >= Build.VERSION_CODES.R) { The safer of these two options is (2), which will ensure that developers only get the behavior when *both* the app and the platform concur on the specific SDK level having shipped. 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. Bug: 64412239 Test: ./build/soong/soong_ui.bash --make-mode services RUN_ERROR_PRONE=true Exempt-From-Owner-Approval: trivial blueprint changes Change-Id: Ia20181f8602451ac9a719ea488d148e160708592 --- errorprone/Android.bp | 25 +++++ .../bugpatterns/android/TargetSdkChecker.java | 81 +++++++++++++++ .../errorprone/matchers/FieldMatchers.java | 99 +++++++++++++++++++ services/Android.bp | 8 +- services/accessibility/Android.bp | 1 + services/appprediction/Android.bp | 1 + services/appwidget/Android.bp | 1 + services/autofill/Android.bp | 1 + services/backup/Android.bp | 1 + services/companion/Android.bp | 1 + services/contentcapture/Android.bp | 1 + services/contentsuggestions/Android.bp | 1 + services/core/Android.bp | 1 + services/coverage/Android.bp | 1 + services/devicepolicy/Android.bp | 1 + services/midi/Android.bp | 1 + services/net/Android.bp | 1 + services/people/Android.bp | 1 + services/print/Android.bp | 1 + services/restrictions/Android.bp | 1 + services/startop/Android.bp | 1 + services/systemcaptions/Android.bp | 1 + services/usage/Android.bp | 1 + services/usb/Android.bp | 1 + services/voiceinteraction/Android.bp | 1 + services/wifi/Android.bp | 1 + 26 files changed, 234 insertions(+), 1 deletion(-) create mode 100644 errorprone/Android.bp create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java create mode 100644 errorprone/java/com/google/errorprone/matchers/FieldMatchers.java diff --git a/errorprone/Android.bp b/errorprone/Android.bp new file mode 100644 index 000000000000..016b85510a94 --- /dev/null +++ b/errorprone/Android.bp @@ -0,0 +1,25 @@ + +java_plugin { + name: "error_prone_android_framework", + + static_libs: [ + "error_prone_android_framework_lib", + ], +} + +java_library_host { + name: "error_prone_android_framework_lib", + + srcs: ["java/**/*.java"], + + static_libs: [ + "//external/error_prone:error_prone_core", + "//external/dagger2:dagger2-auto-service", + ], + + plugins: [ + "//external/dagger2:dagger2-auto-service", + ], + + javacflags: ["-verbose"], +} diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java new file mode 100644 index 000000000000..1ce816c34990 --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java @@ -0,0 +1,81 @@ +/* + * 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.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.anything; +import static com.google.errorprone.matchers.Matchers.kindIs; + +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.BinaryTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.FieldMatchers; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree.Kind; + +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkTargetSdk", + summary = "Verifies that all target SDK comparisons are sane", + severity = WARNING) +public final class TargetSdkChecker extends BugChecker implements BinaryTreeMatcher { + private static final Matcher VERSION_CODE = FieldMatchers + .anyFieldInClass("android.os.Build.VERSION_CODES"); + + private static final Matcher INVALID_OLD_BEHAVIOR = anyOf( + allOf(kindIs(Kind.LESS_THAN_EQUAL), binaryTreeExact(anything(), VERSION_CODE)), + allOf(kindIs(Kind.GREATER_THAN_EQUAL), binaryTreeExact(VERSION_CODE, anything()))); + + private static final Matcher INVALID_NEW_BEHAVIOR = anyOf( + allOf(kindIs(Kind.GREATER_THAN), binaryTreeExact(anything(), VERSION_CODE)), + allOf(kindIs(Kind.LESS_THAN), binaryTreeExact(VERSION_CODE, anything()))); + + @Override + public Description matchBinary(BinaryTree tree, VisitorState state) { + if (INVALID_OLD_BEHAVIOR.matches(tree, state)) { + return buildDescription(tree) + .setMessage("Legacy behaviors must be written in style " + + "'targetSdk < Build.VERSION_CODES.Z'") + .build(); + } + if (INVALID_NEW_BEHAVIOR.matches(tree, state)) { + return buildDescription(tree) + .setMessage("Modern behaviors must be written in style " + + "'targetSdk >= Build.VERSION_CODES.Z'") + .build(); + } + return Description.NO_MATCH; + } + + private static Matcher binaryTreeExact(Matcher left, + Matcher right) { + return new Matcher() { + @Override + public boolean matches(BinaryTree tree, VisitorState state) { + return left.matches(tree.getLeftOperand(), state) + && right.matches(tree.getRightOperand(), state); + } + }; + } +} diff --git a/errorprone/java/com/google/errorprone/matchers/FieldMatchers.java b/errorprone/java/com/google/errorprone/matchers/FieldMatchers.java new file mode 100644 index 000000000000..46f0fb2e534c --- /dev/null +++ b/errorprone/java/com/google/errorprone/matchers/FieldMatchers.java @@ -0,0 +1,99 @@ +/* + * Copyright 2018 The Error Prone Authors. + * + * 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.matchers; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.ImportTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import javax.annotation.Nullable; + +// TODO(glorioso): this likely wants to be a fluent interface like MethodMatchers. +// Ex: [staticField()|instanceField()] +// .[onClass(String)|onAnyClass|onClassMatching] +// .[named(String)|withAnyName|withNameMatching] +/** Static utility methods for creating {@link Matcher}s for detecting references to fields. */ +public final class FieldMatchers { + private FieldMatchers() {} + + public static Matcher anyFieldInClass(String className) { + return new FieldReferenceMatcher() { + @Override + boolean classIsAppropriate(ClassSymbol classSymbol) { + return classSymbol.getQualifiedName().contentEquals(className); + } + + @Override + boolean fieldSymbolIsAppropriate(Symbol symbol) { + return true; + } + }; + } + + public static Matcher staticField(String className, String fieldName) { + return new FieldReferenceMatcher() { + @Override + boolean classIsAppropriate(ClassSymbol classSymbol) { + return classSymbol.getQualifiedName().contentEquals(className); + } + + @Override + boolean fieldSymbolIsAppropriate(Symbol symbol) { + return symbol.isStatic() && symbol.getSimpleName().contentEquals(fieldName); + } + }; + } + + public static Matcher instanceField(String className, String fieldName) { + return new FieldReferenceMatcher() { + @Override + boolean classIsAppropriate(ClassSymbol classSymbol) { + return classSymbol.getQualifiedName().contentEquals(className); + } + + @Override + boolean fieldSymbolIsAppropriate(Symbol symbol) { + return !symbol.isStatic() && symbol.getSimpleName().contentEquals(fieldName); + } + }; + } + + private abstract static class FieldReferenceMatcher implements Matcher { + @Override + public boolean matches(ExpressionTree expressionTree, VisitorState state) { + return isSymbolFieldInAppropriateClass(ASTHelpers.getSymbol(expressionTree)) + // Don't match if this is part of a static import tree, since they will get the finding + // on any usage of the field in their source. + && ASTHelpers.findEnclosingNode(state.getPath(), ImportTree.class) == null; + } + + private boolean isSymbolFieldInAppropriateClass(@Nullable Symbol symbol) { + if (symbol == null) { + return false; + } + return symbol.getKind().isField() + && fieldSymbolIsAppropriate(symbol) + && classIsAppropriate(symbol.owner.enclClass()); + } + + abstract boolean fieldSymbolIsAppropriate(Symbol symbol); + + abstract boolean classIsAppropriate(ClassSymbol classSymbol); + } +} diff --git a/services/Android.bp b/services/Android.bp index 6d637bedeef7..74db5bde7d84 100644 --- a/services/Android.bp +++ b/services/Android.bp @@ -1,3 +1,10 @@ +java_defaults { + name: "services_defaults", + plugins: [ + "error_prone_android_framework", + ], +} + filegroup { name: "services-main-sources", srcs: ["java/**/*.java"], @@ -83,7 +90,6 @@ java_library { // Uncomment to enable output of certain warnings (deprecated, unchecked) //javacflags: ["-Xlint"], - } // native library diff --git a/services/accessibility/Android.bp b/services/accessibility/Android.bp index 284a2f2626a4..21a0c7489827 100644 --- a/services/accessibility/Android.bp +++ b/services/accessibility/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.accessibility", + defaults: ["services_defaults"], srcs: [":services.accessibility-sources"], libs: ["services.core"], } diff --git a/services/appprediction/Android.bp b/services/appprediction/Android.bp index e14e1df0b5c7..c12f62fc6cd1 100644 --- a/services/appprediction/Android.bp +++ b/services/appprediction/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.appprediction", + defaults: ["services_defaults"], srcs: [":services.appprediction-sources"], libs: ["services.core"], } diff --git a/services/appwidget/Android.bp b/services/appwidget/Android.bp index 54cf6cec78ea..83a9aa493bb0 100644 --- a/services/appwidget/Android.bp +++ b/services/appwidget/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.appwidget", + defaults: ["services_defaults"], srcs: [":services.appwidget-sources"], libs: ["services.core"], } diff --git a/services/autofill/Android.bp b/services/autofill/Android.bp index 539eb1a5220e..1e65e8459edf 100644 --- a/services/autofill/Android.bp +++ b/services/autofill/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.autofill", + defaults: ["services_defaults"], srcs: [":services.autofill-sources"], libs: ["services.core"], } diff --git a/services/backup/Android.bp b/services/backup/Android.bp index f02da2076706..56b788e34d35 100644 --- a/services/backup/Android.bp +++ b/services/backup/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.backup", + defaults: ["services_defaults"], srcs: [":services.backup-sources"], libs: ["services.core"], static_libs: ["backuplib"], diff --git a/services/companion/Android.bp b/services/companion/Android.bp index 9677a7d83bfb..e25104227b71 100644 --- a/services/companion/Android.bp +++ b/services/companion/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.companion", + defaults: ["services_defaults"], srcs: [":services.companion-sources"], libs: ["services.core"], } diff --git a/services/contentcapture/Android.bp b/services/contentcapture/Android.bp index 96e20726fed3..7006430067ec 100644 --- a/services/contentcapture/Android.bp +++ b/services/contentcapture/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.contentcapture", + defaults: ["services_defaults"], srcs: [":services.contentcapture-sources"], libs: ["services.core"], } diff --git a/services/contentsuggestions/Android.bp b/services/contentsuggestions/Android.bp index d17f06f79e97..3fe3cd252592 100644 --- a/services/contentsuggestions/Android.bp +++ b/services/contentsuggestions/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.contentsuggestions", + defaults: ["services_defaults"], srcs: [":services.contentsuggestions-sources"], libs: ["services.core"], } diff --git a/services/core/Android.bp b/services/core/Android.bp index 77773edc28ca..40f67c7cf065 100644 --- a/services/core/Android.bp +++ b/services/core/Android.bp @@ -148,6 +148,7 @@ java_genrule { java_library { name: "services.core", + defaults: ["services_defaults"], static_libs: ["services.core.priorityboosted"], } diff --git a/services/coverage/Android.bp b/services/coverage/Android.bp index e4f54644df46..df054b006cd3 100644 --- a/services/coverage/Android.bp +++ b/services/coverage/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.coverage", + defaults: ["services_defaults"], srcs: [":services.coverage-sources"], libs: ["jacocoagent"], } diff --git a/services/devicepolicy/Android.bp b/services/devicepolicy/Android.bp index 2f6592bd33b0..7a80fb1b8856 100644 --- a/services/devicepolicy/Android.bp +++ b/services/devicepolicy/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.devicepolicy", + defaults: ["services_defaults"], srcs: [":services.devicepolicy-sources"], libs: [ diff --git a/services/midi/Android.bp b/services/midi/Android.bp index 20e00834d0ad..6bce5b51ecb7 100644 --- a/services/midi/Android.bp +++ b/services/midi/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.midi", + defaults: ["services_defaults"], srcs: [":services.midi-sources"], libs: ["services.core"], } diff --git a/services/net/Android.bp b/services/net/Android.bp index bb5409b3e032..eaf11fd73a89 100644 --- a/services/net/Android.bp +++ b/services/net/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.net", + defaults: ["services_defaults"], srcs: [ ":net-module-utils-srcs", ":services.net-sources", diff --git a/services/people/Android.bp b/services/people/Android.bp index d64097a03ae9..c863f1f5919f 100644 --- a/services/people/Android.bp +++ b/services/people/Android.bp @@ -1,5 +1,6 @@ java_library_static { name: "services.people", + defaults: ["services_defaults"], srcs: ["java/**/*.java"], libs: ["services.core"], } diff --git a/services/print/Android.bp b/services/print/Android.bp index aad24d72345b..93b5ef040926 100644 --- a/services/print/Android.bp +++ b/services/print/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.print", + defaults: ["services_defaults"], srcs: [":services.print-sources"], libs: ["services.core"], } diff --git a/services/restrictions/Android.bp b/services/restrictions/Android.bp index 805858f7f654..28830956e7f5 100644 --- a/services/restrictions/Android.bp +++ b/services/restrictions/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.restrictions", + defaults: ["services_defaults"], srcs: [":services.restrictions-sources"], libs: ["services.core"], } diff --git a/services/startop/Android.bp b/services/startop/Android.bp index 093b4ec66ddf..46a81aae63c5 100644 --- a/services/startop/Android.bp +++ b/services/startop/Android.bp @@ -16,6 +16,7 @@ java_library_static { name: "services.startop", + defaults: ["services_defaults"], static_libs: [ // frameworks/base/startop/iorap diff --git a/services/systemcaptions/Android.bp b/services/systemcaptions/Android.bp index 1ce3e665c665..54968c003560 100644 --- a/services/systemcaptions/Android.bp +++ b/services/systemcaptions/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.systemcaptions", + defaults: ["services_defaults"], srcs: [":services.systemcaptions-sources"], libs: ["services.core"], } diff --git a/services/usage/Android.bp b/services/usage/Android.bp index 156bf330c128..463673f104ab 100644 --- a/services/usage/Android.bp +++ b/services/usage/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.usage", + defaults: ["services_defaults"], srcs: [":services.usage-sources"], libs: ["services.core"], } diff --git a/services/usb/Android.bp b/services/usb/Android.bp index a9474c10017e..4e984093cfec 100644 --- a/services/usb/Android.bp +++ b/services/usb/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.usb", + defaults: ["services_defaults"], srcs: [":services.usb-sources"], libs: [ diff --git a/services/voiceinteraction/Android.bp b/services/voiceinteraction/Android.bp index 85b96f34f4f6..47129ad62e86 100644 --- a/services/voiceinteraction/Android.bp +++ b/services/voiceinteraction/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.voiceinteraction", + defaults: ["services_defaults"], srcs: [":services.voiceinteraction-sources"], libs: ["services.core"], } diff --git a/services/wifi/Android.bp b/services/wifi/Android.bp index f56c2cf76956..3975fd238bb5 100644 --- a/services/wifi/Android.bp +++ b/services/wifi/Android.bp @@ -7,6 +7,7 @@ filegroup { java_library_static { name: "services.wifi", + defaults: ["services_defaults"], srcs: [ ":services.wifi-sources", ],