Recent work has been using Error Prone rules and annotations to
reflect the current state of permission enforcement across the
Bluetooth stack, and we're now in a position were we can add new
permission enforcement that had been missing.
We've currently standardized on saying that APIs that return device
or Bluetooth state information (without sharing details about any
particular remote Bluetooth device) do not need to be permission
protected.
Bug: 183626724
Test: ./build/soong/soong_ui.bash --make-mode Bluetooth RUN_ERROR_PRONE=true
Change-Id: I53ac7a4fe1dea57316048c3cac4fa237b6ba3d38
This change adds a "BluetoothPermissionChecker" that ensures that
all Bluetooth permission annotations are consistent. In addition, it
verifies that all Bluetooth public APIs have been audited to be
permission protected where relevant.
We've currently standardized on saying that APIs that return device
or Bluetooth state information (without sharing details about any
particular remote Bluetooth device) do not need to be permission
protected.
This change is only annotations and has no behavior changes.
Bug: 183626724
Test: ./build/soong/soong_ui.bash --make-mode Bluetooth RUN_ERROR_PRONE=true
Change-Id: Ie80b15b058359bf1e9a6ee881b89cb3e5b584ca1
We've had @RequiresPermission annotations across public APIs for many
years, but we've never built out the tooling to validate that the
service implementations actually enforced those permissions.
This change adds an Error Prone checker that does bi-directional
validation of these annotations, confirming that AIDL implementations
enforce the permissions, and that AIDL callers carry those
annotations through any indirect call-paths.
Currently, enforcement validation is best-effort, since it assumes
that any enforcement referencing the annotated permissions is enough
to pass; it doesn't attempt any code flow analysis. It also doesn't
understand concepts like Binder.clearCallingIdentity().
To begin using this checker, simply begin annotating your AIDL files
using a strategy like this:
@JavaPassthrough(annotation="@android.annotation.RequiresPermission(android.Manifest.permission.BLUETOOTH_PRIVILEGED)")
void aidlMethod();
Bug: 183626724
Test: atest error_prone_android_framework_test:RequiresPermissionCheckerTest
Change-Id: I26a872f07ab13931c241cbb02ff7228edf7dc3b9
* unclear why the test mapping file is not running with host:true since
it's a java_test_host
* See go/a-unit-tests for full details
The test will continue to be part of presubmit but running as part of
the unit tests suite instead of TEST_MAPPING.
For unit tests, explicit test_mapping declaration isn't needed to run.
Change-Id: I9c0b93d6e6eb0b97ff96b1ac7c02d4525b762bd6
Test: presubmit
Bug: 180736967
Merged-In: I9c0b93d6e6eb0b97ff96b1ac7c02d4525b762bd6
Bug: 174932174
Test: I solemnly swear I tested this conflict resolution.
Exempt-From-Owner-Approval: refactoring with team leads buy-in
Change-Id: I9262a08ffc1ccede8e519d0eed90ed2bfcf0232c
As general background, OWNERS files expedite code reviews by helping
code authors quickly find relevant reviewers, and they also ensure
that stakeholders are involved in code changes in their areas.
Some teams under frameworks/base/ have been using OWNERS files
successfully for many years, and we're ready to expand them to cover
more areas. Here's the historical coverage statistics for the last
two years of changes before these new OWNERS changes land:
-- 56% of changes are fully covered by OWNERS
-- 17% of changes are partially covered by OWNERS
-- 25% of changes have no OWNERS coverage
Working closely with team leads, we've now identified clear OWNERS on
a per-package basis, and we're using "include" directives whenever
possible to to simplify future maintenance. With this extensive
effort, we've now improved our coverage as follows:
-- 98% of changes are fully covered by OWNERS
-- 1% of changes are partially covered by OWNERS
-- 1% of changes have no OWNERS coverage
This specific change is automatically generated by a script from
detailed ownership information confirmed by team leads.
Bug: 174932174
Test: manual
Exempt-From-Owner-Approval: refactoring with team leads buy-in
Merged-In: I9789c97c1de8e5d962b48c29c57d82fe83729eba
Change-Id: I9789c97c1de8e5d962b48c29c57d82fe83729eba
Previous changes have applied mechanical refactorings, but this change
hand-migrates the remaining logic which was too complex to identify.
This change should have no behavior change; famous last words.
Bug: 171832118
Test: manual
Exempt-From-Owner-Approval: trivial refactoring
Change-Id: If2d4b1a354481a83edb4bbcfa22f690b7f5e97c3
Since we have over 100 unique schemas across the OS, it would be
incredibly tedious to try migrating all that logic to use the more
efficient TypedXmlSerializer and TypedXmlPullParser interfaces.
To aid this migration process, this change adds Refaster templates
that offer to refactor matching existing code in a no-op fashion.
This change also upgrades Error Prone to the latest release.
Bug: 171832118
Test: manual
Change-Id: Ic93a46d646edee98af7d0e2f7891d931fca4825f
Related changes are introducing new TypedXmlSerializer and
TypedXmlPullParser interfaces which offer efficient access to
primitive attributes, and this Error Prone detector helps identify
code that should shift to using those new interfaces.
Bug: 171832118
Test: atest error_prone_android_framework_test
Change-Id: Ic3ca6b96d2b056e6178e407af886bb925a3471c8
This gives all our various new performance-related checkers a
consistent "EfficientFooChecker" naming style.
Bug: 155703208
Test: atest error_prone_android_framework_test
Change-Id: I8809fe46d20357201c8559037b468d717319afb9
One of the highest-traffic users of String.format() is in the
notification code, which uses argument widths. To support these
use-cases, this change adds argument width support, with tests.
Bug: 170978902
Test: atest error_prone_android_framework_test
Exempt-From-Owner-Approval: trivial additions
Change-Id: I8e36d4725a6d0cc896dedc5c457eb5f38486d7b6
It's okay if callers try mixing "static final" values into strings,
since the compiler will inline these to avoid the StringBuilder.
We also expand to catch any arguments that might be dynamically
calculated, such as method invocations.
Identify additional inefficient code patterns:
-- Passing dynamic strings into a StringBuilder, which acquires a
transparent StringBuilder for each append.
-- Using "str += val;" style concatenation, which acquires a
transparent StringBuilder for each append.
-- Using StringBuffer which has synchronization overhead.
Bug: 170978902
Test: atest error_prone_android_framework_test
Change-Id: Ia3758dd55a0e6753b0cc5bc83ae8fe45b6bfde1f
Android offers several efficient alternatives to some upstream
String operations, such as the newly added TextUtils.formatSimple().
This checker also detects and discourages transparent StringBuilder
operations related to Preconditions, where we always pay the cost of
building the failure message string, even in the successful case.
Bug: 170978902
Test: atest error_prone_android_framework_test
Change-Id: I8cef4c50d8b0da3f1e66727dfa724ad44b88963b
Add a mutability flag check for all method calls that create a PendingIntent.
Bug: 160794467
Test: atest error_prone_android_framework_test:com.google.errorprone.bugpatterns.android.PendingIntentMutabilityCheckerTest
Change-Id: I26a51a6dddb2793e9a56e72876f3f9d2aea4e3fb
Android offers several efficient alternatives to some upstream
Collections containers, such as SparseIntArray instead of
Map<Integer, Integer>.
Bug: 155703208
Test: atest error_prone_android_framework_test
Change-Id: I080fd9489fb037391b717901345a905a9753b370
Binder maintains thread-local identity information about any remote
caller, which can be temporarily cleared while performing operations
that need to be handled as the current process. However, it's
important to restore the original remote calling identity after
carefully scoping this work inside a try/finally block, to avoid
obscure security vulnerabilities.
Bug: 155703208
Test: atest error_prone_android_framework_test
Change-Id: I568771a50af27637e4984950dcada2248ce16afe
Two specific managers have their own strategies using indirection to
obtain Context.getUserId(), and we're willing to special case these
to reduce warning noise.
Bug: 115654727, 159626156
Test: atest error_prone_android_framework_test
Change-Id: I290b74f7b6ec17c647f1cbb9ea7995f3fd56b132
Purposefully exclude telephony Binder interfaces, since we know they
always run under the separate AID_RADIO.
Bug: 155703208
Test: atest error_prone_android_framework_test
Change-Id: I3ce87caeb2abe3a7ca01ce10560d02b499ece07d
Because shifting newly written code over to using CompatChanges is
important, this change refines the recently added check and upgrades
it to become a fatal build error.
Bug: 169879376
Test: atest error_prone_android_framework_test
Change-Id: Ic3126518ebaac9995b8f649e44b839de30faa17f
Each SDK level often has dozens of different behavior changes, which
can be difficult for large app developers to adjust to during preview
or beta releases. For this reason, android.app.compat.CompatChanges
was introduced as a new best-practice for adding behavior changes.
During a preview or beta release, developers can temporarily opt-out
of each individual change to aid debugging. This opt-out is only
available during preview of beta releases, and cannot be adjusted on
finalized builds.
Bug: 169879376
Test: atest error_prone_android_framework_test
Change-Id: Ib3b2e2139e084b0fa1bcbb5e89dd55e7ca4bfa00
Parcelable data can be transported in many ways (some of which can be
very inefficient) so this checker guides developers towards using
high-performance best-practices.
Bug: 154436100, 155703208
Test: atest error_prone_android_framework_test
Change-Id: I253b5e1088c9bf9c3cf0d684cf73134f3bbf27ab
Use the AutoValue built from source instead of the prebuilts
from external/dagger2.
Bug: 158870037
Test: m checkbuild
Change-Id: Ibadc0845cbb41f1dd312f38ac45d91e56cbe9a62
Several managers keep an "int mUserId" field which is assigned from
Context.getUserId(), so Binder calls referencing that field are okay.
Also shift to borrowing the "flavor" logic for detecting userId
parameters consistently.
Bug: 115654727, 159626156
Test: atest error_prone_android_framework_test
Change-Id: I9841fdf16f34c08b113e689e74b94f1ede839e2c
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
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
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
Often a permission check in the app's process is an indicative of a
security issue since the app could work around it. Permission checks
should be done on system_server.
This errorprone warning checks for invocations of
Context.checkPermission() in any class inside android.app package and
emits a warning if it finds one. I also added a @SuppressWarnings for
one such call that has a todo and it and seems like an already tracked
workaround. The other call found by the checker is tracked in
b/157548188.
I also found that errorprone was not running for framework-minus-apex,
so I added the plugin to the relevant build rule. Let me know if this is
not the way to go!
Test: build/soong/soong_ui.bash --make-mode framework-minus-apex
RUN_ERROR_PRONE=true
Bug: 157626959
Change-Id: Ieb94f2f43722837c8354ac66474797f4f338ae16
Apps making calls into the system server may end up persisting
internal state or making security decisions based on the perceived
success or failure of a call, or any default values returned. For
this reason, we want to strongly throw when there was trouble with
the transaction.
The rethrowFromSystemServer() method is the best-practice way of
doing this correctly, so that we don't clutter logs with misleading
stack traces, and this checker verifies that best-practice is used.
Disable this check on managers that we know are hosted outside the
system process.
Bug: 155703208
Test: ./build/soong/soong_ui.bash --make-mode framework-minus-apex services RUN_ERROR_PRONE=true
Exempt-From-Owner-Approval: trivial annotations
Change-Id: I04b4daf7c92251a14bcc3ebb1e18cd00f6a7f283
Over the years we've had several obscure bugs related to how SDK level
comparisons are performed, specifically during the window of time
where we've started distributing the "frankenbuild" to developers.
Consider the case where a framework developer shipping release "R"
wants to only grant a specific behavior to modern apps; they could
write this in two different ways:
1. if (targetSdkVersion > Build.VERSION_CODES.Q) {
2. if (targetSdkVersion >= Build.VERSION_CODES.R) {
The safer of these two options is (2), which will ensure that
developers only get the behavior when *both* the app and the
platform concur on the specific SDK level having shipped.
Consider the breakage that would happen with option (1) if we
started shipping APKs that are based on the final R SDK, but are
then installed on earlier preview releases which still consider R
to be CUR_DEVELOPMENT; they'd risk crashing due to behaviors that
were never part of the official R SDK.
Bug: 64412239
Test: ./build/soong/soong_ui.bash --make-mode services RUN_ERROR_PRONE=true
Exempt-From-Owner-Approval: trivial blueprint changes
Change-Id: Ia20181f8602451ac9a719ea488d148e160708592
