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 0000000000000..68477edf97d1f --- /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); + } +} 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 0000000000000..c4c1ab6482ee5 --- /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/BinderIdentityCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/BinderIdentityCheckerTest.java new file mode 100644 index 0000000000000..9448344f7abb3 --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/BinderIdentityCheckerTest.java @@ -0,0 +1,111 @@ +/* + * 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 BinderIdentityCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + BinderIdentityChecker.class, getClass()); + } + + @Test + public void testValid() { + compilationHelper + .addSourceFile("/android/os/Binder.java") + .addSourceLines("FooService.java", + "import android.os.Binder;", + "public class FooService {", + " void bar() {", + " final long token = Binder.clearCallingIdentity();", + " try {", + " FooService.class.toString();", + " } finally {", + " Binder.restoreCallingIdentity(token);", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void testInvalid() { + compilationHelper + .addSourceFile("/android/os/Binder.java") + .addSourceLines("FooService.java", + "import android.os.Binder;", + "public class FooService {", + " void noRestore() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " FooService.class.toString();", + " }", + " void noTry() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " FooService.class.toString();", + " Binder.restoreCallingIdentity(token);", + " }", + " void noImmediateTry() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " FooService.class.toString();", + " try {", + " FooService.class.toString();", + " } finally {", + " Binder.restoreCallingIdentity(token);", + " }", + " }", + " void noFinally() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " try {", + " FooService.class.toString();", + " } catch (Exception ignored) { }", + " }", + " void noFinal() {", + " // BUG: Diagnostic contains:", + " long token = Binder.clearCallingIdentity();", + " try {", + " FooService.class.toString();", + " } finally {", + " Binder.restoreCallingIdentity(token);", + " }", + " }", + " void noRecording() {", + " // BUG: Diagnostic contains:", + " Binder.clearCallingIdentity();", + " FooService.class.toString();", + " }", + " void noWork() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " }", + "}") + .doTest(); + } +} 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 0000000000000..e128b6ad2cfd9 --- /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(); + } +} diff --git a/errorprone/tests/res/android/os/Binder.java b/errorprone/tests/res/android/os/Binder.java index d388587c2f58b..c969108bd425b 100644 --- a/errorprone/tests/res/android/os/Binder.java +++ b/errorprone/tests/res/android/os/Binder.java @@ -20,4 +20,12 @@ public class Binder { public static int getCallingUid() { throw new UnsupportedOperationException(); } + + public static long clearCallingIdentity() { + throw new UnsupportedOperationException(); + } + + public static void restoreCallingIdentity(long token) { + throw new UnsupportedOperationException(); + } }