From acc7080d7eceea8343a079ccade1a917aeaf2fc6 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 23 Jun 2020 23:19:57 -0600 Subject: Add checker to support createUserContext(). To avoid an explosion of startActivityForUser style methods, we've converged on recommending the use of Context.createContextAsUser(), and then ensuring that all system services pass Context.getUserId() for any int userId arguments across Binder interfaces. This design allows developers to easily redirect all services obtained from a specific Context to a different user with no additional API surface. Bug: 115654727, 159626156 Test: atest error_prone_android_framework_test Change-Id: I2d665016e8356807c371a1e18a4e102dea5b5d8e --- .../bugpatterns/android/ContextUserIdChecker.java | 100 +++++++++++++++++++++ .../android/ContextUserIdCheckerTest.java | 82 +++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java (limited to 'errorprone') diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java new file mode 100644 index 000000000000..7f2cce6ea7f0 --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/ContextUserIdChecker.java @@ -0,0 +1,100 @@ +/* + * 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.enclosingClass; +import static com.google.errorprone.matchers.Matchers.hasAnnotation; +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.Matchers.methodInvocation; + +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.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.VarSymbol; + +import java.util.List; +import java.util.Locale; +import java.util.function.Predicate; + +/** + * To avoid an explosion of {@code startActivityForUser} style methods, we've + * converged on recommending the use of {@code Context.createContextAsUser()}, + * and then ensuring that all system services pass {@link Context.getUserId()} + * for any {@code int userId} arguments across Binder interfaces. + *

+ * This design allows developers to easily redirect all services obtained from a + * specific {@code Context} to a different user with no additional API surface. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkContextUserId", + summary = "Verifies that system_server calls use Context.getUserId()", + severity = WARNING) +public final class ContextUserIdChecker extends BugChecker implements MethodInvocationTreeMatcher { + private static final Matcher INSIDE_MANAGER = + enclosingClass(hasAnnotation("android.annotation.SystemService")); + + private static final Matcher BINDER_CALL = methodInvocation( + instanceMethod().onDescendantOf("android.os.IInterface").withAnyName()); + private static final Matcher GET_USER_ID_CALL = methodInvocation( + instanceMethod().onDescendantOf("android.content.Context").named("getUserId")); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (INSIDE_MANAGER.matches(tree, state) + && BINDER_CALL.matches(tree, state)) { + final List vars = ASTHelpers.getSymbol(tree).params(); + for (int i = 0; i < vars.size(); i++) { + if (USER_ID_VAR.test(vars.get(i)) && + !GET_USER_ID_CALL.matches(tree.getArguments().get(i), state)) { + return buildDescription(tree) + .setMessage("Must pass Context.getUserId() as user ID" + + "to enable createContextAsUser()") + .build(); + } + } + } + return Description.NO_MATCH; + } + + private static final UserIdMatcher USER_ID_VAR = new UserIdMatcher(); + + private static class UserIdMatcher implements Predicate { + @Override + public boolean test(VarSymbol t) { + if ("int".equals(t.type.toString())) { + switch (t.name.toString().toLowerCase(Locale.ROOT)) { + case "user": + case "userid": + case "userhandle": + case "user_id": + return true; + } + } + return false; + } + } +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java new file mode 100644 index 000000000000..46a24d16f35a --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/ContextUserIdCheckerTest.java @@ -0,0 +1,82 @@ +/* + * 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 ContextUserIdCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + ContextUserIdChecker.class, getClass()); + } + + @Test + public void testValid() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.java") + .addSourceFile("/android/content/Context.java") + .addSourceFile("/android/foo/IFooService.java") + .addSourceFile("/android/os/IInterface.java") + .addSourceFile("/android/os/RemoteException.java") + .addSourceLines("FooManager.java", + "import android.annotation.SystemService;", + "import android.content.Context;", + "import android.foo.IFooService;", + "import android.os.RemoteException;", + "@SystemService(\"foo\") public class FooManager {", + " Context mContext;", + " IFooService mService;", + " void bar() throws RemoteException {", + " mService.baz(null, mContext.getUserId());", + " }", + "}") + .doTest(); + } + + @Test + public void testInvalid() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.java") + .addSourceFile("/android/content/Context.java") + .addSourceFile("/android/foo/IFooService.java") + .addSourceFile("/android/os/IInterface.java") + .addSourceFile("/android/os/RemoteException.java") + .addSourceLines("FooManager.java", + "import android.annotation.SystemService;", + "import android.content.Context;", + "import android.foo.IFooService;", + "import android.os.RemoteException;", + "@SystemService(\"foo\") public class FooManager {", + " Context mContext;", + " IFooService mService;", + " void bar() throws RemoteException {", + " // BUG: Diagnostic contains:", + " mService.baz(null, 0);", + " }", + "}") + .doTest(); + } +} -- cgit v1.2.3-59-g8ed1b