No production code relies on the mutability. Code in fakeandroid's
AlarmManager added in http://ag/3826144 relies on the mutability,
and backup/internal/PerformInitializeTask sets a (now immutable)
PendingIntent, but fakeandroid uses a different AlarmManager instance
that the real system so these should be unrelated (I think - reviewer,
please double-check my thinking).
Fixes: 170163281
Test: Treehugger
Change-Id: I3dd45cc270c2fd8ab9d4b3402edc6cc5d765fd5a
* changes:
Upgrade AndroidFrameworkBinderIdentity to fatal.
Tighten up Binder.clearCallingIdentity() usage.
Tighten up Binder.clearCallingIdentity() usage.
Tighten up Binder.clearCallingIdentity() usage.
This is a third CL in a chain that adjusts existing malformed code
to follow AndroidFrameworkBinderIdentity best-practices.
Specifically, if a thread clears an identity they need to restore it
to avoid obscure security vulnerabilities. In addition, the relevant
"try" block must start immediately after the identity is cleared to
ensure that its restored if/when any exceptions are thrown.
Bug: 155703208
Test: make
Exempt-From-Owner-Approval: trivial refactoring
Change-Id: I74cb958b68d55a647547aae21baff6ddc364859b
This change is motivated by b/161248425 where the restore times out before all
call logs can be restored by CallLogBackupAgent. The CL increases
restore time limit for agents running in the system process. See the
linked bug below for rationale as to why the change is limited to system
agents only.
Changes in the CL:
* Split timeouts for restore session and agent restore into 2 separate
values in BackupAgentTimeoutParameters, so that the latter can be
changed independently.
* Pass application UID to #getRestoreSessionTimeoutMillis() so that we
adjust the return value based on whether the agent is part of the system.
Bug: 170076589
Test: 1. atest BackupAgentTimeoutParametersTest
2. Populate 7000 call log entries; run backup; clear call history;
run restore and verify it fails without the change and succeeds
with the change.
Change-Id: Id32e34973be380f7206cb9000ca95e6b76bbf8c0
The recently added AndroidFrameworkBinderIdentity Error Prone checker
examines code to ensure that any cleared identities are restored to
avoid obscure security vulnerabilities.
This change is a purely mechanical refactoring that adds the "final"
keyword to the cleared identity to ensure that it's not accidentally
modified before eventually being cleared. Here's the exact command
used to generate this CL:
$ find . -name "*.java" -exec sed -Ei \
's/ (long \w+ = .+?clearCallingIdentity)/ final \1/' \
{} \;
Bug: 155703208
Test: make
Exempt-From-Owner-Approval: trivial refactoring
Change-Id: I832c9d70c3dfcd8d669cf71939d97837becc973a
Context: KeyValueBackupTask's ctor takes a DataChangedJournal instance.
KeyValueBackupTaskTest uses a mock instance to check that remove() is
called. http://ag/12218310 made DataChangedJournal final, breaking the
test. This CL fixes the failure cause by making the class nonfinal.
Note that the test still fails because at least one unrelated failure
cause remains - see http://b/162022005#comment11
In the longer term, we may want to consider splitting DataChangedJournal
into an interface/abstract class + a concrete implementation; there's no
good reason why KeyValueBackupTask and KeyValueBackupReporter need to
depend on specifically the implementation class, when all they're calling
is remove() and toString(). However, the change to make the class final
wasn't important, and reverting it is the easiest / fasted way to unbreak
the test.
Bug: 162022005
Test: atest KeyValueBackupTaskTest
Change-Id: I22b2a5d65f03c4ccdcc718ed3866d37ba9441385
System server should not directly called Settings method such as Settings.Secure.getString()
instead of Settings.Secure.getStringForUser(). The "ForUser" methods allow us to properly
enforce the user's restrictions and it prevents issues like b/163571398.
This CL is part of the effort that changes existing usage of non "ForUser" methods
to using "ForUser" methods. It is supposed to act as a no-op.
BUG: 166312046
Test: builds
Change-Id: I632a243ec916a95437bcd32e5c480ab947eebb4e
Add an API to BackupManager to start restore in migration mode.
Bug: 160407842
Test: atest PerformUnifiedRestoreTaskTest BackupManagerServiceTest
UserBackupManagerServiceTest
Change-Id: I75fdb1c99edc3d11d1264d69e1f20c682d588479
This is just a plain refactoring: the removed methods in the changed
classes were called by default by the new methods in the superclass
(SystemService).
Test: m
Test: atest NotificationManagerServiceTest BackupManagerServiceRoboTest
Fixes: 161943081
Exempt-From-Owner-Approval: refactoring without side-effects
Change-Id: Ifd8df592eb4494cc0922b7e0b2ff20187b8a8b3e
See https://source.android.com/setup/contribute/respectful-code for reference
PerformAdb{Backup,Restore}Task handles backing up to and restoring from
a file. The backup data is encrypted with a randomly generated
encryption key. The encryption key is encrypted with a key that is
derived from a user supplied passphrase.
The key that was used to encrypt and decrypt the backup data used to be
called the "master key", I've renamed it to "encryption key" which
better describes its purpose.
Bug: 161896447
Test: `atest PerformAdbRestoreTaskTest`, `atest TarBackupReaderTest`
and manually tested using adb with the following commands:
`adb backup -all -apk -shared -f backup.ab` and
`adb restore backup.ab`.
Change-Id: I8e6b22a06b2a583440839994f0626d370bdb2e19
With the goal of reducing log spam, print only a summary 'Found stale
backup journal' messages instead of logging within the inner loop.
Previously, over 12k messages could be printed at a time from this
function.
Before this CL:
- a backup was scheduled for each packageName from each stale
journal
- one (or two, if MORE_DEBUG) message was logged for each
packageName in each journal file.
After this CL:
- packageNames are de-duplicated before scheduling backups or logging
(it's not clear to me whether duplicate packageNames previously
occurred, in practice).
- one message is logged for the number (if > 0) of stale journals.
- one message is logged for the number (including their names, if
MORE_DEBUG) of packages.
Bug: 161940947
Test: fewer 'Found state backup journal' messages printed
Merged-In: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
Change-Id: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
With the goal of reducing log spam, print only a summary 'Found stale
backup journal' messages instead of logging within the inner loop.
Previously, over 12k messages could be printed at a time from this
function.
Before this CL:
- a backup was scheduled for each packageName from each stale
journal
- one (or two, if MORE_DEBUG) message was logged for each
packageName in each journal file.
After this CL:
- packageNames are de-duplicated before scheduling backups or logging
(it's not clear to me whether duplicate packageNames previously
occurred, in practice).
- one message is logged for the number (if > 0) of stale journals.
- one message is logged for the number (including their names, if
MORE_DEBUG) of packages.
Bug: 161940947
Test: fewer 'Found state backup journal' messages printed
Merged-In: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
Change-Id: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
(cherry picked from commit 112e3c2d049d304b0aa57750fcaa6415ea2b6fef)
Since commit c31a839fd3ecc91807d735884d09fcbaf62e9244, this
logic looped "while (dataInputStream.available() > 0)", which
indicates whether more bytes can be read _without blocking_.
It's possible for this to be false even when there are more
bytes available in the file, which means that this loop was
prone to premature termination; somewhat surprisingly to me, a
DataInputStream(BufferedInputStream(FileInputStream))) wrapper
does return available() > 0 initially (empirically tested on
the latest development version), but it's not clear whether
this will reliably remain the case later on in the file and
how often in practice this failed. Either way, this code
was fragile and subject to potential silent breakage in future
where some packageNames would not have been reliably read.
This CL changes the logic back to reading unconditionally but
catching EOFException, like the corresponding logic did prior
to commit c31a839fd3ecc91807d735884d09fcbaf62e9244 (July 2017).
To demonstrate the original bug through a regression test, I
would have needed to extract a static helper method
@VisibleForTesting forEach(Consumer, InputStream) to demonstrate
the behavior with an InputStream whose available() returns 0.
This didn't seem worth it, so I've only added a comment explaining
the logic to minimize the chance of future regression.
This CL also fixes incorrect use of a try-with-resources statement
that would have not closed the FileInputStream if the
BufferedInputStream ctor had thrown (would not have occurred in
practice).
Bug: 162160897
Test: atest \
FrameworksServicesTests:com.android.server.backup.DataChangedJournalTest
Change-Id: I7641cf9ea269ef050ceb716c4e7fe34166bc8295
Passing a null File into DataChangedJournal would always have
thrown NPE if various methods (toString(), equals(), hashCode(),
addPackage()) were called later, but by that time the origin
(stacktrace) of the offending null value would have been lost.
This CL changes the class to detect this error earlier, namely
in the ctor.
In addition, this CL changes newJournal(File journalDirectory)
to not accept null; previously, if null was passed, then the
journal would have been created in the wrong directory, which
would then have led to incorrect behavior of listJournals:
- listJournals(null) would have thrown NPE
- listJournals(nonNullJournalDirectory) would not have found
the journal.
I convinced myself through code inspection that this error case
is not currently possible, but this was not straightforward:
UserBackupManagerService.mJournalDir is passed to DCJ.newJournal()
from writeToJournalLocked() which is called from dataChangedImpl()
which is called from mPackageTrackingReceiver, which is luckily
registered only after mJournalDir was initialized fairly late in the
non-test ctor); should this become buggy in future, then after this
CL it should be more likely to be discovered.
Bug: 162022005
Test: Manually checked no non-test callers pass in non-null.
Test: atest FrameworksServicesTests:com.android.server.backup.DataChangedJournalTest
Change-Id: I1756add05f3d65e455b2e61448f98ceceb5b39f1
Problems:
1. The class implemented equals() but not hashCode()
2. equals() attempted to canonicalize the underlying path and silently
returned false if the I/O operation failed.
Fixes:
1. Implemented hashCode().
2. Stopped equals() attempting canonicalization and built in directly
on top of File.equals() instead. I also considered moving the
canonicalization to the DataChangedJournal ctor, but that
- had the undesirable side effect of stopping interaction with the
journal for reasons other than the file itself being corrupt, and
- posed the problem of what to do in case of an IOException during
construction.
The downside of course is that caller which (inappropriately) relied
on equals() doing canonicalization can no longer rely on that. I
didn't find any such callers.
Note:
When DateChangedJournal.listJournals() fails to perform a directory
listing (e.g. if the journal directory doesn't exist), it currently
still returns an (empty) ArrayList<DateChangedJournal>. This CL does
not address this, but in a follow-up CL I might change this to return
an unmodifiable List or Set<DateChangedJournal>, and throw an
IOException if the directory contents can't be listed.
Bug: 162022005
Test: atest FrameworksServicesTests:com.android.server.backup.DataChangedJournalTest
Change-Id: I94f35f9a3082f398758fe5b03fb8d260e4fb3e1b
With the goal of reducing log spam, print only a summary 'Found stale
backup journal' messages instead of logging within the inner loop.
Previously, over 12k messages could be printed at a time from this
function.
Before this CL:
- a backup was scheduled for each packageName from each stale
journal
- one (or two, if MORE_DEBUG) message was logged for each
packageName in each journal file.
After this CL:
- packageNames are de-duplicated before scheduling backups or logging
(it's not clear to me whether duplicate packageNames previously
occurred, in practice).
- one message is logged for the number (if > 0) of stale journals.
- one message is logged for the number (including their names, if
MORE_DEBUG) of packages.
Bug: 161940947
Test: fewer 'Found state backup journal' messages printed
Change-Id: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
After refactoring AppBackupUtils into BackupEligibilityRules (see the
other linked CL), update the usages throughout the code.
Bug: 161241479
Test: atest UserBackupManagerServiceTest
atest TarBackupReaderTest
atest BackupHandlerTest
atest PerformUnifiedRestoreTaskTest
Change-Id: I2a90c4f5b951fa3e3c564a1065ad10a88cc16273
AppBackupUtils contains static utility methods for checking backup
eligibility of packages. Refactor it into an instance class called
BackupEligibilityRules to better reflect the responsibilities of the
class, encapsulate eligibility-related state as well as logic and
simplify testing. See the bug description for full rationale.
Bug: 161241479
Test: atest AppBackupUtilsTest
Change-Id: I8ae47ac2a15e3c70da8feab752dd8354951224ea
Add an override of BackupManager#requestBackup where type of the
operation (a regular backup or a migration) can be specified.
Bug: 160407842
Test: atest UserBackupManagerServiceTest
Change-Id: Ia54fa26b040c3ec3612672585561794ff831afef
This CL will be part of a series to implement backup / restore of all
app data during a device-to-device migration. Detailed overview of the changes at go/br-fsd2d-design-doc-app-data.
Bug: 160407842
Test: atest AppBackupUtilsTest
Change-Id: I35ce627f1d372437d8ba8495029c6df15db31552
Since Android 10, backupPm() includes sendDataToTransport(), which was
not previously the case. This means that error handling logic that
deletes the backup state file (causing initialize_device() on the next
attempt, which deletes any existing backup) will now also be triggered
upon errors during sendDataToTransport(), which wasn't previously
(Android <= 9) the case.
This has the potential of making an existing temporary outage much
worse:
1. A few devices might run into temporary issues, e.g. a B&R server
returning HTTP 503 Service Unavailable (treated as a
TransientHttpStatusException instanceof NetworkException, which
is mapped to TRANSPORT_ERROR during handleTransportStatus(),
which results in a TaskException with stateCompromised==false
but which backupPm() wraps in another TaskException that forces
stateCompromised=true).
2. On their next backup attempt, those devices throw away any
existing backup and start from scratch (initialize_device()),
increasing the load on the server.
3. This leads to a positive-feedback loop where more devices than
before run into HTTP 503 Service Unavailable.
4. As a result, masses of devices delete their backups and then
hammer the B&R server with attempts to upload new backups.
5. Backups are unavailable to any users who would otherwise rely
on them during this outage.
To improve on this dangerous situation, this CL changes the code to
force stateCompromised=true only for TaskExceptions thrown
specifically during extractPmAgentData(), and (as before) for all
AgentExceptions.
Note that the code is still quite brittle. It still seems like we
are probably forcing stateCompromised=true in too many situations,
but it's hard to say so this CL is being conservative about the
changes. Changing back to the old behavior could be done through
a local change around KeyValueBackupTask.java:676; a future CL may
do this to have a safety hatch in case we want to cherry-pick this
CL into an upcoming Android release late in the release cycle.
[1] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/pie-dev/services/backup/java/com/android/server/backup/internal/PerformBackupTask.java#1035
[2] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java#1040
[3] https://source.corp.google.com/piper///depot/google3/java/com/google/android/gmscore/integ/modules/backup/transport/src/com/google/android/gms/backup/transport/GmsBackupTransport.java;l=770;rcl=281845876
Bug: 144030477
Test: Checked that the following passes after this CL, but
testRunTask_whenTransportReturnsErrorForPm_updatesFilesAndCleansUp()
fails if I revert the state of KeyValueBackupTask.java to before this CL:
ROBOTEST_FILTER=KeyValueBackupTaskTest make \
RunBackupFrameworksServicesRoboTests
Change-Id: I6c622c55fbd804ec0a12e0bea7ade1308f7a3877
(cherry picked from commit 819ed81faaa295d9e1096f13f599cb43d48cda88)
Bug: 159648065
Test: N/A
Change the setting key to be 'backup_skip_user_facing_packages'. See the
bug for context.
Change-Id: I315141a509976821c7db311544a5c0f4e6fd1917
