From 1561df4a9b09a4118c3dfddd80c5691a183a9939 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 16:01:02 -0600 Subject: Detector for Binder.clearCallingIdentity() bugs. Binder maintains thread-local identity information about any remote caller, which can be temporarily cleared while performing operations that need to be handled as the current process. However, it's important to restore the original remote calling identity after carefully scoping this work inside a try/finally block, to avoid obscure security vulnerabilities. Bug: 155703208 Test: atest error_prone_android_framework_test Change-Id: I568771a50af27637e4984950dcada2248ce16afe --- .../bugpatterns/android/BinderIdentityChecker.java | 108 +++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java (limited to 'errorprone/java') diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java new file mode 100644 index 000000000000..68477edf97d1 --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java @@ -0,0 +1,108 @@ +/* + * 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.contains; +import static com.google.errorprone.matchers.Matchers.methodInvocation; +import static com.google.errorprone.matchers.Matchers.staticMethod; + +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.MethodInvocationTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.TryTree; +import com.sun.source.tree.VariableTree; + +import java.util.List; + +import javax.lang.model.element.Modifier; + +/** + * Binder maintains thread-local identity information about any remote caller, + * which can be temporarily cleared while performing operations that need to be + * handled as the current process. However, it's important to restore the + * original remote calling identity after carefully scoping this work inside a + * try/finally block, to avoid obscure security vulnerabilities. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkBinderIdentity", + summary = "Verifies that Binder.clearCallingIdentity() is always restored", + severity = WARNING) +public final class BinderIdentityChecker extends BugChecker implements MethodInvocationTreeMatcher { + private static final Matcher CLEAR_CALL = methodInvocation(staticMethod() + .onClass("android.os.Binder").withSignature("clearCallingIdentity()")); + private static final Matcher RESTORE_CALL = methodInvocation(staticMethod() + .onClass("android.os.Binder").withSignature("restoreCallingIdentity(long)")); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (CLEAR_CALL.matches(tree, state)) { + // First, make sure we're recording the token for later + final VariableTree token = state.findEnclosing(VariableTree.class); + if (token == null || !token.getModifiers().getFlags().contains(Modifier.FINAL)) { + return buildDescription(tree) + .setMessage("Must store Binder.clearCallingIdentity() token as final" + + " variable to support safe restore") + .build(); + } + + // Next, verify the very next block is try-finally; any other calls + // between the clearing and try risk throwing an exception without + // doing a safe restore + final Tree next = nextStatement(token, state); + if (next == null || next.getKind() != Kind.TRY) { + return buildDescription(tree) + .setMessage("Must immediately define a try-finally block after" + + " Binder.clearCallingIdentity() to support safe restore") + .build(); + } + + // Finally, verify that we restore inside the finally block + final TryTree tryTree = (TryTree) next; + final BlockTree finallyTree = tryTree.getFinallyBlock(); + if (finallyTree == null + || !contains(ExpressionTree.class, RESTORE_CALL).matches(finallyTree, state)) { + return buildDescription(tree) + .setMessage("Must call Binder.restoreCallingIdentity() in finally" + + " block to support safe restore") + .build(); + } + } + return Description.NO_MATCH; + } + + private static Tree nextStatement(Tree tree, VisitorState state) { + final BlockTree block = state.findEnclosing(BlockTree.class); + if (block == null) return null; + final List siblings = block.getStatements(); + if (siblings == null) return null; + final int index = siblings.indexOf(tree); + if (index == -1 || index + 1 >= siblings.size()) return null; + return siblings.get(index + 1); + } +} -- cgit v1.2.3-59-g8ed1b From 749789d24047f546a18392f83a9438bc737de490 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 16:49:24 -0600 Subject: Detector to suggest more efficient collections. Android offers several efficient alternatives to some upstream Collections containers, such as SparseIntArray instead of Map. Bug: 155703208 Test: atest error_prone_android_framework_test Change-Id: I080fd9489fb037391b717901345a905a9753b370 --- .../android/EfficientCollectionsChecker.java | 98 ++++++++++++++++++++++ .../android/EfficientCollectionsCheckerTest.java | 96 +++++++++++++++++++++ 2 files changed, 194 insertions(+) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java (limited to 'errorprone/java') diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java new file mode 100644 index 000000000000..c4c1ab6482ee --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java @@ -0,0 +1,98 @@ +/* + * 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.isSubtypeOf; + +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.NewClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; + +import java.util.Collections; +import java.util.List; + +/** + * Android offers several efficient alternatives to some upstream + * {@link Collections} containers, such as {@code SparseIntArray} instead of + * {@code Map}. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkEfficientCollections", + summary = "Verifies efficient collections best-practices", + severity = WARNING) +public final class EfficientCollectionsChecker extends BugChecker implements NewClassTreeMatcher { + private static final Matcher IS_LIST = isSubtypeOf("java.util.List"); + private static final Matcher IS_MAP = isSubtypeOf("java.util.Map"); + + private static final String INTEGER = "java.lang.Integer"; + private static final String LONG = "java.lang.Long"; + private static final String BOOLEAN = "java.lang.Boolean"; + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + final List types = ASTHelpers.getType(tree).getTypeArguments(); + if (IS_LIST.matches(tree, state) && types != null && types.size() == 1) { + final Type first = types.get(0); + if (ASTHelpers.isSameType(first, state.getTypeFromString(INTEGER), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with IntArray for efficiency") + .build(); + } else if (ASTHelpers.isSameType(first, state.getTypeFromString(LONG), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with LongArray for efficiency") + .build(); + } + } else if (IS_MAP.matches(tree, state) && types != null && types.size() == 2) { + final Type first = types.get(0); + final Type second = types.get(1); + if (ASTHelpers.isSameType(first, state.getTypeFromString(INTEGER), state)) { + if (ASTHelpers.isSameType(second, state.getTypeFromString(INTEGER), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with SparseIntArray for efficiency") + .build(); + } else if (ASTHelpers.isSameType(second, state.getTypeFromString(LONG), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with SparseLongArray for efficiency") + .build(); + } else if (ASTHelpers.isSameType(second, state.getTypeFromString(BOOLEAN), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with SparseBooleanArray for efficiency") + .build(); + } else { + return buildDescription(tree) + .setMessage("Consider replacing with SparseArray for efficiency") + .build(); + } + } else if (ASTHelpers.isSameType(first, state.getTypeFromString(LONG), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with LongSparseArray for efficiency") + .build(); + } + } + return Description.NO_MATCH; + } +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java new file mode 100644 index 000000000000..e128b6ad2cfd --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java @@ -0,0 +1,96 @@ +/* + * 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 com.google.errorprone.CompilationTestHelper; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class EfficientCollectionsCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + EfficientCollectionsChecker.class, getClass()); + } + + @Test + public void testMap() { + compilationHelper + .addSourceLines("Example.java", + "import java.util.HashMap;", + "public class Example {", + " public void exampleInteger() {", + " // BUG: Diagnostic contains:", + " HashMap a = new HashMap<>();", + " // BUG: Diagnostic contains:", + " HashMap b = new HashMap<>();", + " // BUG: Diagnostic contains:", + " HashMap c = new HashMap<>();", + " // BUG: Diagnostic contains:", + " HashMap d = new HashMap<>();", + " }", + " public void exampleLong() {", + " // BUG: Diagnostic contains:", + " HashMap res = new HashMap<>();", + " }", + " public void exampleOther() {", + " HashMap res = new HashMap<>();", + " }", + "}") + .doTest(); + } + + @Test + public void testList() { + compilationHelper + .addSourceLines("Example.java", + "import java.util.ArrayList;", + "public class Example {", + " public void exampleInteger() {", + " // BUG: Diagnostic contains:", + " ArrayList res = new ArrayList<>();", + " }", + " public void exampleLong() {", + " // BUG: Diagnostic contains:", + " ArrayList res = new ArrayList<>();", + " }", + " public void exampleOther() {", + " ArrayList res = new ArrayList<>();", + " }", + "}") + .doTest(); + } + + @Test + public void testErasure() { + compilationHelper + .addSourceLines("Example.java", + "import java.util.HashMap;", + "public class Example {", + " public void example() {", + " HashMap a = new HashMap();", + " }", + "}") + .doTest(); + } +} -- cgit v1.2.3-59-g8ed1b