From a63a893a61d082e9f819e9f17f0981f1cad886ff Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 13 Aug 2020 11:00:51 -0300 Subject: [PATCH] Ensure streams get closed eventually --- .../transport/backup/BackupCoordinator.kt | 19 ++++++---- .../seedvault/transport/backup/FullBackup.kt | 2 +- .../seedvault/transport/backup/KVBackup.kt | 25 +++++++++---- .../seedvault/transport/restore/KVRestore.kt | 10 ++--- .../transport/backup/BackupCoordinatorTest.kt | 18 +++++++++ .../transport/backup/KVBackupTest.kt | 37 ++++++++++++++----- 6 files changed, 79 insertions(+), 32 deletions(-) diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt index 79850a72..877db369 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt @@ -77,7 +77,9 @@ internal class BackupCoordinator( val token = clock.time() if (plugin.initializeDevice(token)) { Log.d(TAG, "Resetting backup metadata...") - metadataManager.onDeviceInitialization(token, plugin.getMetadataOutputStream()) + plugin.getMetadataOutputStream().use { + metadataManager.onDeviceInitialization(token, it) + } } else { Log.d(TAG, "Storage was already initialized, doing no-op") } @@ -302,8 +304,9 @@ internal class BackupCoordinator( apkBackup.backupApkIfNecessary(packageInfo, packageState) { plugin.getApkOutputStream(packageInfo) }?.let { packageMetadata -> - val outputStream = plugin.getMetadataOutputStream() - metadataManager.onApkBackedUp(packageInfo, packageMetadata, outputStream) + plugin.getMetadataOutputStream().use { + metadataManager.onApkBackedUp(packageInfo, packageMetadata, it) + } } } catch (e: IOException) { 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) { val packageName = packageInfo.packageName try { - val outputStream = plugin.getMetadataOutputStream() - metadataManager.onPackageBackedUp(packageInfo, outputStream) + plugin.getMetadataOutputStream().use { + metadataManager.onPackageBackedUp(packageInfo, it) + } } catch (e: IOException) { Log.e(TAG, "Error while writing metadata for $packageName", e) } @@ -325,8 +329,9 @@ internal class BackupCoordinator( if (cancelReason == NO_DATA && packageInfo.isSystemApp()) return val packageName = packageInfo.packageName try { - val outputStream = plugin.getMetadataOutputStream() - metadataManager.onPackageBackupError(packageInfo, cancelReason, outputStream) + plugin.getMetadataOutputStream().use { + metadataManager.onPackageBackupError(packageInfo, cancelReason, it) + } } catch (e: IOException) { Log.e(TAG, "Error while writing metadata for $packageName", e) } diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/FullBackup.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/FullBackup.kt index 9dc8cf94..0053a3b9 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/FullBackup.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/FullBackup.kt @@ -149,7 +149,7 @@ internal class FullBackup( "No OutputStream xor no StreamGetter" } 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 stream }() diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt index c674471d..56abd080 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt @@ -114,15 +114,26 @@ internal class KVBackup( plugin.deleteRecord(packageInfo, op.base64Key) } else { val outputStream = plugin.getOutputStreamForRecord(packageInfo, op.base64Key) - val header = VersionHeader(packageName = packageInfo.packageName, key = op.key) - headerWriter.writeVersion(outputStream, header) - crypto.encryptHeader(outputStream, header) - crypto.encryptMultipleSegments(outputStream, op.value) - outputStream.flush() - closeQuietly(outputStream) + try { + val header = VersionHeader( + packageName = packageInfo.packageName, + key = op.key + ) + headerWriter.writeVersion(outputStream, header) + crypto.encryptHeader(outputStream, header) + crypto.encryptMultipleSegments(outputStream, op.value) + outputStream.flush() + } finally { + closeQuietly(outputStream) + } } } catch (e: IOException) { 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) } } @@ -141,7 +152,7 @@ internal class KVBackup( return generateSequence { // read the next header or end the sequence in case of error or no more headers try { - if (!changeSet.readNextHeader()) return@generateSequence null // end the sequence + if (!changeSet.readNextHeader()) return@generateSequence null // end the sequence } catch (e: IOException) { Log.e(TAG, "Error reading next header", e) return@generateSequence Result.Error(e) diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestore.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestore.kt index aee90c46..ef86a4a8 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestore.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestore.kt @@ -143,10 +143,8 @@ internal class KVRestore( state: KVRestoreState, dKey: DecodedKey, out: BackupDataOutput - ) { - val inputStream = - plugin.getInputStreamForRecord(state.token, state.packageInfo, dKey.base64Key) - try { + ) = plugin.getInputStreamForRecord(state.token, state.packageInfo, dKey.base64Key) + .use { inputStream -> val version = headerReader.readVersion(inputStream) crypto.decryptHeader(inputStream, version, state.packageInfo.packageName, dKey.key) val value = crypto.decryptMultipleSegments(inputStream) @@ -155,10 +153,8 @@ internal class KVRestore( out.writeEntityHeader(dKey.key, size) out.writeEntityData(value, size) - } finally { - closeQuietly(inputStream) + Unit } - } private class DecodedKey(internal val base64Key: String) : Comparable { internal val key = base64Key.decodeBase64() diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt index 97c484ca..c435a0a3 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt @@ -70,9 +70,12 @@ internal class BackupCoordinatorTest : BackupTest() { every { metadataManager.onDeviceInitialization(token, metadataOutputStream) } just Runs every { kv.hasState() } returns false every { full.hasState() } returns false + every { metadataOutputStream.close() } just Runs assertEquals(TRANSPORT_OK, backup.initializeDevice()) assertEquals(TRANSPORT_OK, backup.finishBackup()) + + verify { metadataOutputStream.close() } } @Test @@ -137,7 +140,10 @@ internal class BackupCoordinatorTest : BackupTest() { } else { every { kv.getQuota() } returns quota } + every { metadataOutputStream.close() } just Runs assertEquals(quota, backup.getBackupQuota(packageInfo.packageName, isFullBackup)) + + verify { metadataOutputStream.close() } } @Test @@ -192,8 +198,11 @@ internal class BackupCoordinatorTest : BackupTest() { coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs every { kv.finishBackup() } returns result + every { metadataOutputStream.close() } just Runs assertEquals(result, backup.finishBackup()) + + verify { metadataOutputStream.close() } } @Test @@ -206,8 +215,11 @@ internal class BackupCoordinatorTest : BackupTest() { coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs every { full.finishBackup() } returns result + every { metadataOutputStream.close() } just Runs assertEquals(result, backup.finishBackup()) + + verify { metadataOutputStream.close() } } @Test @@ -234,6 +246,7 @@ internal class BackupCoordinatorTest : BackupTest() { } just Runs coEvery { full.cancelFullBackup() } just Runs every { settingsManager.getStorage() } returns storage + every { metadataOutputStream.close() } just Runs assertEquals( TRANSPORT_OK, @@ -253,6 +266,7 @@ internal class BackupCoordinatorTest : BackupTest() { verify(exactly = 1) { metadataManager.onPackageBackupError(packageInfo, QUOTA_EXCEEDED, metadataOutputStream) } + verify { metadataOutputStream.close() } } @Test @@ -271,6 +285,7 @@ internal class BackupCoordinatorTest : BackupTest() { } just Runs coEvery { full.cancelFullBackup() } just Runs every { settingsManager.getStorage() } returns storage + every { metadataOutputStream.close() } just Runs assertEquals( TRANSPORT_OK, @@ -287,6 +302,7 @@ internal class BackupCoordinatorTest : BackupTest() { verify(exactly = 1) { metadataManager.onPackageBackupError(packageInfo, NO_DATA, metadataOutputStream) } + verify { metadataOutputStream.close() } } @Test @@ -326,6 +342,7 @@ internal class BackupCoordinatorTest : BackupTest() { } just Runs // do actual @pm@ backup coEvery { kv.performBackup(packageInfo, fileDescriptor, 0) } returns TRANSPORT_OK + every { metadataOutputStream.close() } just Runs assertEquals( TRANSPORT_OK, @@ -335,6 +352,7 @@ internal class BackupCoordinatorTest : BackupTest() { coVerify { apkBackup.backupApkIfNecessary(notAllowedPackages[0], NOT_ALLOWED, any()) apkBackup.backupApkIfNecessary(notAllowedPackages[1], NOT_ALLOWED, any()) + metadataOutputStream.close() } } diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt index 2647e51e..1de78def 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt @@ -15,13 +15,14 @@ import io.mockk.coEvery import io.mockk.every import io.mockk.just import io.mockk.mockk +import io.mockk.verify import kotlinx.coroutines.runBlocking import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test import java.io.IOException -import java.util.* +import java.util.Base64 import kotlin.random.Random @Suppress("BlockingMethodInNonBlockingContext") @@ -56,7 +57,10 @@ internal class KVBackupTest : BackupTest() { fun `incremental backup with no data gets rejected`() = runBlocking { 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()) } @@ -80,15 +84,19 @@ internal class KVBackupTest : BackupTest() { } @Test - fun `ignoring exception when clearing data when non-incremental backup has data`() = runBlocking { - singleRecordBackup(true) - coEvery { plugin.removeDataOfPackage(packageInfo) } throws IOException() + fun `ignoring exception when clearing data when non-incremental backup has data`() = + runBlocking { + singleRecordBackup(true) + coEvery { plugin.removeDataOfPackage(packageInfo) } throws IOException() - assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, FLAG_NON_INCREMENTAL)) - assertTrue(backup.hasState()) - assertEquals(TRANSPORT_OK, backup.finishBackup()) - assertFalse(backup.hasState()) - } + assertEquals( + TRANSPORT_OK, + backup.performBackup(packageInfo, data, FLAG_NON_INCREMENTAL) + ) + assertTrue(backup.hasState()) + assertEquals(TRANSPORT_OK, backup.finishBackup()) + assertFalse(backup.hasState()) + } @Test fun `ensuring storage throws exception`() = runBlocking { @@ -139,9 +147,12 @@ internal class KVBackupTest : BackupTest() { getDataInput(listOf(true)) coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream every { headerWriter.writeVersion(outputStream, versionHeader) } throws IOException() + every { outputStream.close() } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) + + verify { outputStream.close() } } @Test @@ -152,9 +163,12 @@ internal class KVBackupTest : BackupTest() { coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream every { headerWriter.writeVersion(outputStream, versionHeader) } just Runs every { crypto.encryptMultipleSegments(outputStream, any()) } throws IOException() + every { outputStream.close() } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) + + verify { outputStream.close() } } @Test @@ -164,9 +178,12 @@ internal class KVBackupTest : BackupTest() { writeHeaderAndEncrypt() every { outputStream.write(value) } just Runs every { outputStream.flush() } throws IOException() + every { outputStream.close() } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) + + verify { outputStream.close() } } @Test