Skip to content

Commit 8c48b69

Browse files
wescandewescande
committed
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
1 parent a55f178 commit 8c48b69

File tree

4 files changed

+101
-71
lines changed

4 files changed

+101
-71
lines changed

‎errorprone/java/com/google/errorprone/bugpatterns/android/RequiresPermissionChecker.java

+73-71
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,9 @@
5555
import com.sun.tools.javac.code.Symbol;
5656
import com.sun.tools.javac.code.Symbol.ClassSymbol;
5757
import com.sun.tools.javac.code.Symbol.MethodSymbol;
58-
import com.sun.tools.javac.code.Symbol.VarSymbol;
5958
import com.sun.tools.javac.code.Type;
6059
import com.sun.tools.javac.code.Type.ClassType;
60+
import com.sun.tools.javac.tree.JCTree.JCNewClass;
6161

6262
import java.util.ArrayList;
6363
import java.util.Arrays;
@@ -67,7 +67,6 @@
6767
import java.util.Optional;
6868
import java.util.Set;
6969
import java.util.concurrent.atomic.AtomicReference;
70-
import java.util.function.Predicate;
7170
import java.util.regex.Pattern;
7271

7372
import javax.lang.model.element.Name;
@@ -125,6 +124,12 @@ public final class RequiresPermissionChecker extends BugChecker
125124
instanceMethod()
126125
.onDescendantOf("android.content.Context")
127126
.withNameMatching(Pattern.compile("^send(Ordered|Sticky)?Broadcast.*$")));
127+
private static final Matcher<ExpressionTree> SEND_BROADCAST_AS_USER =
128+
methodInvocation(
129+
instanceMethod()
130+
.onDescendantOf("android.content.Context")
131+
.withNameMatching(
132+
Pattern.compile("^send(Ordered|Sticky)?Broadcast.*AsUser.*$")));
128133
private static final Matcher<ExpressionTree> SEND_PENDING_INTENT = methodInvocation(
129134
instanceMethod()
130135
.onDescendantOf("android.app.PendingIntent")
@@ -306,18 +311,6 @@ public void addConstValue(Tree tree) {
306311
}
307312
}
308313

