| Age | Commit message (Collapse) | Author |
|---|
|
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
|
|
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
|
|
* changes:
Recommend efficient String operations.
Simple alternative to String.format().
|
|
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
|
|
* changes:
Detector to suggest more efficient collections.
Detector for Binder.clearCallingIdentity() bugs.
|
|
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
|
|
* changes:
Slight relaxing of Context.getUserId() checks.
Exclude Telephony Binder interfaces.
|
|
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
|
|
Bug: 162543841
Test: atest ./errorprone/tests/java/com/google/errorprone/bugpatterns/android/UidCheckerTest.java
Change-Id: Id013a6416f687411567fa3d363c917106ab3608e
|
|
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
|
|
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
|
