RequiresPermissionChecker: change detection method
Change the way the checker find the intent and the permission by using
the order instead of using the name of the variable. (since some target
have their parameters name being obfuscated prior to pass them to the
annotation processor)
Also fix when the intent is set in a chained pattern like:
new Intent(Foo).putExtra(...)
Add various test to make sure the new detection works for differently
ordered parameters
Fix checkstyle reported errors and move some check to be early return
instead of having a unneeded indentation level
Bug: 352610940
Fix: 352610940
Test: atest error_prone_android_framework_test | for unit test
Test: m Bluetooth and check the warning / errors
Flag: TEST_ONLY
Change-Id: I9308c08c7275708c5b1db3f35a9753563ba05dc9
diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/RequiresPermissionChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/RequiresPermissionChecker.java
index 7c7cb18..9887c27 100644
--- a/errorprone/java/com/google/errorprone/bugpatterns/android/RequiresPermissionChecker.java
+++ b/errorprone/java/com/google/errorprone/bugpatterns/android/RequiresPermissionChecker.java
@@ -55,9 +55,9 @@
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
-import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Type.ClassType;
+import com.sun.tools.javac.tree.JCTree.JCNewClass;
import java.util.ArrayList;
import java.util.Arrays;
@@ -67,7 +67,6 @@
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
-import java.util.function.Predicate;
import java.util.regex.Pattern;
import javax.lang.model.element.Name;
@@ -125,6 +124,12 @@
instanceMethod()
.onDescendantOf("android.content.Context")
.withNameMatching(Pattern.compile("^send(Ordered|Sticky)?Broadcast.*$")));
+ private static final Matcher<ExpressionTree> SEND_BROADCAST_AS_USER =
+ methodInvocation(
+ instanceMethod()
+ .onDescendantOf("android.content.Context")
+ .withNameMatching(
+ Pattern.compile("^send(Ordered|Sticky)?Broadcast.*AsUser.*$")));
private static final Matcher<ExpressionTree> SEND_PENDING_INTENT = methodInvocation(
instanceMethod()
.onDescendantOf("android.app.PendingIntent")
@@ -306,18 +311,6 @@
}
}
- private static ExpressionTree findArgumentByParameterName(MethodInvocationTree tree,
- Predicate<String> paramName) {
- final MethodSymbol sym = ASTHelpers.getSymbol(tree);
- final List<VarSymbol> params = sym.getParameters();
- for (int i = 0; i < params.size(); i++) {
- if (paramName.test(params.get(i).name.toString())) {
- return tree.getArguments().get(i);
- }
- }
- return null;
- }
-
private static Name resolveName(ExpressionTree tree) {
if (tree instanceof IdentifierTree) {
return ((IdentifierTree) tree).getName();
@@ -345,76 +338,85 @@
private static ParsedRequiresPermission parseBroadcastSourceRequiresPermission(
MethodInvocationTree methodTree, VisitorState state) {
- final ExpressionTree arg = findArgumentByParameterName(methodTree,
- (name) -> name.toLowerCase().contains("intent"));
- if (arg instanceof IdentifierTree) {
- final Name argName = ((IdentifierTree) arg).getName();
- final MethodTree method = state.findEnclosing(MethodTree.class);
- final AtomicReference<ParsedRequiresPermission> res = new AtomicReference<>();
- method.accept(new TreeScanner<Void, Void>() {
- private ParsedRequiresPermission last;
+ if (methodTree.getArguments().size() < 1) {
+ return null;
+ }
+ final ExpressionTree arg = methodTree.getArguments().get(0);
+ if (!(arg instanceof IdentifierTree)) {
+ return null;
+ }
+ final Name argName = ((IdentifierTree) arg).getName();
+ final MethodTree method = state.findEnclosing(MethodTree.class);
+ final AtomicReference<ParsedRequiresPermission> res = new AtomicReference<>();
+ method.accept(new TreeScanner<Void, Void>() {
+ private ParsedRequiresPermission mLast;
- @Override
- public Void visitMethodInvocation(MethodInvocationTree tree, Void param) {
- if (Objects.equal(methodTree, tree)) {
- res.set(last);
- } else {
- final Name name = resolveName(tree.getMethodSelect());
- if (Objects.equal(argName, name)
- && INTENT_SET_ACTION.matches(tree, state)) {
- last = parseIntentAction(tree);
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree tree, Void param) {
+ if (Objects.equal(methodTree, tree)) {
+ res.set(mLast);
+ } else {
+ final Name name = resolveName(tree.getMethodSelect());
+ if (Objects.equal(argName, name) && INTENT_SET_ACTION.matches(tree, state)) {
+ mLast = parseIntentAction(tree);
+ } else if (name == null && tree.getMethodSelect() instanceof MemberSelectTree) {
+ ExpressionTree innerTree =
+ ((MemberSelectTree) tree.getMethodSelect()).getExpression();
+ if (innerTree instanceof JCNewClass) {
+ mLast = parseIntentAction((NewClassTree) innerTree);
}
}
- return super.visitMethodInvocation(tree, param);
}
+ return super.visitMethodInvocation(tree, param);
+ }
- @Override
- public Void visitAssignment(AssignmentTree tree, Void param) {
- final Name name = resolveName(tree.getVariable());
- final Tree init = tree.getExpression();
- if (Objects.equal(argName, name)
- && init instanceof NewClassTree) {
- last = parseIntentAction((NewClassTree) init);
- }
- return super.visitAssignment(tree, param);
+ @Override
+ public Void visitAssignment(AssignmentTree tree, Void param) {
+ final Name name = resolveName(tree.getVariable());
+ final Tree init = tree.getExpression();
+ if (Objects.equal(argName, name) && init instanceof NewClassTree) {
+ mLast = parseIntentAction((NewClassTree) init);
}
+ return super.visitAssignment(tree, param);
+ }
- @Override
- public Void visitVariable(VariableTree tree, Void param) {
- final Name name = tree.getName();
- final ExpressionTree init = tree.getInitializer();
- if (Objects.equal(argName, name)
- && init instanceof NewClassTree) {
- last = parseIntentAction((NewClassTree) init);
- }
- return super.visitVariable(tree, param);
+ @Override
+ public Void visitVariable(VariableTree tree, Void param) {
+ final Name name = tree.getName();
+ final ExpressionTree init = tree.getInitializer();
+ if (Objects.equal(argName, name) && init instanceof NewClassTree) {
+ mLast = parseIntentAction((NewClassTree) init);
}
- }, null);
- return res.get();
- }
- return null;
+ return super.visitVariable(tree, param);
+ }
+ }, null);
+ return res.get();
}
private static ParsedRequiresPermission parseBroadcastTargetRequiresPermission(
MethodInvocationTree tree, VisitorState state) {
- final ExpressionTree arg = findArgumentByParameterName(tree,
- (name) -> name.toLowerCase().contains("permission"));
final ParsedRequiresPermission res = new ParsedRequiresPermission();
- if (arg != null) {
- arg.accept(new TreeScanner<Void, Void>() {
- @Override
- public Void visitIdentifier(IdentifierTree tree, Void param) {
- res.addConstValue(tree);
- return super.visitIdentifier(tree, param);
- }
-
- @Override
- public Void visitMemberSelect(MemberSelectTree tree, Void param) {
- res.addConstValue(tree);
- return super.visitMemberSelect(tree, param);
- }
- }, null);
+ int permission_position = 1;
+ if (SEND_BROADCAST_AS_USER.matches(tree, state)) {
+ permission_position = 2;
}
+ if (tree.getArguments().size() < permission_position + 1) {
+ return res;
+ }
+ final ExpressionTree arg = tree.getArguments().get(permission_position);
+ arg.accept(new TreeScanner<Void, Void>() {
+ @Override
+ public Void visitIdentifier(IdentifierTree tree, Void param) {
+ res.addConstValue(tree);
+ return super.visitIdentifier(tree, param);
+ }
+
+ @Override
+ public Void visitMemberSelect(MemberSelectTree tree, Void param) {
+ res.addConstValue(tree);
+ return super.visitMemberSelect(tree, param);
+ }
+ }, null);
return res;
}
diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RequiresPermissionCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RequiresPermissionCheckerTest.java
index e53372d..05fde7c 100644
--- a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RequiresPermissionCheckerTest.java
+++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RequiresPermissionCheckerTest.java
@@ -412,6 +412,19 @@
" context.sendBroadcast(intent);",
" }",
" }",
+ " public void exampleWithChainedMethod(Context context) {",
+ " Intent intent = new Intent(FooManager.ACTION_RED)",
+ " .putExtra(\"foo\", 42);",
+ " context.sendBroadcast(intent, FooManager.PERMISSION_RED);",
+ " context.sendBroadcastWithMultiplePermissions(intent,",
+ " new String[] { FooManager.PERMISSION_RED });",
+ " }",
+ " public void exampleWithAsUser(Context context) {",
+ " Intent intent = new Intent(FooManager.ACTION_RED);",
+ " context.sendBroadcastAsUser(intent, 42, FooManager.PERMISSION_RED);",
+ " context.sendBroadcastAsUserMultiplePermissions(intent, 42,",
+ " new String[] { FooManager.PERMISSION_RED });",
+ " }",
"}")
.doTest();
}
diff --git a/errorprone/tests/res/android/content/Context.java b/errorprone/tests/res/android/content/Context.java
index efc4fb1..9d622ff 100644
--- a/errorprone/tests/res/android/content/Context.java
+++ b/errorprone/tests/res/android/content/Context.java
@@ -36,4 +36,15 @@
public void sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) {
throw new UnsupportedOperationException();
}
+
+ /* Fake user type for test purposes */
+ public void sendBroadcastAsUser(Intent intent, int user, String receiverPermission) {
+ throw new UnsupportedOperationException();
+ }
+
+ /* Fake user type for test purposes */
+ public void sendBroadcastAsUserMultiplePermissions(
+ Intent intent, int user, String[] receiverPermissions) {
+ throw new UnsupportedOperationException();
+ }
}
diff --git a/errorprone/tests/res/android/content/Intent.java b/errorprone/tests/res/android/content/Intent.java
index 288396e..7ccea78 100644
--- a/errorprone/tests/res/android/content/Intent.java
+++ b/errorprone/tests/res/android/content/Intent.java
@@ -24,4 +24,8 @@
public Intent setAction(String action) {
throw new UnsupportedOperationException();
}
+
+ public Intent putExtra(String extra, int value) {
+ throw new UnsupportedOperationException();
+ }
}