309-
private static ExpressionTree findArgumentByParameterName(MethodInvocationTree tree,
310-
Predicate<String> paramName) {
311-
final MethodSymbol sym = ASTHelpers.getSymbol(tree);
312-
final List<VarSymbol> params = sym.getParameters();
313-
for (int i = 0; i < params.size(); i++) {
314-
if (paramName.test(params.get(i).name.toString())) {
315-
return tree.getArguments().get(i);
316-
}
317-
}
318-
return null;
319-
}
320-
321314
private static Name resolveName(ExpressionTree tree) {
322315
if (tree instanceof IdentifierTree) {
323316
return ((IdentifierTree) tree).getName();
@@ -345,76 +338,85 @@ private static ParsedRequiresPermission parseIntentAction(MethodInvocationTree t
345338

346339
private static ParsedRequiresPermission parseBroadcastSourceRequiresPermission(
347340
MethodInvocationTree methodTree, VisitorState state) {
348-
final ExpressionTree arg = findArgumentByParameterName(methodTree,
349-
(name) -> name.toLowerCase().contains("intent"));
350-
if (arg instanceof IdentifierTree) {
351-
final Name argName = ((IdentifierTree) arg).getName();
352-
final MethodTree method = state.findEnclosing(MethodTree.class);
353-
final AtomicReference<ParsedRequiresPermission> res = new AtomicReference<>();
354-
method.accept(new TreeScanner<Void, Void>() {
355-
private ParsedRequiresPermission last;
356-
357-
@Override
358-
public Void visitMethodInvocation(MethodInvocationTree tree, Void param) {
359-
if (Objects.equal(methodTree, tree)) {
360-
res.set(last);
361-
} else {
362-
final Name name = resolveName(tree.getMethodSelect());
363-
if (Objects.equal(argName, name)
364-
&& INTENT_SET_ACTION.matches(tree, state)) {
365-
last = parseIntentAction(tree);
341+
if (methodTree.getArguments().size() < 1) {
342+
return null;
343+
}
344+
final ExpressionTree arg = methodTree.getArguments().get(0);
345+
if (!(arg instanceof IdentifierTree)) {
346+
return null;
347+
}
348+
final Name argName = ((IdentifierTree) arg).getName();
349+
final MethodTree method = state.findEnclosing(MethodTree.class);
350+
final AtomicReference<ParsedRequiresPermission> res = new AtomicReference<>();
351+
method.accept(new TreeScanner<Void, Void>() {
352+
private ParsedRequiresPermission mLast;
353+
354+
@Override
355+
public Void visitMethodInvocation(MethodInvocationTree tree, Void param) {
356+
if (Objects.equal(methodTree, tree)) {
357+
res.set(mLast);
358+
} else {
359+
final Name name = resolveName(tree.getMethodSelect());
360+
if (Objects.equal(argName, name) && INTENT_SET_ACTION.matches(tree, state)) {
361+
mLast = parseIntentAction(tree);
362+
} else if (name == null && tree.getMethodSelect() instanceof MemberSelectTree) {
363+
ExpressionTree innerTree =
364+
((MemberSelectTree) tree.getMethodSelect()).getExpression();
365+
if (innerTree instanceof JCNewClass) {
366+
mLast = parseIntentAction((NewClassTree) innerTree);
366367
}
367368
}
368-
return super.visitMethodInvocation(tree, param);
369369
}
370+
return super.visitMethodInvocation(tree, param);
371+
}
370372

371-
@Override
372-
public Void visitAssignment(AssignmentTree tree, Void param) {
373-
final Name name = resolveName(tree.getVariable());
374-
final Tree init = tree.getExpression();
375-
if (Objects.equal(argName, name)
376-
&& init instanceof NewClassTree) {
377-
last = parseIntentAction((NewClassTree) init);
378-
}
379-
return super.visitAssignment(tree, param);
373+
@Override
374+
public Void visitAssignment(AssignmentTree tree, Void param) {
375+
final Name name = resolveName(tree.getVariable());
376+
final Tree init = tree.getExpression();
377+
if (Objects.equal(argName, name) && init instanceof NewClassTree) {
378+
mLast = parseIntentAction((NewClassTree) init);
380379
}
380+
return super.visitAssignment(tree, param);
381+
}
381382

382-
@Override
383-
public Void visitVariable(VariableTree tree, Void param) {
384-
final Name name = tree.getName();
385-
final ExpressionTree init = tree.getInitializer();
386-
if (Objects.equal(argName, name)
387-
&& init instanceof NewClassTree) {
388-
last = parseIntentAction((NewClassTree) init);
389-
}
390-
return super.visitVariable(tree, param);
383+
@Override
384+
public Void visitVariable(VariableTree tree, Void param) {
385+
final Name name = tree.getName();
386+
final ExpressionTree init = tree.getInitializer();
387+
if (Objects.equal(argName, name) && init instanceof NewClassTree) {
388+
mLast = parseIntentAction((NewClassTree) init);
391389
}
392-
}, null);
393-
return res.get();
394-
}
395-
return null;
390+
return super.visitVariable(tree, param);
391+
}
392+
}, null);
393+
return res.get();
396394
}
397395

398396
private static ParsedRequiresPermission parseBroadcastTargetRequiresPermission(
399397
MethodInvocationTree tree, VisitorState state) {
400-
final ExpressionTree arg = findArgumentByParameterName(tree,
401-
(name) -> name.toLowerCase().contains("permission"));
402398
final ParsedRequiresPermission res = new ParsedRequiresPermission();
403-
if (arg != null) {
404-
arg.accept(new TreeScanner<Void, Void>() {
405-
@Override
406-
public Void visitIdentifier(IdentifierTree tree, Void param) {
407-
res.addConstValue(tree);
408-
return super.visitIdentifier(tree, param);
409-
}
410-
411-
@Override
412-
public Void visitMemberSelect(MemberSelectTree tree, Void param) {
413-
res.addConstValue(tree);
414-
return super.visitMemberSelect(tree, param);
415-
}
416-
}, null);
399+
int permission_position = 1;
400+
if (SEND_BROADCAST_AS_USER.matches(tree, state)) {
401+
permission_position = 2;
417402
}
403+
if (tree.getArguments().size() < permission_position + 1) {
404+
return res;
405+
}
406+
final ExpressionTree arg = tree.getArguments().get(permission_position);
407+
arg.accept(new TreeScanner<Void, Void>() {
408+
@Override
409+
public Void visitIdentifier(IdentifierTree tree, Void param) {
410+
res.addConstValue(tree);
411+
return super.visitIdentifier(tree, param);
412+
}
413+
414+
@Override
415+
public Void visitMemberSelect(MemberSelectTree tree, Void param) {
416+
res.addConstValue(tree);
417+
return super.visitMemberSelect(tree, param);
418+
}
419+
}, null);
418420
return res;
419421
}
420422

‎errorprone/tests/java/com/google/errorprone/bugpatterns/android/RequiresPermissionCheckerTest.java

+13
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,19 @@ public void testSendBroadcast() {
412412
" context.sendBroadcast(intent);",
413413
" }",
414414
" }",
415+
" public void exampleWithChainedMethod(Context context) {",
416+
" Intent intent = new Intent(FooManager.ACTION_RED)",
417+
" .putExtra(\"foo\", 42);",
418+
" context.sendBroadcast(intent, FooManager.PERMISSION_RED);",
419+
" context.sendBroadcastWithMultiplePermissions(intent,",
420+
" new String[] { FooManager.PERMISSION_RED });",
421+
" }",
422+
" public void exampleWithAsUser(Context context) {",
423+
" Intent intent = new Intent(FooManager.ACTION_RED);",
424+
" context.sendBroadcastAsUser(intent, 42, FooManager.PERMISSION_RED);",
425+
" context.sendBroadcastAsUserMultiplePermissions(intent, 42,",
426+
" new String[] { FooManager.PERMISSION_RED });",
427+
" }",
415428
"}")
416429
.doTest();
417430
}

‎errorprone/tests/res/android/content/Context.java

+11
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,15 @@ public void sendBroadcast(Intent intent, String receiverPermission) {
3636
public void sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) {
3737
throw new UnsupportedOperationException();
3838
}
39+
40+
/* Fake user type for test purposes */
41+
public void sendBroadcastAsUser(Intent intent, int user, String receiverPermission) {
42+
throw new UnsupportedOperationException();
43+
}
44+
45+
/* Fake user type for test purposes */
46+
public void sendBroadcastAsUserMultiplePermissions(
47+
Intent intent, int user, String[] receiverPermissions) {
48+
throw new UnsupportedOperationException();
49+
}
3950
}

‎errorprone/tests/res/android/content/Intent.java

+4
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,8 @@ public Intent(String action) {
2424
public Intent setAction(String action) {
2525
throw new UnsupportedOperationException();
2626
}
27+
28+
public Intent putExtra(String extra, int value) {
29+
throw new UnsupportedOperationException();
30+
}
2731
}

0 commit comments

Comments
 (0)