diff --git a/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt index 5cb74ee239..c079c68a6e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt @@ -47,17 +47,17 @@ class LoginStateRepository @Inject constructor( private val mutableLoggedInState: MutableStateFlow - init { - var initialState = prefs[keyState]?.let { serializedState -> - runCatching { - json.decodeFromString( - KeyStoreHelper.unseal(serializedState).toString(Charsets.UTF_8) - ) + /** + * Set when the stored login state existed but could not be unsealed, which is not the same thing + * as having no account. Callers cannot tell the two apart from a null state, and treating an + * unreadable account as an absent one sends the user to onboarding, where creating an account + * overwrites the seed that was only unreadable. + */ + @Volatile + private var sealedStateUnreadable = false - }.onFailure { - Log.e(TAG, "Unable to unseal login state", it) - }.getOrNull() - } + init { + var initialState = readSealedState() if (initialState == null) { initialState = runCatching { @@ -179,6 +179,37 @@ class LoginStateRepository @Inject constructor( mutableLoggedInState.value = null } + /** + * Re-attempts a sealed state read that previously failed, and reports whether an account was + * recovered. + * + * Keystore faults are frequently transient, but the read is only performed once when this + * singleton is built, so a fault at startup leaves the account unreadable for the rest of the + * process even after the keystore recovers. Call this wherever there is fresh reason to believe + * it has. + */ + fun reloadUnreadableState(): Boolean { + if (!sealedStateUnreadable || mutableLoggedInState.value != null) return false + + val recovered = readSealedState() ?: return false + + Log.i(TAG, "Recovered a login state that was previously unreadable") + sealedStateUnreadable = false + mutableLoggedInState.value = recovered + return true + } + + private fun readSealedState(): LoggedInState? = prefs[keyState]?.let { serializedState -> + runCatching { + json.decodeFromString( + KeyStoreHelper.unseal(serializedState).toString(Charsets.UTF_8) + ) + }.onFailure { + sealedStateUnreadable = true + Log.e(TAG, "Unable to unseal login state", it) + }.getOrNull() + } + /** * Updates the current [LoggedInState] using the provided [updater] function. * The [org.thoughtcrime.securesms.auth.LoginStateRepository] will manage the persistence diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java index ce53ca37ba..91aff24375 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java @@ -45,6 +45,8 @@ import org.thoughtcrime.securesms.pro.db.ProDatabase; import org.thoughtcrime.securesms.util.ConfigurationMessageUtilities; +import java.io.File; + import javax.inject.Provider; import kotlinx.serialization.json.Json; @@ -141,8 +143,29 @@ public void postKey(SQLiteConnection connection) { long currentTime = System.currentTimeMillis(); // 7 days if (currentTime - TextSecurePreferences.getLastVacuumTime(context) > 604_800_000) { - connection.execute("VACUUM;", null, null); + // Recorded before the attempt rather than after it: a VACUUM that throws would + // otherwise leave the timestamp untouched and be retried on every open from then on, + // which is what turns one unlucky moment into a launch that never succeeds again. TextSecurePreferences.setLastVacuumNow(context); + + // VACUUM rebuilds the database into a temporary copy before replacing the original, so + // it needs roughly twice the file's size free. Below that it is going to fail, and + // failing takes as long as the copy it got through first. + File database = context.getDatabasePath(DATABASE_NAME); + long requiredSpace = database.length() * 2; + + if (database.getUsableSpace() > requiredSpace) { + // This is maintenance, and it runs on the database-open path, where an exception + // does not reach the caller as a failed VACUUM — it reaches them as a database that + // would not open. Nothing here is worth not starting the app for. + try { + connection.execute("VACUUM;", null, null); + } catch (Exception e) { + Log.w(TAG, "Vacuum failed, continuing", e); + } + } else { + Log.i(TAG, "Skipping vacuum: needs " + requiredSpace + " bytes free"); + } } } }, diff --git a/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt b/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt index 1f1aa4b7f3..16c0355047 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt @@ -1,5 +1,9 @@ package org.thoughtcrime.securesms.dependencies +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch import org.session.libsession.messaging.notifications.TokenFetcher import org.session.libsession.messaging.sending_receiving.pollers.OpenGroupPollerManager import org.session.libsession.network.SnodeClock @@ -17,8 +21,45 @@ import org.thoughtcrime.securesms.tokenpage.TokenDataManager import org.thoughtcrime.securesms.util.CurrentActivityObserver import org.thoughtcrime.securesms.webrtc.WebRtcCallBridge import javax.inject.Inject +import javax.inject.Provider -class OnAppStartupComponents private constructor( +class OnAppStartupComponents @Inject constructor( + private val databaseMigrationManager: DatabaseMigrationManager, + private val currentActivityObserver: CurrentActivityObserver, + private val appDisguiseManager: AppDisguiseManager, + private val notificationChannelManager: NotificationChannelManager, + private val deferredComponents: Provider, + @param:ManagerScope private val scope: CoroutineScope, +) { + fun onPostAppStarted() { + databaseMigrationManager.onPostAppStarted() + + // These three take no database dependency, and waiting costs something: CurrentActivityObserver + // registers the activity lifecycle callbacks in its own constructor, so deferring it drops the + // events that arrive before the migration finishes. + currentActivityObserver.onPostAppStarted() + appDisguiseManager.onPostAppStarted() + notificationChannelManager.onPostAppStarted() + + // The rest wait because some of them reach the database, several from their own constructors, + // and the open helper has nothing to hand out until the migration has produced a usable secret. + // Whichever touches it first otherwise dies on a failure the user could have retried from, + // taking the process with it and leaving the migration screen no chance to appear (#2213). + // Deferring the whole remainder is the conservative choice: transitive reach through injected + // dependencies is not cheap to establish per component, and starting one too early costs more + // than starting one too late. A retry that succeeds still reaches Completed, so this also + // starts them after a recovery. + scope.launch(Dispatchers.Main) { + databaseMigrationManager.migrationState.first { + it == DatabaseMigrationManager.MigrationState.Completed + } + + deferredComponents.get().onPostAppStarted() + } + } +} + +class DeferredStartupComponents private constructor( private val components: List ) { fun onPostAppStarted() { @@ -29,36 +70,28 @@ class OnAppStartupComponents private constructor( groupPollerManager: GroupPollerManager, expiredGroupManager: ExpiredGroupManager, openGroupPollerManager: OpenGroupPollerManager, - databaseMigrationManager: DatabaseMigrationManager, tokenManager: TokenDataManager, - currentActivityObserver: CurrentActivityObserver, webRtcCallBridge: WebRtcCallBridge, persistentLogger: PersistentLogger, - appDisguiseManager: AppDisguiseManager, tokenFetcher: TokenFetcher, emojiIndexLoader: EmojiIndexLoader, subscriptionCoordinator: SubscriptionCoordinator, authAwareHandler: AuthAwareComponentsHandler, snodeClock: SnodeClock, subscriptionManagers: Set<@JvmSuppressWildcards SubscriptionManager>, - notificationChannelManager: NotificationChannelManager, ): this( components = listOf( groupPollerManager, expiredGroupManager, openGroupPollerManager, - databaseMigrationManager, tokenManager, - currentActivityObserver, webRtcCallBridge, persistentLogger, - appDisguiseManager, tokenFetcher, emojiIndexLoader, subscriptionCoordinator, authAwareHandler, - snodeClock, - notificationChannelManager + snodeClock ) + subscriptionManagers ) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt index 386ae2ba9f..7ada19885a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt @@ -1,7 +1,9 @@ package org.thoughtcrime.securesms.migration import android.app.Application +import android.os.Build import android.os.SystemClock +import androidx.annotation.RequiresApi import androidx.annotation.StringRes import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -17,6 +19,7 @@ import net.zetetic.database.sqlcipher.SQLiteDatabaseHook import network.loki.messenger.R import org.session.libsession.utilities.TextSecurePreferences import org.session.libsignal.utilities.Log +import org.thoughtcrime.securesms.auth.LoginStateRepository import org.thoughtcrime.securesms.crypto.DatabaseSecretProvider import org.thoughtcrime.securesms.database.helpers.SQLCipherOpenHelper import org.thoughtcrime.securesms.dependencies.ManagerScope @@ -30,6 +33,7 @@ class DatabaseMigrationManager @Inject constructor( private val application: Application, private val prefs: TextSecurePreferences, private val databaseSecretProvider: DatabaseSecretProvider, + private val loginStateRepository: Provider, jsonProvider: Provider, @param:ManagerScope private val scope: CoroutineScope, ) : OnAppStartupComponent { @@ -43,10 +47,13 @@ class DatabaseMigrationManager @Inject constructor( // First perform a cheap check to see if the migration is done, if so we can skip the wait. if (mutableMigrationState.value != MigrationState.Completed) { - // Wait until the migration is done. This is a semi-expensive call but it's necessary - // to block the callers from accessing the database until we have sorted out the migration - // process. Note that we don't pass in errors here as the callers of this function - // don't expect the exceptions at all so we have no choice but to block them. + // Callers cannot do anything sensible with a half-migrated database, so they wait + // here rather than be handed one, and a retry that succeeds releases them. + // + // Known hazard: Error is terminal and is not a terminating condition for this wait, so + // a caller arriving after a failed migration blocks for the life of the process and its + // thread is never returned. Deferring database-backed startup work keeps the usual + // arrivals away from it, but callers reached from login-state flows are not covered. runBlocking { migrationState.first { it == MigrationState.Completed } } @@ -90,6 +97,12 @@ class DatabaseMigrationManager @Inject constructor( mutableMigrationState.value = MigrationState.Migrating(steps.toList()) try { + // Resolving the secret here is what puts a failed KeyStoreHelper.unseal into + // MigrationState.Error, where the user is offered retry and log export. Its only other + // dereference is in openHelper, which sits outside every handler on this path, so left + // to happen there the AssertionError kills the process on every launch instead (#2213). + dbSecret + for ((index, desc) in stepDescriptors.withIndex()) { Log.d(TAG, "Starting migration step: ${desc.name}") val stepStartedAt = SystemClock.elapsedRealtime() @@ -106,13 +119,58 @@ class DatabaseMigrationManager @Inject constructor( mutableMigrationState.value = MigrationState.Migrating(steps.toList()) } + // Reaching Completed after a failure means the keystore is working again, and the login + // state is only read once when its repository is built — so an account left unreadable + // by the same fault has to be re-read before anything routes on it, or a recovered user + // is sent to onboarding on top of their own data. + loginStateRepository.get().reloadUnreadableState() + mutableMigrationState.value = MigrationState.Completed } catch (ec: Exception) { - mutableMigrationState.value = MigrationState.Error(ec) + recordMigrationFailure(ec) + return + } catch (ec: AssertionError) { + // AssertionError is caught alongside Exception because KeyStoreHelper throws it on + // purpose — it is how every crypto failure is reported, a deliberate signal rather than + // a symptom of a dying VM. Errors that do mean the process is already lost + // (OutOfMemoryError, StackOverflowError, LinkageError) are deliberately left to kill it. + recordMigrationFailure(ec) return } } + private fun recordMigrationFailure(error: Throwable) { + logKeyStoreFailure(error) + mutableMigrationState.value = MigrationState.Error(error) + } + + /** + * Records the keystore error code behind a crypto failure, if there is one. + * + * The code distinguishes a transient keystore fault, where the data is intact and a retry may + * succeed, from a key that can no longer decrypt what it sealed. Nothing else in the crash + * reaching us carries that distinction. + */ + private fun logKeyStoreFailure(error: Throwable) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + logKeyStoreExceptionDetails(error) + } + } + + @RequiresApi(Build.VERSION_CODES.TIRAMISU) + private fun logKeyStoreExceptionDetails(error: Throwable) { + val keyStoreError = generateSequence(error) { it.cause } + .filterIsInstance() + .firstOrNull() ?: return + + Log.w( + TAG, + "Keystore failure: code=${keyStoreError.numericErrorCode}, " + + "transient=${keyStoreError.isTransientFailure}, " + + "systemError=${keyStoreError.isSystemError}" + ) + } + private fun migrateCipherSettings(fromRetry: Boolean) { if (prefs.migratedToDisablingKDF) { Log.i(TAG, "Already migrated to latest cipher settings") diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt index ceff850492..5c31039b7e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt @@ -56,12 +56,14 @@ fun DatabaseMigrationScreen( clearDataUtils: ClearDataUtils, exporter: LogExporter, fm: FragmentManager, + canRestoreAccount: Boolean, ) { val scope = rememberCoroutineScope() var showExportLogDialog by retain { mutableStateOf(false) } DatabaseMigration( state = migrationManager.migrationState.collectAsState().value, + canRestoreAccount = canRestoreAccount, onRetry = { migrationManager.requestMigration(fromRetry = true) }, @@ -95,6 +97,7 @@ fun DatabaseMigrationScreen( private fun DatabaseMigration( @PreviewParameter(DatabaseMigrationStateProvider::class) state: DatabaseMigrationManager.MigrationState, + canRestoreAccount: Boolean = true, onRetry: () -> Unit = {}, onExportLogs: () -> Unit = {}, onClearData: () -> Unit = {}, @@ -164,12 +167,18 @@ private fun DatabaseMigration( Spacer(Modifier.size(LocalDimensions.current.mediumSpacing)) - OutlineButton( - text = stringResource(R.string.clearDeviceRestore), - color = LocalColors.current.danger, - onClick = { showingClearDeviceRestoreWarning = true } - ) - Spacer(Modifier.size(LocalDimensions.current.xsSpacing)) + // Restoring replays the account from the network using the login state + // held in memory, so with no login state there is nothing to restore with + // and this would silently do what "clear and restart" does. Offering it + // then would promise an account recovery that cannot happen. + if (canRestoreAccount) { + OutlineButton( + text = stringResource(R.string.clearDeviceRestore), + color = LocalColors.current.danger, + onClick = { showingClearDeviceRestoreWarning = true } + ) + Spacer(Modifier.size(LocalDimensions.current.xsSpacing)) + } OutlineButton( text = stringResource(R.string.clearDeviceRestart), color = LocalColors.current.danger, diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt index adcb7c86f4..416c9dbf9f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt @@ -7,6 +7,7 @@ import androidx.compose.runtime.collectAsState import androidx.core.content.IntentCompat import dagger.hilt.android.AndroidEntryPoint import org.thoughtcrime.securesms.FullComposeActivity +import org.thoughtcrime.securesms.auth.LoginStateRepository import org.thoughtcrime.securesms.ui.components.LogExporter import org.thoughtcrime.securesms.util.ClearDataUtils import javax.inject.Inject @@ -22,6 +23,9 @@ class DatabaseMigrationStateActivity : FullComposeActivity() { @Inject lateinit var exporter: LogExporter + @Inject + lateinit var loginStateRepository: LoginStateRepository + @Composable override fun ComposeContent() { DatabaseMigrationScreen( @@ -29,6 +33,7 @@ class DatabaseMigrationStateActivity : FullComposeActivity() { fm = supportFragmentManager, exporter = exporter, clearDataUtils = clearDataUtils, + canRestoreAccount = loginStateRepository.peekLoginState() != null, ) val state = migrationManager.migrationState.collectAsState().value