From 86ba608e34b881e2af42602cf4211de967d29df5 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 23 Jun 2020 23:16:59 -0600 Subject: [PATCH 1/3] Add tests for framework-specific Error Prone. We recently started writing custom Error Prone checkers, but it's been painfully slow to develop against the giant source tree, so this change adds tests to verify existing behavior and to enable TDD for future checkers. Bug: 155703208 Test: atest error_prone_android_framework_test Change-Id: I7ea7484db5d19e812354703e561a499077329098 --- errorprone/Android.bp | 22 ++++ errorprone/TEST_MAPPING | 7 ++ .../android/RethrowFromSystemCheckerTest.java | 108 ++++++++++++++++++ .../android/TargetSdkCheckerTest.java | 67 +++++++++++ .../res/android/annotation/SystemService.java | 21 ++++ .../tests/res/android/content/Context.java | 23 ++++ .../tests/res/android/foo/IFooService.java | 24 ++++ errorprone/tests/res/android/os/Build.java | 23 ++++ .../tests/res/android/os/IInterface.java | 20 ++++ .../tests/res/android/os/RemoteException.java | 23 ++++ 10 files changed, 338 insertions(+) create mode 100644 errorprone/TEST_MAPPING create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.java create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/TargetSdkCheckerTest.java create mode 100644 errorprone/tests/res/android/annotation/SystemService.java create mode 100644 errorprone/tests/res/android/content/Context.java create mode 100644 errorprone/tests/res/android/foo/IFooService.java create mode 100644 errorprone/tests/res/android/os/Build.java create mode 100644 errorprone/tests/res/android/os/IInterface.java create mode 100644 errorprone/tests/res/android/os/RemoteException.java diff --git a/errorprone/Android.bp b/errorprone/Android.bp index 098f4bfa74ac9..c5f189243a209 100644 --- a/errorprone/Android.bp +++ b/errorprone/Android.bp @@ -21,3 +21,25 @@ java_library_host { "//external/dagger2:dagger2-auto-service", ], } + +java_test_host { + name: "error_prone_android_framework_test", + test_suites: ["general-tests"], + srcs: ["tests/java/**/*.java"], + java_resource_dirs: ["tests/res"], + java_resources: [":error_prone_android_framework_testdata"], + static_libs: [ + "error_prone_android_framework_lib", + "error_prone_test_helpers", + "hamcrest-library", + "hamcrest", + "platform-test-annotations", + "junit", + ], +} + +filegroup { + name: "error_prone_android_framework_testdata", + path: "tests/res", + srcs: ["tests/res/**/*.java"], +} diff --git a/errorprone/TEST_MAPPING b/errorprone/TEST_MAPPING new file mode 100644 index 0000000000000..ee4552fb3b331 --- /dev/null +++ b/errorprone/TEST_MAPPING @@ -0,0 +1,7 @@ +{ + "presubmit": [ + { + "name": "error_prone_android_framework_test" + } + ] +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.java new file mode 100644 index 0000000000000..32efbf206a456 --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemCheckerTest.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 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 RethrowFromSystemCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + RethrowFromSystemChecker.class, getClass()); + } + + @Test + public void testValid() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.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.foo.IFooService;", + "import android.os.RemoteException;", + "@SystemService(\"foo\") public class FooManager {", + " IFooService mService;", + " void bar() {", + " try {", + " mService.bar();", + " } catch (RemoteException e) {", + " throw e.rethrowFromSystemServer();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void testInvalid() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.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.foo.IFooService;", + "import android.os.RemoteException;", + "@SystemService(\"foo\") public class FooManager {", + " IFooService mService;", + " void bar() {", + " try {", + " mService.bar();", + " // BUG: Diagnostic contains:", + " } catch (RemoteException e) {", + " e.printStackTrace();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void testIgnored() { + compilationHelper + .addSourceFile("/android/annotation/SystemService.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.foo.IFooService;", + "import android.os.RemoteException;", + "@SystemService(\"foo\") public class FooManager {", + " IFooService mService;", + " void bar() {", + " try {", + " mService.bar();", + " // BUG: Diagnostic contains:", + " } catch (RemoteException ignored) {", + " }", + " }", + "}") + .doTest(); + } +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/TargetSdkCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/TargetSdkCheckerTest.java new file mode 100644 index 0000000000000..99a21c973fe89 --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/TargetSdkCheckerTest.java @@ -0,0 +1,67 @@ +/* + * 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 TargetSdkCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + TargetSdkChecker.class, getClass()); + } + + @Test + public void testValid() { + compilationHelper + .addSourceFile("/android/os/Build.java") + .addSourceLines("Example.java", + "import android.os.Build;", + "public class Example {", + " void test(int targetSdkVersion) {", + " boolean res = targetSdkVersion >= Build.VERSION_CODES.DONUT;", + " if (targetSdkVersion < Build.VERSION_CODES.DONUT) { }", + " }", + "}") + .doTest(); + } + + @Test + public void testInvalid() { + compilationHelper + .addSourceFile("/android/os/Build.java") + .addSourceLines("Example.java", + "import android.os.Build;", + "public class Example {", + " void test(int targetSdkVersion) {", + " // BUG: Diagnostic contains:", + " boolean res = targetSdkVersion > Build.VERSION_CODES.DONUT;", + " // BUG: Diagnostic contains:", + " if (targetSdkVersion <= Build.VERSION_CODES.DONUT) { }", + " }", + "}") + .doTest(); + } +} diff --git a/errorprone/tests/res/android/annotation/SystemService.java b/errorprone/tests/res/android/annotation/SystemService.java new file mode 100644 index 0000000000000..b84edcbd25436 --- /dev/null +++ b/errorprone/tests/res/android/annotation/SystemService.java @@ -0,0 +1,21 @@ +/* + * 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 android.annotation; + +public @interface SystemService { + String value(); +} diff --git a/errorprone/tests/res/android/content/Context.java b/errorprone/tests/res/android/content/Context.java new file mode 100644 index 0000000000000..7ba3fbb56245d --- /dev/null +++ b/errorprone/tests/res/android/content/Context.java @@ -0,0 +1,23 @@ +/* + * 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 android.content; + +public class Context { + public int getUserId() { + return 0; + } +} diff --git a/errorprone/tests/res/android/foo/IFooService.java b/errorprone/tests/res/android/foo/IFooService.java new file mode 100644 index 0000000000000..1ae7253d889b6 --- /dev/null +++ b/errorprone/tests/res/android/foo/IFooService.java @@ -0,0 +1,24 @@ +/* + * 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 android.foo; + +import android.os.RemoteException; + +public interface IFooService extends android.os.IInterface { + public void bar() throws RemoteException; + public void baz(String baz, int userId) throws RemoteException; +} diff --git a/errorprone/tests/res/android/os/Build.java b/errorprone/tests/res/android/os/Build.java new file mode 100644 index 0000000000000..bbf7ef2172b54 --- /dev/null +++ b/errorprone/tests/res/android/os/Build.java @@ -0,0 +1,23 @@ +/* + * 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 android.os; + +public class Build { + public static class VERSION_CODES { + public static final int DONUT = 4; + } +} diff --git a/errorprone/tests/res/android/os/IInterface.java b/errorprone/tests/res/android/os/IInterface.java new file mode 100644 index 0000000000000..1a8a37cdb50c8 --- /dev/null +++ b/errorprone/tests/res/android/os/IInterface.java @@ -0,0 +1,20 @@ +/* + * 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 android.os; + +public interface IInterface { +} diff --git a/errorprone/tests/res/android/os/RemoteException.java b/errorprone/tests/res/android/os/RemoteException.java new file mode 100644 index 0000000000000..afe19881aae3e --- /dev/null +++ b/errorprone/tests/res/android/os/RemoteException.java @@ -0,0 +1,23 @@ +/* + * 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 android.os; + +public class RemoteException extends Exception { + public RuntimeException rethrowFromSystemServer() { + throw new RuntimeException(this); + } +} From acc7080d7eceea8343a079ccade1a917aeaf2fc6 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 23 Jun 2020 23:19:57 -0600 Subject: [PATCH 2/3] 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 --- .../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 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 0000000000000..7f2cce6ea7f05 --- /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 0000000000000..46a24d16f35a4 --- /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(); + } +} From 439b86167748a3e65b473bc80b3229b045bee90b Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Wed, 24 Jun 2020 12:58:02 -0600 Subject: [PATCH 3/3] Add checker for PID/UID/user ID arguments. Many system internals pass around PID, UID and user ID arguments as a single weakly-typed "int" value, which developers can accidentally cross in method argument lists, resulting in obscure bugs. Bug: 155703208 Test: atest error_prone_android_framework_test Change-Id: I5e4d9b5a533071f94d82dff17faff5d52ae54564 --- .../bugpatterns/android/UidChecker.java | 108 ++++++++++++++++++ .../bugpatterns/android/UidCheckerTest.java | 101 ++++++++++++++++ errorprone/tests/res/android/os/Binder.java | 23 ++++ .../tests/res/android/os/UserHandle.java | 31 +++++ 4 files changed, 263 insertions(+) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.java create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/UidCheckerTest.java create mode 100644 errorprone/tests/res/android/os/Binder.java create mode 100644 errorprone/tests/res/android/os/UserHandle.java diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.java new file mode 100644 index 0000000000000..533586f65c619 --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/UidChecker.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 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.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol.VarSymbol; + +import java.util.List; +import java.util.regex.Pattern; + +/** + * Many system internals pass around PID, UID and user ID arguments as a single + * weakly-typed {@code int} value, which developers can accidentally cross in + * method argument lists, resulting in obscure bugs. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkUid", + summary = "Verifies that PID, UID and user ID arguments aren't crossed", + severity = WARNING) +public final class UidChecker extends BugChecker implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + final List vars = ASTHelpers.getSymbol(tree).params(); + final List args = tree.getArguments(); + for (int i = 0; i < Math.min(vars.size(), args.size()); i++) { + final Flavor varFlavor = getFlavor(vars.get(i)); + final Flavor argFlavor = getFlavor(args.get(i)); + if (varFlavor == Flavor.UNKNOWN || argFlavor == Flavor.UNKNOWN) { + continue; + } + if (varFlavor != argFlavor) { + return buildDescription(tree).setMessage("Argument #" + (i + 1) + " expected " + + varFlavor + " but passed " + argFlavor).build(); + } + } + return Description.NO_MATCH; + } + + private static enum Flavor { + UNKNOWN(null), + PID(Pattern.compile("(^pid$|Pid$)")), + UID(Pattern.compile("(^uid$|Uid$)")), + USER_ID(Pattern.compile("(^userId$|UserId$|^userHandle$|UserHandle$)")); + + private Pattern pattern; + private Flavor(Pattern pattern) { + this.pattern = pattern; + } + public boolean matches(CharSequence input) { + return (pattern != null) && pattern.matcher(input).find(); + } + } + + private static Flavor getFlavor(String name) { + for (Flavor f : Flavor.values()) { + if (f.matches(name)) { + return f; + } + } + return Flavor.UNKNOWN; + } + + private static Flavor getFlavor(VarSymbol symbol) { + final String type = symbol.type.toString(); + if ("int".equals(type)) { + return getFlavor(symbol.name.toString()); + } + return Flavor.UNKNOWN; + } + + private static Flavor getFlavor(ExpressionTree tree) { + if (tree instanceof IdentifierTree) { + return getFlavor(((IdentifierTree) tree).getName().toString()); + } else if (tree instanceof MemberSelectTree) { + return getFlavor(((MemberSelectTree) tree).getIdentifier().toString()); + } else if (tree instanceof MethodInvocationTree) { + return getFlavor(((MethodInvocationTree) tree).getMethodSelect()); + } + return Flavor.UNKNOWN; + } +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/UidCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/UidCheckerTest.java new file mode 100644 index 0000000000000..74da947310924 --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/UidCheckerTest.java @@ -0,0 +1,101 @@ +/* + * 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 UidCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + UidChecker.class, getClass()); + } + + @Test + public void testTypical() { + compilationHelper + .addSourceLines("Example.java", + "public abstract class Example {", + " abstract void bar(int pid, int uid, int userId);", + " abstract int getUserId();", + " void foo(int pid, int uid, int userId, int unrelated) {", + " bar(0, 0, 0);", + " bar(pid, uid, userId);", + " bar(pid, uid, getUserId());", + " bar(unrelated, unrelated, unrelated);", + " // BUG: Diagnostic contains:", + " bar(uid, pid, userId);", + " // BUG: Diagnostic contains:", + " bar(pid, userId, uid);", + " // BUG: Diagnostic contains:", + " bar(getUserId(), 0, 0);", + " }", + "}") + .doTest(); + } + + @Test + public void testCallingUid() { + compilationHelper + .addSourceFile("/android/os/Binder.java") + .addSourceFile("/android/os/UserHandle.java") + .addSourceLines("Example.java", + "import android.os.Binder;", + "import android.os.UserHandle;", + "public abstract class Example {", + " int callingUserId;", + " int callingUid;", + " abstract void setCallingUserId(int callingUserId);", + " abstract void setCallingUid(int callingUid);", + " void doUserId(int callingUserId) {", + " setCallingUserId(UserHandle.getUserId(Binder.getCallingUid()));", + " setCallingUserId(this.callingUserId);", + " setCallingUserId(callingUserId);", + " // BUG: Diagnostic contains:", + " setCallingUserId(Binder.getCallingUid());", + " // BUG: Diagnostic contains:", + " setCallingUserId(this.callingUid);", + " // BUG: Diagnostic contains:", + " setCallingUserId(callingUid);", + " }", + " void doUid(int callingUserId) {", + " setCallingUid(Binder.getCallingUid());", + " setCallingUid(this.callingUid);", + " setCallingUid(callingUid);", + " // BUG: Diagnostic contains:", + " setCallingUid(UserHandle.getUserId(Binder.getCallingUid()));", + " // BUG: Diagnostic contains:", + " setCallingUid(this.callingUserId);", + " // BUG: Diagnostic contains:", + " setCallingUid(callingUserId);", + " }", + " void doInner() {", + " // BUG: Diagnostic contains:", + " setCallingUserId(UserHandle.getUserId(callingUserId));", + " }", + "}") + .doTest(); + } +} diff --git a/errorprone/tests/res/android/os/Binder.java b/errorprone/tests/res/android/os/Binder.java new file mode 100644 index 0000000000000..d388587c2f58b --- /dev/null +++ b/errorprone/tests/res/android/os/Binder.java @@ -0,0 +1,23 @@ +/* + * 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 android.os; + +public class Binder { + public static int getCallingUid() { + throw new UnsupportedOperationException(); + } +} diff --git a/errorprone/tests/res/android/os/UserHandle.java b/errorprone/tests/res/android/os/UserHandle.java new file mode 100644 index 0000000000000..a05fb9e42efa0 --- /dev/null +++ b/errorprone/tests/res/android/os/UserHandle.java @@ -0,0 +1,31 @@ +/* + * 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 android.os; + +public class UserHandle { + public static int getUserId(int uid) { + throw new UnsupportedOperationException(); + } + + public static int getAppId(int uid) { + throw new UnsupportedOperationException(); + } + + public static int getUid(int userId, int appId) { + throw new UnsupportedOperationException(); + } +}