Ensure streams get closed eventually

This commit is contained in:
Torsten Grote 2020-08-13 11:00:51 -03:00 committed by Chirayu Desai
parent 5515e5c88f
commit a63a893a61
6 changed files with 79 additions and 32 deletions

View file

@ -77,7 +77,9 @@ internal class BackupCoordinator(
val token = clock.time() val token = clock.time()
if (plugin.initializeDevice(token)) { if (plugin.initializeDevice(token)) {
Log.d(TAG, "Resetting backup metadata...") Log.d(TAG, "Resetting backup metadata...")
metadataManager.onDeviceInitialization(token, plugin.getMetadataOutputStream()) plugin.getMetadataOutputStream().use {
metadataManager.onDeviceInitialization(token, it)
}
} else { } else {
Log.d(TAG, "Storage was already initialized, doing no-op") Log.d(TAG, "Storage was already initialized, doing no-op")
} }
@ -302,8 +304,9 @@ internal class BackupCoordinator(
apkBackup.backupApkIfNecessary(packageInfo, packageState) { apkBackup.backupApkIfNecessary(packageInfo, packageState) {
plugin.getApkOutputStream(packageInfo) plugin.getApkOutputStream(packageInfo)
}?.let { packageMetadata -> }?.let { packageMetadata ->
val outputStream = plugin.getMetadataOutputStream() plugin.getMetadataOutputStream().use {
metadataManager.onApkBackedUp(packageInfo, packageMetadata, outputStream) metadataManager.onApkBackedUp(packageInfo, packageMetadata, it)
}
} }
} catch (e: IOException) { } catch (e: IOException) {
Log.e(TAG, "Error while writing APK or metadata for $packageName", e) Log.e(TAG, "Error while writing APK or metadata for $packageName", e)
@ -313,8 +316,9 @@ internal class BackupCoordinator(
private suspend fun onPackageBackedUp(packageInfo: PackageInfo) { private suspend fun onPackageBackedUp(packageInfo: PackageInfo) {
val packageName = packageInfo.packageName val packageName = packageInfo.packageName
try { try {
val outputStream = plugin.getMetadataOutputStream() plugin.getMetadataOutputStream().use {
metadataManager.onPackageBackedUp(packageInfo, outputStream) metadataManager.onPackageBackedUp(packageInfo, it)
}
} catch (e: IOException) { } catch (e: IOException) {
Log.e(TAG, "Error while writing metadata for $packageName", e) Log.e(TAG, "Error while writing metadata for $packageName", e)
} }
@ -325,8 +329,9 @@ internal class BackupCoordinator(
if (cancelReason == NO_DATA && packageInfo.isSystemApp()) return if (cancelReason == NO_DATA && packageInfo.isSystemApp()) return
val packageName = packageInfo.packageName val packageName = packageInfo.packageName
try { try {
val outputStream = plugin.getMetadataOutputStream() plugin.getMetadataOutputStream().use {
metadataManager.onPackageBackupError(packageInfo, cancelReason, outputStream) metadataManager.onPackageBackupError(packageInfo, cancelReason, it)
}
} catch (e: IOException) { } catch (e: IOException) {
Log.e(TAG, "Error while writing metadata for $packageName", e) Log.e(TAG, "Error while writing metadata for $packageName", e)
} }

View file

@ -149,7 +149,7 @@ internal class FullBackup(
"No OutputStream xor no StreamGetter" "No OutputStream xor no StreamGetter"
} }
val outputStream = state.outputStream ?: suspend { val outputStream = state.outputStream ?: suspend {
val stream = state.outputStreamInit!!() // not-null due to check above val stream = state.outputStreamInit!!() // not-null due to check above
state.outputStream = stream state.outputStream = stream
stream stream
}() }()

View file

@ -114,15 +114,26 @@ internal class KVBackup(
plugin.deleteRecord(packageInfo, op.base64Key) plugin.deleteRecord(packageInfo, op.base64Key)
} else { } else {
val outputStream = plugin.getOutputStreamForRecord(packageInfo, op.base64Key) val outputStream = plugin.getOutputStreamForRecord(packageInfo, op.base64Key)
val header = VersionHeader(packageName = packageInfo.packageName, key = op.key) try {
headerWriter.writeVersion(outputStream, header) val header = VersionHeader(
crypto.encryptHeader(outputStream, header) packageName = packageInfo.packageName,
crypto.encryptMultipleSegments(outputStream, op.value) key = op.key
outputStream.flush() )
closeQuietly(outputStream) headerWriter.writeVersion(outputStream, header)
crypto.encryptHeader(outputStream, header)
crypto.encryptMultipleSegments(outputStream, op.value)
outputStream.flush()
} finally {
closeQuietly(outputStream)
}
} }
} catch (e: IOException) { } catch (e: IOException) {
Log.e(TAG, "Unable to update base64Key file for base64Key ${op.base64Key}", e) Log.e(TAG, "Unable to update base64Key file for base64Key ${op.base64Key}", e)
// Returning something more forgiving such as TRANSPORT_PACKAGE_REJECTED
// will still make the entire backup fail.
// TODO However, TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED might buy us a retry,
// we would just need to be careful not to create an infinite loop
// for permanent errors.
return backupError(TRANSPORT_ERROR) return backupError(TRANSPORT_ERROR)
} }
} }
@ -141,7 +152,7 @@ internal class KVBackup(
return generateSequence { return generateSequence {
// read the next header or end the sequence in case of error or no more headers // read the next header or end the sequence in case of error or no more headers
try { try {
if (!changeSet.readNextHeader()) return@generateSequence null // end the sequence if (!changeSet.readNextHeader()) return@generateSequence null // end the sequence
} catch (e: IOException) { } catch (e: IOException) {
Log.e(TAG, "Error reading next header", e) Log.e(TAG, "Error reading next header", e)
return@generateSequence Result.Error(e) return@generateSequence Result.Error(e)

View file

@ -143,10 +143,8 @@ internal class KVRestore(
state: KVRestoreState, state: KVRestoreState,
dKey: DecodedKey, dKey: DecodedKey,
out: BackupDataOutput out: BackupDataOutput
) { ) = plugin.getInputStreamForRecord(state.token, state.packageInfo, dKey.base64Key)
val inputStream = .use { inputStream ->
plugin.getInputStreamForRecord(state.token, state.packageInfo, dKey.base64Key)
try {
val version = headerReader.readVersion(inputStream) val version = headerReader.readVersion(inputStream)
crypto.decryptHeader(inputStream, version, state.packageInfo.packageName, dKey.key) crypto.decryptHeader(inputStream, version, state.packageInfo.packageName, dKey.key)
val value = crypto.decryptMultipleSegments(inputStream) val value = crypto.decryptMultipleSegments(inputStream)
@ -155,10 +153,8 @@ internal class KVRestore(
out.writeEntityHeader(dKey.key, size) out.writeEntityHeader(dKey.key, size)
out.writeEntityData(value, size) out.writeEntityData(value, size)
} finally { Unit
closeQuietly(inputStream)
} }
}
private class DecodedKey(internal val base64Key: String) : Comparable<DecodedKey> { private class DecodedKey(internal val base64Key: String) : Comparable<DecodedKey> {
internal val key = base64Key.decodeBase64() internal val key = base64Key.decodeBase64()

View file

@ -70,9 +70,12 @@ internal class BackupCoordinatorTest : BackupTest() {
every { metadataManager.onDeviceInitialization(token, metadataOutputStream) } just Runs every { metadataManager.onDeviceInitialization(token, metadataOutputStream) } just Runs
every { kv.hasState() } returns false every { kv.hasState() } returns false
every { full.hasState() } returns false every { full.hasState() } returns false
every { metadataOutputStream.close() } just Runs
assertEquals(TRANSPORT_OK, backup.initializeDevice()) assertEquals(TRANSPORT_OK, backup.initializeDevice())
assertEquals(TRANSPORT_OK, backup.finishBackup()) assertEquals(TRANSPORT_OK, backup.finishBackup())
verify { metadataOutputStream.close() }
} }
@Test @Test
@ -137,7 +140,10 @@ internal class BackupCoordinatorTest : BackupTest() {
} else { } else {
every { kv.getQuota() } returns quota every { kv.getQuota() } returns quota
} }
every { metadataOutputStream.close() } just Runs
assertEquals(quota, backup.getBackupQuota(packageInfo.packageName, isFullBackup)) assertEquals(quota, backup.getBackupQuota(packageInfo.packageName, isFullBackup))
verify { metadataOutputStream.close() }
} }
@Test @Test
@ -192,8 +198,11 @@ internal class BackupCoordinatorTest : BackupTest() {
coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream
every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs
every { kv.finishBackup() } returns result every { kv.finishBackup() } returns result
every { metadataOutputStream.close() } just Runs
assertEquals(result, backup.finishBackup()) assertEquals(result, backup.finishBackup())
verify { metadataOutputStream.close() }
} }
@Test @Test
@ -206,8 +215,11 @@ internal class BackupCoordinatorTest : BackupTest() {
coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream
every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs
every { full.finishBackup() } returns result every { full.finishBackup() } returns result
every { metadataOutputStream.close() } just Runs
assertEquals(result, backup.finishBackup()) assertEquals(result, backup.finishBackup())
verify { metadataOutputStream.close() }
} }
@Test @Test
@ -234,6 +246,7 @@ internal class BackupCoordinatorTest : BackupTest() {
} just Runs } just Runs
coEvery { full.cancelFullBackup() } just Runs coEvery { full.cancelFullBackup() } just Runs
every { settingsManager.getStorage() } returns storage every { settingsManager.getStorage() } returns storage
every { metadataOutputStream.close() } just Runs
assertEquals( assertEquals(
TRANSPORT_OK, TRANSPORT_OK,
@ -253,6 +266,7 @@ internal class BackupCoordinatorTest : BackupTest() {
verify(exactly = 1) { verify(exactly = 1) {
metadataManager.onPackageBackupError(packageInfo, QUOTA_EXCEEDED, metadataOutputStream) metadataManager.onPackageBackupError(packageInfo, QUOTA_EXCEEDED, metadataOutputStream)
} }
verify { metadataOutputStream.close() }
} }
@Test @Test
@ -271,6 +285,7 @@ internal class BackupCoordinatorTest : BackupTest() {
} just Runs } just Runs
coEvery { full.cancelFullBackup() } just Runs coEvery { full.cancelFullBackup() } just Runs
every { settingsManager.getStorage() } returns storage every { settingsManager.getStorage() } returns storage
every { metadataOutputStream.close() } just Runs
assertEquals( assertEquals(
TRANSPORT_OK, TRANSPORT_OK,
@ -287,6 +302,7 @@ internal class BackupCoordinatorTest : BackupTest() {
verify(exactly = 1) { verify(exactly = 1) {
metadataManager.onPackageBackupError(packageInfo, NO_DATA, metadataOutputStream) metadataManager.onPackageBackupError(packageInfo, NO_DATA, metadataOutputStream)
} }
verify { metadataOutputStream.close() }
} }
@Test @Test
@ -326,6 +342,7 @@ internal class BackupCoordinatorTest : BackupTest() {
} just Runs } just Runs
// do actual @pm@ backup // do actual @pm@ backup
coEvery { kv.performBackup(packageInfo, fileDescriptor, 0) } returns TRANSPORT_OK coEvery { kv.performBackup(packageInfo, fileDescriptor, 0) } returns TRANSPORT_OK
every { metadataOutputStream.close() } just Runs
assertEquals( assertEquals(
TRANSPORT_OK, TRANSPORT_OK,
@ -335,6 +352,7 @@ internal class BackupCoordinatorTest : BackupTest() {
coVerify { coVerify {
apkBackup.backupApkIfNecessary(notAllowedPackages[0], NOT_ALLOWED, any()) apkBackup.backupApkIfNecessary(notAllowedPackages[0], NOT_ALLOWED, any())
apkBackup.backupApkIfNecessary(notAllowedPackages[1], NOT_ALLOWED, any()) apkBackup.backupApkIfNecessary(notAllowedPackages[1], NOT_ALLOWED, any())
metadataOutputStream.close()
} }
} }

View file

@ -15,13 +15,14 @@ import io.mockk.coEvery
import io.mockk.every import io.mockk.every
import io.mockk.just import io.mockk.just
import io.mockk.mockk import io.mockk.mockk
import io.mockk.verify
import kotlinx.coroutines.runBlocking import kotlinx.coroutines.runBlocking
import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertFalse
import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Assertions.assertTrue
import org.junit.jupiter.api.Test import org.junit.jupiter.api.Test
import java.io.IOException import java.io.IOException
import java.util.* import java.util.Base64
import kotlin.random.Random import kotlin.random.Random
@Suppress("BlockingMethodInNonBlockingContext") @Suppress("BlockingMethodInNonBlockingContext")
@ -56,7 +57,10 @@ internal class KVBackupTest : BackupTest() {
fun `incremental backup with no data gets rejected`() = runBlocking { fun `incremental backup with no data gets rejected`() = runBlocking {
coEvery { plugin.hasDataForPackage(packageInfo) } returns false coEvery { plugin.hasDataForPackage(packageInfo) } returns false
assertEquals(TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED, backup.performBackup(packageInfo, data, FLAG_INCREMENTAL)) assertEquals(
TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED,
backup.performBackup(packageInfo, data, FLAG_INCREMENTAL)
)
assertFalse(backup.hasState()) assertFalse(backup.hasState())
} }
@ -80,15 +84,19 @@ internal class KVBackupTest : BackupTest() {
} }
@Test @Test
fun `ignoring exception when clearing data when non-incremental backup has data`() = runBlocking { fun `ignoring exception when clearing data when non-incremental backup has data`() =
singleRecordBackup(true) runBlocking {
coEvery { plugin.removeDataOfPackage(packageInfo) } throws IOException() singleRecordBackup(true)
coEvery { plugin.removeDataOfPackage(packageInfo) } throws IOException()
assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, FLAG_NON_INCREMENTAL)) assertEquals(
assertTrue(backup.hasState()) TRANSPORT_OK,
assertEquals(TRANSPORT_OK, backup.finishBackup()) backup.performBackup(packageInfo, data, FLAG_NON_INCREMENTAL)
assertFalse(backup.hasState()) )
} assertTrue(backup.hasState())
assertEquals(TRANSPORT_OK, backup.finishBackup())
assertFalse(backup.hasState())
}
@Test @Test
fun `ensuring storage throws exception`() = runBlocking { fun `ensuring storage throws exception`() = runBlocking {
@ -139,9 +147,12 @@ internal class KVBackupTest : BackupTest() {
getDataInput(listOf(true)) getDataInput(listOf(true))
coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream
every { headerWriter.writeVersion(outputStream, versionHeader) } throws IOException() every { headerWriter.writeVersion(outputStream, versionHeader) } throws IOException()
every { outputStream.close() } just Runs
assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
assertFalse(backup.hasState()) assertFalse(backup.hasState())
verify { outputStream.close() }
} }
@Test @Test
@ -152,9 +163,12 @@ internal class KVBackupTest : BackupTest() {
coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream
every { headerWriter.writeVersion(outputStream, versionHeader) } just Runs every { headerWriter.writeVersion(outputStream, versionHeader) } just Runs
every { crypto.encryptMultipleSegments(outputStream, any()) } throws IOException() every { crypto.encryptMultipleSegments(outputStream, any()) } throws IOException()
every { outputStream.close() } just Runs
assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
assertFalse(backup.hasState()) assertFalse(backup.hasState())
verify { outputStream.close() }
} }
@Test @Test
@ -164,9 +178,12 @@ internal class KVBackupTest : BackupTest() {
writeHeaderAndEncrypt() writeHeaderAndEncrypt()
every { outputStream.write(value) } just Runs every { outputStream.write(value) } just Runs
every { outputStream.flush() } throws IOException() every { outputStream.flush() } throws IOException()
every { outputStream.close() } just Runs
assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
assertFalse(backup.hasState()) assertFalse(backup.hasState())
verify { outputStream.close() }
} }
@Test @Test