From f0de8d34a3ac66a9b2fd7dd6c0beb9ebd979f1e6 Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Mon, 19 Jul 2021 10:30:18 +0000 Subject: [PATCH 1/4] Rename AndroidFrameworkLintChecker's package to com.google.android.lint The previous package name com.android.lint is not allowed for custom lint checks because lint thinks any package with com.android. is its own API, so it performs a reflection check to see if it is a known API, but it fails because it is a custom check. For testing, verify that the lint report does not report an Obsolete Custom Lint Check issue. Bug: 192870410 Test: m out/soong/.intermediates/frameworks/base/services/autofill/services.autofill/android_common/lint/lint-report.html Change-Id: I4970d230329a2ab48ec1c0152d824995fa005127 --- .../lint/CallingIdentityTokenDetector.kt | 28 +++++++++---------- .../lint/CallingIdentityTokenIssueRegistry.kt | 16 +++++------ .../lint/CallingIdentityTokenDetectorTest.kt | 2 +- 3 files changed, 22 insertions(+), 24 deletions(-) rename tools/lint/checks/src/main/java/com/{ => google}/android/lint/CallingIdentityTokenDetector.kt (90%) rename tools/lint/checks/src/main/java/com/{ => google}/android/lint/CallingIdentityTokenIssueRegistry.kt (96%) rename tools/lint/checks/src/test/java/com/{ => google}/android/lint/CallingIdentityTokenDetectorTest.kt (99%) diff --git a/tools/lint/checks/src/main/java/com/android/lint/CallingIdentityTokenDetector.kt b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt similarity index 90% rename from tools/lint/checks/src/main/java/com/android/lint/CallingIdentityTokenDetector.kt rename to tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt index a41d65d7ea3ae..b5cab99b4e527 100644 --- a/tools/lint/checks/src/main/java/com/android/lint/CallingIdentityTokenDetector.kt +++ b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt @@ -14,21 +14,21 @@ * limitations under the License. */ -package com.android.lint +package com.google.android.lint -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_NESTED_CLEAR_IDENTITY_CALLS -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_NON_FINAL_TOKEN -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_UNUSED_TOKEN -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageClearIdentityCallNotFollowedByTryFinally -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNestedClearIdentityCallsPrimary -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNestedClearIdentityCallsSecondary -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNonFinalToken -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageRestoreIdentityCallNotInFinallyBlock -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageUnusedToken -import com.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_NESTED_CLEAR_IDENTITY_CALLS +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_NON_FINAL_TOKEN +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_UNUSED_TOKEN +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageClearIdentityCallNotFollowedByTryFinally +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNestedClearIdentityCallsPrimary +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNestedClearIdentityCallsSecondary +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNonFinalToken +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageRestoreIdentityCallNotInFinallyBlock +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageUnusedToken +import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity import com.android.tools.lint.client.api.UElementHandler import com.android.tools.lint.detector.api.Context import com.android.tools.lint.detector.api.Detector diff --git a/tools/lint/checks/src/main/java/com/android/lint/CallingIdentityTokenIssueRegistry.kt b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenIssueRegistry.kt similarity index 96% rename from tools/lint/checks/src/main/java/com/android/lint/CallingIdentityTokenIssueRegistry.kt rename to tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenIssueRegistry.kt index 1bd63ea17374f..8a747b64c3a4f 100644 --- a/tools/lint/checks/src/main/java/com/android/lint/CallingIdentityTokenIssueRegistry.kt +++ b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenIssueRegistry.kt @@ -14,11 +14,10 @@ * limitations under the License. */ -package com.android.lint +package com.google.android.lint import com.android.tools.lint.client.api.IssueRegistry -// TODO: uncomment when lint API in Soong becomes 30.0+ -// import com.android.tools.lint.client.api.Vendor +import com.android.tools.lint.client.api.Vendor import com.android.tools.lint.detector.api.CURRENT_API import com.android.tools.lint.detector.api.Category import com.android.tools.lint.detector.api.Implementation @@ -45,12 +44,11 @@ class CallingIdentityTokenIssueRegistry : IssueRegistry() { override val minApi: Int get() = 8 -// TODO: uncomment when lint API in Soong becomes 30.0+ -// override val vendor: Vendor = Vendor( -// vendorName = "Android Open Source Project", -// feedbackUrl = "http://b/issues/new?component=315013", -// contact = "brufino@google.com" -// ) + override val vendor: Vendor = Vendor( + vendorName = "Android", + feedbackUrl = "http://b/issues/new?component=315013", + contact = "brufino@google.com" + ) companion object { /** Issue: unused token from Binder.clearCallingIdentity() */ diff --git a/tools/lint/checks/src/test/java/com/android/lint/CallingIdentityTokenDetectorTest.kt b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt similarity index 99% rename from tools/lint/checks/src/test/java/com/android/lint/CallingIdentityTokenDetectorTest.kt rename to tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt index 0fd1a7621238f..8d12050cae1a4 100644 --- a/tools/lint/checks/src/test/java/com/android/lint/CallingIdentityTokenDetectorTest.kt +++ b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt @@ -14,7 +14,7 @@ * limitations under the License. */ -package com.android.lint +package com.google.android.lint import com.android.tools.lint.checks.infrastructure.LintDetectorTest import com.android.tools.lint.checks.infrastructure.TestFile From 8afcc7d5ba3e10f7d4d8dbdade417a99aab352ba Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Mon, 12 Jul 2021 12:34:46 +0000 Subject: [PATCH 2/4] Moved issues/IssueRegistry to mimic AS convention + README for AndroidFrameworkLintChecker Moved CallingIdentityTokenDetector's issues from IssueRegistry to itself since this is the pattern used for built-in Android Studio checks. Wrote README.md on how to build a lint report and how to add new lint checks. Bug: 192925086 Test: m out/soong/.intermediates/frameworks/base/services/accessibility/services.accessibility/android_common/lint/lint-report.html Change-Id: I6be3d49e5eacc34cf60db125a52ae3da9174be4a --- tools/lint/README.md | 51 ++++ .../lint/AndroidFrameworkIssueRegistry.kt | 47 +++ .../lint/CallingIdentityTokenDetector.kt | 266 +++++++++++++++-- .../lint/CallingIdentityTokenIssueRegistry.kt | 272 ------------------ .../lint/CallingIdentityTokenDetectorTest.kt | 13 +- 5 files changed, 342 insertions(+), 307 deletions(-) create mode 100644 tools/lint/README.md create mode 100644 tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt delete mode 100644 tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenIssueRegistry.kt diff --git a/tools/lint/README.md b/tools/lint/README.md new file mode 100644 index 0000000000000..d661bc4bbadd0 --- /dev/null +++ b/tools/lint/README.md @@ -0,0 +1,51 @@ +# Android Framework Lint Checker + +Custom lint checks written here are going to be executed for modules that opt in to those (e.g. any +`services.XXX` module) and results will be automatically reported on CLs on gerrit. + +## How to add new lint checks + +1. Write your detector with its issues and put it into + `checks/src/main/java/com/google/android/lint`. +2. Add your detector's issues into `AndroidFrameworkIssueRegistry`'s `issues` field. +3. Write unit tests for your detector in one file and put it into + `checks/test/java/com/google/android/lint`. +4. Done! Your lint checks should be applied in lint report builds for modules that include + `AndroidFrameworkLintChecker`. + +## How to run lint against your module + +1. Add the following `lint` attribute to the module definition, e.g. `services.autofill`: +``` +java_library_static { + name: "services.autofill", + ... + lint: { + extra_check_modules: ["AndroidFrameworkLintChecker"], + }, +} +``` +2. Run the following command to verify that the report is being correctly built: +``` +m out/soong/.intermediates/frameworks/base/services/autofill/services.autofill/android_common/lint/lint-report.html +``` + (Lint report can be found in the same path, i.e. `out/../lint-report.html`) +3. Now lint issues should appear on gerrit! + +**Notes:** + +- Lint report will not be produced if you just build the module, i.e. `m services.autofill` will not + build the lint report. +- If you want to build lint reports for more than 1 module and they depend on a common module, e.g. + `platform_service_defaults`, you can add the `lint` property to that common module instead of + adding it in every module. + +## Documentation + +- [go/android-security-lint-checks](http://go/android-security-lint-checks) - presentation about + this module +- [Android Lint Docs](http://googlesamples.github.io/android-custom-lint-rules/) +- [Android Lint source files](https://source.corp.google.com/studio-main/tools/base/lint/libs/lint-api/src/main/java/com/android/tools/lint/) +- [PSI source files](https://github.com/JetBrains/intellij-community/tree/master/java/java-psi-api/src/com/intellij/psi) +- [UAST source files](https://upsource.jetbrains.com/idea-ce/structure/idea-ce-7b9b8cc138bbd90aec26433f82cd2c6838694003/uast/uast-common/src/org/jetbrains/uast) +- [IntelliJ plugin for viewing PSI tree of files](https://plugins.jetbrains.com/plugin/227-psiviewer) diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt b/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt new file mode 100644 index 0000000000000..5736a8082ca3d --- /dev/null +++ b/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt @@ -0,0 +1,47 @@ +/* + * Copyright (C) 2021 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.android.lint + +import com.android.tools.lint.client.api.IssueRegistry +import com.android.tools.lint.client.api.Vendor +import com.android.tools.lint.detector.api.CURRENT_API +import com.google.auto.service.AutoService + +@AutoService(IssueRegistry::class) +@Suppress("UnstableApiUsage") +class AndroidFrameworkIssueRegistry : IssueRegistry() { + override val issues = listOf( + CallingIdentityTokenDetector.ISSUE_UNUSED_TOKEN, + CallingIdentityTokenDetector.ISSUE_NON_FINAL_TOKEN, + CallingIdentityTokenDetector.ISSUE_NESTED_CLEAR_IDENTITY_CALLS, + CallingIdentityTokenDetector.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, + CallingIdentityTokenDetector.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, + CallingIdentityTokenDetector.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY + ) + + override val api: Int + get() = CURRENT_API + + override val minApi: Int + get() = 8 + + override val vendor: Vendor = Vendor( + vendorName = "Android", + feedbackUrl = "http://b/issues/new?component=315013", + contact = "brufino@google.com" + ) +} diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt index b5cab99b4e527..2236a08b87cfb 100644 --- a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt +++ b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt @@ -16,24 +16,16 @@ package com.google.android.lint -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_NESTED_CLEAR_IDENTITY_CALLS -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_NON_FINAL_TOKEN -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_UNUSED_TOKEN -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageClearIdentityCallNotFollowedByTryFinally -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNestedClearIdentityCallsPrimary -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNestedClearIdentityCallsSecondary -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageNonFinalToken -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageRestoreIdentityCallNotInFinallyBlock -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageUnusedToken -import com.google.android.lint.CallingIdentityTokenIssueRegistry.Companion.getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity import com.android.tools.lint.client.api.UElementHandler +import com.android.tools.lint.detector.api.Category import com.android.tools.lint.detector.api.Context import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Implementation +import com.android.tools.lint.detector.api.Issue import com.android.tools.lint.detector.api.JavaContext import com.android.tools.lint.detector.api.Location +import com.android.tools.lint.detector.api.Scope +import com.android.tools.lint.detector.api.Severity import com.android.tools.lint.detector.api.SourceCodeScanner import com.intellij.psi.PsiMethod import com.intellij.psi.search.PsiSearchScopeUtil @@ -54,21 +46,6 @@ import org.jetbrains.uast.getParentOfType */ @Suppress("UnstableApiUsage") class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { - private companion object { - const val CLASS_BINDER = "android.os.Binder" - const val CLASS_USER_HANDLE = "android.os.UserHandle" - - @JvmField - val callerAwareMethods = listOf( - Method.BINDER_GET_CALLING_PID, - Method.BINDER_GET_CALLING_UID, - Method.BINDER_GET_CALLING_UID_OR_THROW, - Method.BINDER_GET_CALLING_USER_HANDLE, - Method.USER_HANDLE_GET_CALLING_APP_ID, - Method.USER_HANDLE_GET_CALLING_USER_ID - ) - } - /** Map of */ private val tokensMap = mutableMapOf() @@ -357,4 +334,237 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { val location: Location, val finallyBlock: UElement? ) + + companion object { + const val CLASS_BINDER = "android.os.Binder" + const val CLASS_USER_HANDLE = "android.os.UserHandle" + + private val callerAwareMethods = listOf( + Method.BINDER_GET_CALLING_PID, + Method.BINDER_GET_CALLING_UID, + Method.BINDER_GET_CALLING_UID_OR_THROW, + Method.BINDER_GET_CALLING_USER_HANDLE, + Method.USER_HANDLE_GET_CALLING_APP_ID, + Method.USER_HANDLE_GET_CALLING_USER_ID + ) + + /** Issue: unused token from Binder.clearCallingIdentity() */ + @JvmField + val ISSUE_UNUSED_TOKEN: Issue = Issue.create( + id = "UnusedTokenOfOriginalCallingIdentity", + briefDescription = "Unused token of Binder.clearCallingIdentity()", + explanation = """ + You cleared the original calling identity with \ + `Binder.clearCallingIdentity()`, but have not used the returned token to \ + restore the identity. + + Call `Binder.restoreCallingIdentity(token)` in the `finally` block, at the end \ + of the method or when you need to restore the identity. + + `token` is the result of `Binder.clearCallingIdentity()` + """, + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + private fun getIncidentMessageUnusedToken(variableName: String) = "`$variableName` has " + + "not been used to restore the calling identity. Introduce a `try`-`finally` " + + "after the declaration and call `Binder.restoreCallingIdentity($variableName)` " + + "in `finally` or remove `$variableName`." + + /** Issue: non-final token from Binder.clearCallingIdentity() */ + @JvmField + val ISSUE_NON_FINAL_TOKEN: Issue = Issue.create( + id = "NonFinalTokenOfOriginalCallingIdentity", + briefDescription = "Non-final token of Binder.clearCallingIdentity()", + explanation = """ + You cleared the original calling identity with \ + `Binder.clearCallingIdentity()`, but have not made the returned token `final`. + + The token should be `final` in order to prevent it from being overwritten, \ + which can cause problems when restoring the identity with \ + `Binder.restoreCallingIdentity(token)`. + """, + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + private fun getIncidentMessageNonFinalToken(variableName: String) = "`$variableName` is " + + "a non-final token from `Binder.clearCallingIdentity()`. Add `final` keyword to " + + "`$variableName`." + + /** Issue: nested calls of Binder.clearCallingIdentity() */ + @JvmField + val ISSUE_NESTED_CLEAR_IDENTITY_CALLS: Issue = Issue.create( + id = "NestedClearCallingIdentityCalls", + briefDescription = "Nested calls of Binder.clearCallingIdentity()", + explanation = """ + You cleared the original calling identity with \ + `Binder.clearCallingIdentity()` twice without restoring identity with the \ + result of the first call. + + Make sure to restore the identity after each clear identity call. + """, + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + private fun getIncidentMessageNestedClearIdentityCallsPrimary( + firstCallVariableName: String, + secondCallVariableName: String + ): String = "The calling identity has already been cleared and returned into " + + "`$firstCallVariableName`. Move `$secondCallVariableName` declaration after " + + "restoring the calling identity with " + + "`Binder.restoreCallingIdentity($firstCallVariableName)`." + + private fun getIncidentMessageNestedClearIdentityCallsSecondary( + firstCallVariableName: String + ): String = "Location of the `$firstCallVariableName` declaration." + + /** Issue: Binder.clearCallingIdentity() is not followed by `try-finally` statement */ + @JvmField + val ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY: Issue = Issue.create( + id = "ClearIdentityCallNotFollowedByTryFinally", + briefDescription = "Binder.clearCallingIdentity() is not followed by try-finally " + + "statement", + explanation = """ + You cleared the original calling identity with \ + `Binder.clearCallingIdentity()`, but the next statement is not a `try` \ + statement. + + Use the following pattern for running operations with your own identity: + + ``` + final long token = Binder.clearCallingIdentity(); + try { + // Code using your own identity + } finally { + Binder.restoreCallingIdentity(token); + } + ``` + + Any calls/operations between `Binder.clearCallingIdentity()` and `try` \ + statement risk throwing an exception without doing a safe and unconditional \ + restore of the identity with `Binder.restoreCallingIdentity()` as an immediate \ + child of the `finally` block. If you do not follow the pattern, you may run \ + code with your identity that was originally intended to run with the calling \ + application's identity. + """, + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + private fun getIncidentMessageClearIdentityCallNotFollowedByTryFinally( + variableName: String + ): String = "You cleared the calling identity and returned the result into " + + "`$variableName`, but the next statement is not a `try`-`finally` statement. " + + "Define a `try`-`finally` block after `$variableName` declaration to ensure a " + + "safe restore of the calling identity by calling " + + "`Binder.restoreCallingIdentity($variableName)` and making it an immediate child " + + "of the `finally` block." + + /** Issue: Binder.restoreCallingIdentity() is not in finally block */ + @JvmField + val ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK: Issue = Issue.create( + id = "RestoreIdentityCallNotInFinallyBlock", + briefDescription = "Binder.restoreCallingIdentity() is not in finally block", + explanation = """ + You are restoring the original calling identity with \ + `Binder.restoreCallingIdentity()`, but the call is not an immediate child of \ + the `finally` block of the `try` statement. + + Use the following pattern for running operations with your own identity: + + ``` + final long token = Binder.clearCallingIdentity(); + try { + // Code using your own identity + } finally { + Binder.restoreCallingIdentity(token); + } + ``` + + If you do not surround the code using your identity with the `try` statement \ + and call `Binder.restoreCallingIdentity()` as an immediate child of the \ + `finally` block, you may run code with your identity that was originally \ + intended to run with the calling application's identity. + """, + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + private fun getIncidentMessageRestoreIdentityCallNotInFinallyBlock( + variableName: String + ): String = "`Binder.restoreCallingIdentity($variableName)` is not an immediate child of " + + "the `finally` block of the try statement after `$variableName` declaration. " + + "Surround the call with `finally` block and call it unconditionally." + + /** Issue: Use of caller-aware methods after Binder.clearCallingIdentity() */ + @JvmField + val ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY: Issue = Issue.create( + id = "UseOfCallerAwareMethodsWithClearedIdentity", + briefDescription = "Use of caller-aware methods after " + + "Binder.clearCallingIdentity()", + explanation = """ + You cleared the original calling identity with \ + `Binder.clearCallingIdentity()`, but used one of the methods below before \ + restoring the identity. These methods will use your own identity instead of \ + the caller's identity, so if this is expected replace them with methods that \ + explicitly query your own identity such as `Process.myUid()`, \ + `Process.myPid()` and `UserHandle.myUserId()`, otherwise move those methods \ + out of the `Binder.clearCallingIdentity()` / `Binder.restoreCallingIdentity()` \ + section. + + ``` + Binder.getCallingPid() + Binder.getCallingUid() + Binder.getCallingUidOrThrow() + Binder.getCallingUserHandle() + UserHandle.getCallingAppId() + UserHandle.getCallingUserId() + ``` + """, + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + private fun getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity( + variableName: String, + methodName: String + ): String = "You cleared the original identity with `Binder.clearCallingIdentity()` " + + "and returned into `$variableName`, so `$methodName` will be using your own " + + "identity instead of the caller's. Either explicitly query your own identity or " + + "move it after restoring the identity with " + + "`Binder.restoreCallingIdentity($variableName)`." + } } diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenIssueRegistry.kt b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenIssueRegistry.kt deleted file mode 100644 index 8a747b64c3a4f..0000000000000 --- a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenIssueRegistry.kt +++ /dev/null @@ -1,272 +0,0 @@ -/* - * Copyright (C) 2021 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.android.lint - -import com.android.tools.lint.client.api.IssueRegistry -import com.android.tools.lint.client.api.Vendor -import com.android.tools.lint.detector.api.CURRENT_API -import com.android.tools.lint.detector.api.Category -import com.android.tools.lint.detector.api.Implementation -import com.android.tools.lint.detector.api.Issue -import com.android.tools.lint.detector.api.Scope -import com.android.tools.lint.detector.api.Severity -import com.google.auto.service.AutoService - -@AutoService(IssueRegistry::class) -@Suppress("UnstableApiUsage") -class CallingIdentityTokenIssueRegistry : IssueRegistry() { - override val issues = listOf( - ISSUE_UNUSED_TOKEN, - ISSUE_NON_FINAL_TOKEN, - ISSUE_NESTED_CLEAR_IDENTITY_CALLS, - ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, - ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, - ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY - ) - - override val api: Int - get() = CURRENT_API - - override val minApi: Int - get() = 8 - - override val vendor: Vendor = Vendor( - vendorName = "Android", - feedbackUrl = "http://b/issues/new?component=315013", - contact = "brufino@google.com" - ) - - companion object { - /** Issue: unused token from Binder.clearCallingIdentity() */ - @JvmField - val ISSUE_UNUSED_TOKEN: Issue = Issue.create( - id = "UnusedTokenOfOriginalCallingIdentity", - briefDescription = "Unused token of Binder.clearCallingIdentity()", - explanation = """ - You cleared the original calling identity with \ - `Binder.clearCallingIdentity()`, but have not used the returned token to \ - restore the identity. - - Call `Binder.restoreCallingIdentity(token)` in the `finally` block, at the end \ - of the method or when you need to restore the identity. - - `token` is the result of `Binder.clearCallingIdentity()` - """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - - fun getIncidentMessageUnusedToken(variableName: String) = "`$variableName` has not been " + - "used to restore the calling identity. Introduce a `try`-`finally` after the " + - "declaration and call `Binder.restoreCallingIdentity($variableName)` in " + - "`finally` or remove `$variableName`." - - /** Issue: non-final token from Binder.clearCallingIdentity() */ - @JvmField - val ISSUE_NON_FINAL_TOKEN: Issue = Issue.create( - id = "NonFinalTokenOfOriginalCallingIdentity", - briefDescription = "Non-final token of Binder.clearCallingIdentity()", - explanation = """ - You cleared the original calling identity with \ - `Binder.clearCallingIdentity()`, but have not made the returned token `final`. - - The token should be `final` in order to prevent it from being overwritten, \ - which can cause problems when restoring the identity with \ - `Binder.restoreCallingIdentity(token)`. - """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - - fun getIncidentMessageNonFinalToken(variableName: String) = "`$variableName` is a " + - "non-final token from `Binder.clearCallingIdentity()`. Add `final` keyword to " + - "`$variableName`." - - /** Issue: nested calls of Binder.clearCallingIdentity() */ - @JvmField - val ISSUE_NESTED_CLEAR_IDENTITY_CALLS: Issue = Issue.create( - id = "NestedClearCallingIdentityCalls", - briefDescription = "Nested calls of Binder.clearCallingIdentity()", - explanation = """ - You cleared the original calling identity with \ - `Binder.clearCallingIdentity()` twice without restoring identity with the \ - result of the first call. - - Make sure to restore the identity after each clear identity call. - """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - - fun getIncidentMessageNestedClearIdentityCallsPrimary( - firstCallVariableName: String, - secondCallVariableName: String - ): String = "The calling identity has already been cleared and returned into " + - "`$firstCallVariableName`. Move `$secondCallVariableName` declaration after " + - "restoring the calling identity with " + - "`Binder.restoreCallingIdentity($firstCallVariableName)`." - - fun getIncidentMessageNestedClearIdentityCallsSecondary( - firstCallVariableName: String - ): String = "Location of the `$firstCallVariableName` declaration." - - /** Issue: Binder.clearCallingIdentity() is not followed by `try-finally` statement */ - @JvmField - val ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY: Issue = Issue.create( - id = "ClearIdentityCallNotFollowedByTryFinally", - briefDescription = "Binder.clearCallingIdentity() is not followed by try-finally " + - "statement", - explanation = """ - You cleared the original calling identity with \ - `Binder.clearCallingIdentity()`, but the next statement is not a `try` \ - statement. - - Use the following pattern for running operations with your own identity: - - ``` - final long token = Binder.clearCallingIdentity(); - try { - // Code using your own identity - } finally { - Binder.restoreCallingIdentity(token); - } - ``` - - Any calls/operations between `Binder.clearCallingIdentity()` and `try` \ - statement risk throwing an exception without doing a safe and unconditional \ - restore of the identity with `Binder.restoreCallingIdentity()` as an immediate \ - child of the `finally` block. If you do not follow the pattern, you may run \ - code with your identity that was originally intended to run with the calling \ - application's identity. - """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - - fun getIncidentMessageClearIdentityCallNotFollowedByTryFinally( - variableName: String - ): String = "You cleared the calling identity and returned the result into " + - "`$variableName`, but the next statement is not a `try`-`finally` statement. " + - "Define a `try`-`finally` block after `$variableName` declaration to ensure a " + - "safe restore of the calling identity by calling " + - "`Binder.restoreCallingIdentity($variableName)` and making it an immediate child " + - "of the `finally` block." - - /** Issue: Binder.restoreCallingIdentity() is not in finally block */ - @JvmField - val ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK: Issue = Issue.create( - id = "RestoreIdentityCallNotInFinallyBlock", - briefDescription = "Binder.restoreCallingIdentity() is not in finally block", - explanation = """ - You are restoring the original calling identity with \ - `Binder.restoreCallingIdentity()`, but the call is not an immediate child of \ - the `finally` block of the `try` statement. - - Use the following pattern for running operations with your own identity: - - ``` - final long token = Binder.clearCallingIdentity(); - try { - // Code using your own identity - } finally { - Binder.restoreCallingIdentity(token); - } - ``` - - If you do not surround the code using your identity with the `try` statement \ - and call `Binder.restoreCallingIdentity()` as an immediate child of the \ - `finally` block, you may run code with your identity that was originally \ - intended to run with the calling application's identity. - """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - - fun getIncidentMessageRestoreIdentityCallNotInFinallyBlock(variableName: String): String = - "`Binder.restoreCallingIdentity($variableName)` is not an immediate child of the " + - "`finally` block of the try statement after `$variableName` declaration. " + - "Surround the call with `finally` block and call it unconditionally." - - /** Issue: Use of caller-aware methods after Binder.clearCallingIdentity() */ - @JvmField - val ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY: Issue = Issue.create( - id = "UseOfCallerAwareMethodsWithClearedIdentity", - briefDescription = "Use of caller-aware methods after " + - "Binder.clearCallingIdentity()", - explanation = """ - You cleared the original calling identity with \ - `Binder.clearCallingIdentity()`, but used one of the methods below before \ - restoring the identity. These methods will use your own identity instead of \ - the caller's identity, so if this is expected replace them with methods that \ - explicitly query your own identity such as `Process.myUid()`, \ - `Process.myPid()` and `UserHandle.myUserId()`, otherwise move those methods \ - out of the `Binder.clearCallingIdentity()` / `Binder.restoreCallingIdentity()` \ - section. - - ``` - Binder.getCallingPid() - Binder.getCallingUid() - Binder.getCallingUidOrThrow() - Binder.getCallingUserHandle() - UserHandle.getCallingAppId() - UserHandle.getCallingUserId() - ``` - """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - - fun getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity( - variableName: String, - methodName: String - ): String = "You cleared the original identity with `Binder.clearCallingIdentity()` " + - "and returned into `$variableName`, so `$methodName` will be using your own " + - "identity instead of the caller's. Either explicitly query your own identity or " + - "move it after restoring the identity with " + - "`Binder.restoreCallingIdentity($variableName)`." - } -} diff --git a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt index 8d12050cae1a4..0532c2013c874 100644 --- a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt +++ b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt @@ -26,13 +26,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { override fun getDetector(): Detector = CallingIdentityTokenDetector() override fun getIssues(): List = listOf( - CallingIdentityTokenIssueRegistry.ISSUE_UNUSED_TOKEN, - CallingIdentityTokenIssueRegistry.ISSUE_NON_FINAL_TOKEN, - CallingIdentityTokenIssueRegistry.ISSUE_NESTED_CLEAR_IDENTITY_CALLS, - CallingIdentityTokenIssueRegistry.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, - CallingIdentityTokenIssueRegistry - .ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, - CallingIdentityTokenIssueRegistry.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY + CallingIdentityTokenDetector.ISSUE_UNUSED_TOKEN, + CallingIdentityTokenDetector.ISSUE_NON_FINAL_TOKEN, + CallingIdentityTokenDetector.ISSUE_NESTED_CLEAR_IDENTITY_CALLS, + CallingIdentityTokenDetector.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, + CallingIdentityTokenDetector.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, + CallingIdentityTokenDetector.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY ) /** No issue scenario */ From d92d1e4ca20753d43e6806bbefe7074f596660d1 Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Thu, 1 Jul 2021 15:41:33 +0000 Subject: [PATCH 3/4] Enable Android Lint tests for AndroidFrameworkLintChecker Create AndroidFrameworkLintCheckerTest module and add .allowMissingSdk(true) to all tests to enable tests running on gerrit. "unit_test: true" marks the module as a unit test target which runs it on the host without a device and it automatically gets onboarded in the presubmit. Bug: 157626959 Test: atest AndroidFrameworkLintCheckerTest --host Change-Id: I66c6e331e72dfca45d341a49eb43fbd73ccd28ff --- tools/lint/Android.bp | 26 +++++++++---------- .../lint/CallingIdentityTokenDetectorTest.kt | 3 +++ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/tools/lint/Android.bp b/tools/lint/Android.bp index dcbc32b4e2888..17547ef8b561a 100644 --- a/tools/lint/Android.bp +++ b/tools/lint/Android.bp @@ -31,16 +31,16 @@ java_library_host { ], } -// TODO: (b/162368644) Implement these (working in gradle) Kotlin Tests to run on Soong -//java_test_host { -// name: "AndroidFrameworkLintCheckerTest", -// srcs: [ -// "checks/src/test/java/**/*.kt", -// "checks/src/main/java/**/*.kt", -// ], -// plugins: ["auto_service_plugin"], -// static_libs: [ -// "auto_service_annotations", -// "lint_api", -// ], -//} +java_test_host { + name: "AndroidFrameworkLintCheckerTest", + srcs: ["checks/src/test/java/**/*.kt"], + static_libs: [ + "AndroidFrameworkLintChecker", + "junit", + "lint", + "lint_tests", + ], + test_options: { + unit_test: true, + }, +} diff --git a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt index 0532c2013c874..8f67555ec1b72 100644 --- a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt +++ b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt @@ -18,6 +18,7 @@ package com.google.android.lint import com.android.tools.lint.checks.infrastructure.LintDetectorTest import com.android.tools.lint.checks.infrastructure.TestFile +import com.android.tools.lint.checks.infrastructure.TestLintTask import com.android.tools.lint.detector.api.Detector import com.android.tools.lint.detector.api.Issue @@ -34,6 +35,8 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { CallingIdentityTokenDetector.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY ) + override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) + /** No issue scenario */ fun testDoesNotDetectIssuesInCorrectScenario() { From 0dee2ba23a07ffbf584da7a2945259ce7918e47b Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Mon, 19 Jul 2021 16:45:06 +0000 Subject: [PATCH 4/4] [CallingIdentityTokenDetector] Support methods with implicit receiver Modified CallingIdentityTokenDetector to visit UCallExpressions instead of UQualifiedReferenceExpression. UQualifiedReferenceExpression has a structure of receiver.selector which applies to method calls that use the class reference, however, standalone methods will be ignored, i.e. method(). It is required to consider standalone method calls because the class may be a child of Binder and it can call its own method without any class reference. Bug: 194085454 Test: atest AndroidFrameworkLintCheckerTest --host Change-Id: I2b40afb747664cf4a36e441eaba1ecd64960e3b1 --- .../lint/CallingIdentityTokenDetector.kt | 29 ++--- .../lint/CallingIdentityTokenDetectorTest.kt | 109 +++++++++++------- 2 files changed, 82 insertions(+), 56 deletions(-) diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt index 2236a08b87cfb..c133226e49f72 100644 --- a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt +++ b/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt @@ -27,7 +27,6 @@ import com.android.tools.lint.detector.api.Location import com.android.tools.lint.detector.api.Scope import com.android.tools.lint.detector.api.Severity import com.android.tools.lint.detector.api.SourceCodeScanner -import com.intellij.psi.PsiMethod import com.intellij.psi.search.PsiSearchScopeUtil import com.intellij.psi.search.SearchScope import org.jetbrains.uast.UBlockExpression @@ -35,10 +34,11 @@ import org.jetbrains.uast.UCallExpression import org.jetbrains.uast.UDeclarationsExpression import org.jetbrains.uast.UElement import org.jetbrains.uast.ULocalVariable -import org.jetbrains.uast.UQualifiedReferenceExpression import org.jetbrains.uast.USimpleNameReferenceExpression import org.jetbrains.uast.UTryExpression import org.jetbrains.uast.getParentOfType +import org.jetbrains.uast.getQualifiedParentOrThis +import org.jetbrains.uast.getUCallExpression /** * Lint Detector that finds issues with improper usages of the token returned by @@ -50,7 +50,7 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { private val tokensMap = mutableMapOf() override fun getApplicableUastTypes(): List> = - listOf(ULocalVariable::class.java, UQualifiedReferenceExpression::class.java) + listOf(ULocalVariable::class.java, UCallExpression::class.java) override fun createUastHandler(context: JavaContext): UElementHandler = TokenUastHandler(context) @@ -87,7 +87,7 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { * - Stores token variable name, scope in the file, location and finally block in tokensMap */ override fun visitLocalVariable(node: ULocalVariable) { - val rhsExpression = node.uastInitializer as? UQualifiedReferenceExpression ?: return + val rhsExpression = node.uastInitializer?.getUCallExpression() ?: return if (!isMethodCall(rhsExpression, Method.BINDER_CLEAR_CALLING_IDENTITY)) return val location = context.getLocation(node as UElement) val variableName = node.getName() @@ -142,13 +142,13 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { } /** - * For every class.method(): + * For every method(): * - Checks use of caller-aware methods issue * For every call of Binder.restoreCallingIdentity(token): * - Checks for restoreCallingIdentity() not in the finally block issue * - Removes token from tokensMap if token is within the scope of the method */ - override fun visitQualifiedReferenceExpression(node: UQualifiedReferenceExpression) { + override fun visitCallExpression(node: UCallExpression) { val token = findFirstTokenInScope(node) if (isCallerAwareMethod(node) && token != null) { context.report( @@ -162,8 +162,7 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { return } if (!isMethodCall(node, Method.BINDER_RESTORE_CALLING_IDENTITY)) return - val selector = node.selector as UCallExpression - val arg = selector.valueArguments[0] as? USimpleNameReferenceExpression ?: return + val arg = node.valueArguments[0] as? USimpleNameReferenceExpression ?: return val variableName = arg.identifier val originalScope = tokensMap[variableName]?.scope ?: return val psi = arg.sourcePsi ?: return @@ -171,11 +170,15 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { // token declaration. If not within the scope, no action is needed because the token is // irrelevant i.e. not in the same scope or was not declared with clearCallingIdentity() if (!PsiSearchScopeUtil.isInScope(originalScope, psi)) return - // We do not report "restore identity call not in finally" issue when there is no + // - We do not report "restore identity call not in finally" issue when there is no // finally block because that case is already handled by "clear identity call not // followed by try-finally" issue + // - UCallExpression can be a child of UQualifiedReferenceExpression, i.e. + // receiver.selector, so to get the call's immediate parent we need to get the topmost + // parent qualified reference expression and access its parent if (tokensMap[variableName]?.finallyBlock != null && - node.uastParent != tokensMap[variableName]?.finallyBlock) { + node.getQualifiedParentOrThis().uastParent != + tokensMap[variableName]?.finallyBlock) { context.report( ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, context.getLocation(node), @@ -185,14 +188,14 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { tokensMap.remove(variableName) } - private fun isCallerAwareMethod(expression: UQualifiedReferenceExpression): Boolean = + private fun isCallerAwareMethod(expression: UCallExpression): Boolean = callerAwareMethods.any { method -> isMethodCall(expression, method) } private fun isMethodCall( - expression: UQualifiedReferenceExpression, + expression: UCallExpression, method: Method ): Boolean { - val psiMethod = expression.resolve() as? PsiMethod ?: return false + val psiMethod = expression.resolve() ?: return false return psiMethod.getName() == method.methodName && context.evaluator.methodMatches( psiMethod, diff --git a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt index 8f67555ec1b72..e1a5c613dee15 100644 --- a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt +++ b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt @@ -45,7 +45,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ package test.pkg; import android.os.Binder; - public class TestClass1 { + public class TestClass1 extends Binder { private void testMethod() { final long token1 = Binder.clearCallingIdentity(); try { @@ -57,9 +57,14 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } finally { android.os.Binder.restoreCallingIdentity(token2); } + final long token3 = clearCallingIdentity(); + try { + } finally { + restoreCallingIdentity(token3); + } } } - """ + """ ).indented(), *stubs ) @@ -75,7 +80,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ package test.pkg; import android.os.Binder; - public class TestClass1 { + public class TestClass1 extends Binder { private void testMethodImported() { final long token1 = Binder.clearCallingIdentity(); try { @@ -88,6 +93,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } finally { } } + private void testMethodChildOfBinder() { + final long token3 = clearCallingIdentity(); + try { + } finally { + } + } } """ ).indented(), @@ -108,7 +119,13 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { remove token2. [UnusedTokenOfOriginalCallingIdentity] final long token2 = android.os.Binder.clearCallingIdentity(); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 0 errors, 2 warnings + src/test/pkg/TestClass1.java:17: Warning: token3 has not been used to \ + restore the calling identity. Introduce a try-finally after the \ + declaration and call Binder.restoreCallingIdentity(token3) in finally or \ + remove token3. [UnusedTokenOfOriginalCallingIdentity] + final long token3 = clearCallingIdentity(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 3 warnings """.addLineContinuation() ) } @@ -234,7 +251,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ package test.pkg; import android.os.Binder; - public class TestClass1 { + public class TestClass1 extends Binder { private void testMethod() { long token1 = Binder.clearCallingIdentity(); try { @@ -246,6 +263,11 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } finally { android.os.Binder.restoreCallingIdentity(token2); } + long token3 = clearCallingIdentity(); + try { + } finally { + restoreCallingIdentity(token3); + } } } """ @@ -265,7 +287,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { [NonFinalTokenOfOriginalCallingIdentity] long token2 = android.os.Binder.clearCallingIdentity(); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 0 errors, 2 warnings + src/test/pkg/TestClass1.java:15: Warning: token3 is a non-final token from \ + Binder.clearCallingIdentity(). Add final keyword to token3. \ + [NonFinalTokenOfOriginalCallingIdentity] + long token3 = clearCallingIdentity(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 3 warnings """.addLineContinuation() ) } @@ -279,16 +306,16 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ package test.pkg; import android.os.Binder; - public class TestClass1 { + public class TestClass1 extends Binder { private void testMethod() { final long token1 = Binder.clearCallingIdentity(); try { final long token2 = android.os.Binder.clearCallingIdentity(); try { - final long token3 = Binder.clearCallingIdentity(); + final long token3 = clearCallingIdentity(); try { } finally { - Binder.restoreCallingIdentity(token3); + restoreCallingIdentity(token3); } } finally { android.os.Binder.restoreCallingIdentity(token2); @@ -316,8 +343,8 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { been cleared and returned into token1. Move token3 declaration after \ restoring the calling identity with Binder.restoreCallingIdentity(token1). \ [NestedClearCallingIdentityCalls] - final long token3 = Binder.clearCallingIdentity(); - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + final long token3 = clearCallingIdentity(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:5: Location of the token1 declaration. 0 errors, 2 warnings """.addLineContinuation() @@ -332,7 +359,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ package test.pkg; import android.os.Binder; - public class TestClass1 { + public class TestClass1 extends Binder{ private void testMethodNoTry() { final long token = Binder.clearCallingIdentity(); Binder.restoreCallingIdentity(token); @@ -346,10 +373,10 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } private void testMethodLocalVariableBetweenClearAndTry() { - final long token = Binder.clearCallingIdentity(), num = 0; + final long token = clearCallingIdentity(), num = 0; try { } finally { - Binder.restoreCallingIdentity(token); + restoreCallingIdentity(token); } } private void testMethodTryCatch() { @@ -398,8 +425,8 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { to ensure a safe restore of the calling identity by calling \ Binder.restoreCallingIdentity(token) and making it an immediate child of \ the finally block. [ClearIdentityCallNotFollowedByTryFinally] - final long token = Binder.clearCallingIdentity(), num = 0; - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + final long token = clearCallingIdentity(), num = 0; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:24: Warning: You cleared the calling identity \ and returned the result into token, but the next statement is not a \ try-finally statement. Define a try-finally block after token declaration \ @@ -429,7 +456,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ package test.pkg; import android.os.Binder; - public class TestClass1 { + public class TestClass1 extends Binder { private void testMethodImported() { final long token = Binder.clearCallingIdentity(); try { @@ -446,10 +473,10 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { android.os.Binder.restoreCallingIdentity(token); } private void testMethodRestoreInCatch() { - final long token = Binder.clearCallingIdentity(); + final long token = clearCallingIdentity(); try { } catch (Exception e) { - Binder.restoreCallingIdentity(token); + restoreCallingIdentity(token); } finally { } } @@ -480,8 +507,8 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { finally block of the try statement after token declaration. Surround the c\ all with finally block and call it unconditionally. \ [RestoreIdentityCallNotInFinallyBlock] - Binder.restoreCallingIdentity(token); - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + restoreCallingIdentity(token); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 3 warnings """.addLineContinuation() ) @@ -493,7 +520,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ package test.pkg; import android.os.Binder; - public class TestClass1 { + public class TestClass1 extends Binder { private void testMethodOutsideFinally() { final long token1 = Binder.clearCallingIdentity(); try { @@ -525,14 +552,10 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } } - final long token2 = android.os.Binder.clearCallingIdentity(); + final long token2 = clearCallingIdentity(); try { } finally { - { - { - android.os.Binder.restoreCallingIdentity(token2); - } - } + if (true) restoreCallingIdentity(token2); } } } @@ -564,13 +587,13 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { [RestoreIdentityCallNotInFinallyBlock] Binder.restoreCallingIdentity(token1); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - src/test/pkg/TestClass1.java:40: Warning: \ + src/test/pkg/TestClass1.java:38: Warning: \ Binder.restoreCallingIdentity(token2) is not an immediate child of the \ finally block of the try statement after token2 declaration. Surround the \ call with finally block and call it unconditionally. \ [RestoreIdentityCallNotInFinallyBlock] - android.os.Binder.restoreCallingIdentity(token2); - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + if (true) restoreCallingIdentity(token2); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 4 warnings """.addLineContinuation() ) @@ -617,7 +640,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { """ src/test/pkg/TestClass1.java:8: Warning: You cleared the original identity \ with Binder.clearCallingIdentity() and returned into token, so \ - Binder.getCallingPid() will be using your own identity instead of the \ + getCallingPid() will be using your own identity instead of the \ caller's. Either explicitly query your own identity or move it after \ restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -625,7 +648,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:9: Warning: You cleared the original identity \ with Binder.clearCallingIdentity() and returned into token, so \ - android.os.Binder.getCallingPid() will be using your own identity instead \ + getCallingPid() will be using your own identity instead \ of the caller's. Either explicitly query your own identity or move it \ after restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -633,7 +656,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:10: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - Binder.getCallingUid() will be using your own identity instead of the \ + getCallingUid() will be using your own identity instead of the \ caller's. Either explicitly query your own identity or move it after \ restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -641,7 +664,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:11: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - android.os.Binder.getCallingUid() will be using your own identity instead \ + getCallingUid() will be using your own identity instead \ of the caller's. Either explicitly query your own identity or move it \ after restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -649,7 +672,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:12: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - Binder.getCallingUidOrThrow() will be using your own identity instead of \ + getCallingUidOrThrow() will be using your own identity instead of \ the caller's. Either explicitly query your own identity or move it after \ restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -657,7 +680,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:13: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - android.os.Binder.getCallingUidOrThrow() will be using your own identity \ + getCallingUidOrThrow() will be using your own identity \ instead of the caller's. Either explicitly query your own identity or move \ it after restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -665,7 +688,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:14: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - Binder.getCallingUserHandle() will be using your own identity instead of \ + getCallingUserHandle() will be using your own identity instead of \ the caller's. Either explicitly query your own identity or move it after \ restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -673,7 +696,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:15: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - android.os.Binder.getCallingUserHandle() will be using your own identity \ + getCallingUserHandle() will be using your own identity \ instead of the caller's. Either explicitly query your own identity or move \ it after restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -681,7 +704,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:17: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - UserHandle.getCallingAppId() will be using your own identity instead of \ + getCallingAppId() will be using your own identity instead of \ the caller's. Either explicitly query your own identity or move it after \ restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -689,7 +712,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:18: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - android.os.UserHandle.getCallingAppId() will be using your own identity \ + getCallingAppId() will be using your own identity \ instead of the caller's. Either explicitly query your own identity or move \ it after restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -697,7 +720,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:19: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - UserHandle.getCallingUserId() will be using your own identity instead of \ + getCallingUserId() will be using your own identity instead of \ the caller's. Either explicitly query your own identity or move it after \ restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity] @@ -705,7 +728,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/test/pkg/TestClass1.java:20: Warning: You cleared the original \ identity with Binder.clearCallingIdentity() and returned into token, so \ - android.os.UserHandle.getCallingUserId() will be using your own identity \ + getCallingUserId() will be using your own identity \ instead of the caller's. Either explicitly query your own identity or move \ it after restoring the identity with Binder.restoreCallingIdentity(token). \ [UseOfCallerAwareMethodsWithClearedIdentity]