From a01fb7ff1cfd5b4ba9a9718686cea51ff9b21cf1 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Tue, 25 Apr 2023 15:52:09 -0400 Subject: [PATCH] Fix foreign key constraint issues with backup restore. --- .../securesms/backup/FullBackupImporter.java | 56 ++++++++++--------- ...essageRecipientsAndEditMessageMigration.kt | 6 +- .../newdevice/NewDeviceServerTask.java | 4 ++ .../newdevice/NewDeviceTransferFragment.java | 3 + .../fragments/RestoreBackupFragment.java | 7 +++ app/src/main/res/values/strings.xml | 4 ++ .../main/java/org/signal/core/util/SqlUtil.kt | 51 +++++++++++++++++ 7 files changed, 104 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/backup/FullBackupImporter.java b/app/src/main/java/org/thoughtcrime/securesms/backup/FullBackupImporter.java index bdef84e9cb..91e7593540 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/backup/FullBackupImporter.java +++ b/app/src/main/java/org/thoughtcrime/securesms/backup/FullBackupImporter.java @@ -38,6 +38,7 @@ import org.thoughtcrime.securesms.keyvalue.SignalStore; import org.thoughtcrime.securesms.profiles.AvatarHelper; import org.thoughtcrime.securesms.recipients.RecipientId; import org.thoughtcrime.securesms.util.BackupUtil; +import org.thoughtcrime.securesms.util.Util; import java.io.ByteArrayOutputStream; import java.io.File; @@ -80,6 +81,7 @@ public class FullBackupImporter extends FullBackupBase { SQLiteDatabase keyValueDatabase = KeyValueDatabase.getInstance(ApplicationDependencies.getApplication()).getSqlCipherDatabase(); + db.setForeignKeyConstraintsEnabled(false); db.beginTransaction(); keyValueDatabase.beginTransaction(); try { @@ -94,7 +96,7 @@ public class FullBackupImporter extends FullBackupBase { count++; if (frame.version != null) processVersion(db, frame.version); - else if (frame.statement != null) tryProcessStatement(db, frame.statement); + else if (frame.statement != null) processStatement(db, frame.statement); else if (frame.preference != null) processPreference(context, frame.preference); else if (frame.attachment != null) processAttachment(context, attachmentSecret, db, frame.attachment, inputStream); else if (frame.sticker != null) processSticker(context, attachmentSecret, db, frame.sticker, inputStream); @@ -106,8 +108,20 @@ public class FullBackupImporter extends FullBackupBase { db.setTransactionSuccessful(); keyValueDatabase.setTransactionSuccessful(); } finally { + List violations = SqlUtil.getForeignKeyViolations(db) + .stream() + .filter(it -> !it.getTable().startsWith("msl_")) + .collect(Collectors.toList()); + + if (violations.size() > 0) { + Log.w(TAG, "Foreign key constraints failed!\n" + Util.join(violations, "\n")); + //noinspection ThrowFromFinallyBlock + throw new ForeignKeyViolationException(violations); + } + db.endTransaction(); keyValueDatabase.endTransaction(); + db.setForeignKeyConstraintsEnabled(true); } EventBus.getDefault().post(new BackupEvent(BackupEvent.Type.FINISHED, count, 0)); @@ -129,31 +143,6 @@ public class FullBackupImporter extends FullBackupBase { db.setVersion(version.version); } - private static void tryProcessStatement(@NonNull SQLiteDatabase db, SqlStatement statement) { - try { - processStatement(db, statement); - } catch (SQLiteConstraintException e) { - String tableName = "?"; - String statementString = statement.statement; - - if (statementString != null && statementString.startsWith("INSERT INTO ")) { - int nameStart = "INSERT INTO ".length(); - int nameEnd = statementString.indexOf(" ", "INSERT INTO ".length()); - - if (nameStart < statementString.length() && nameEnd > nameStart) { - tableName = statementString.substring(nameStart, nameEnd); - } - } - - if (tableName.startsWith("msl_")) { - Log.w(TAG, "Constraint failed when inserting into " + tableName + ". Ignoring."); - } else { - Log.w(TAG, "Constraint failed when inserting into " + tableName + ". Throwing!"); - throw e; - } - } - } - private static void processStatement(@NonNull SQLiteDatabase db, SqlStatement statement) { if (statement.statement == null) { Log.w(TAG, "Null statement!"); @@ -373,4 +362,19 @@ public class FullBackupImporter extends FullBackupBase { super("Tried to import a backup with version " + backupVersion + " into a database with version " + currentVersion); } } + + public static class ForeignKeyViolationException extends IOException { + public ForeignKeyViolationException(List violations) { + super(buildMessage(violations)); + } + + private static String buildMessage(List violations) { + Set unique = violations + .stream() + .map(it -> it.getTable() + " -> " + it.getColumn()) + .collect(Collectors.toSet()); + + return Util.join(unique, ", "); + } + } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V185_MessageRecipientsAndEditMessageMigration.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V185_MessageRecipientsAndEditMessageMigration.kt index f422df73f1..06206faca8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V185_MessageRecipientsAndEditMessageMigration.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V185_MessageRecipientsAndEditMessageMigration.kt @@ -218,7 +218,11 @@ object V185_MessageRecipientsAndEditMessageMigration : SignalDatabaseMigration { } stopwatch.split("recreate-dependents") - db.execSQL("PRAGMA foreign_key_check") + val foreignKeyViolations: List = SqlUtil.getForeignKeyViolations(db, "message") + if (foreignKeyViolations.isNotEmpty()) { + Log.w(TAG, "Foreign key violations!\n${foreignKeyViolations.joinToString(separator = "\n")}") + throw IllegalStateException("Foreign key violations!") + } stopwatch.split("fk-check") stopwatch.stop(TAG) diff --git a/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceServerTask.java b/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceServerTask.java index bae96cd5f8..1c07ae452a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceServerTask.java +++ b/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceServerTask.java @@ -58,6 +58,9 @@ final class NewDeviceServerTask implements ServerTask { } catch (FullBackupImporter.DatabaseDowngradeException e) { Log.w(TAG, "Failed due to the backup being from a newer version of Signal.", e); EventBus.getDefault().post(new Status(0, Status.State.FAILURE_VERSION_DOWNGRADE)); + } catch (FullBackupImporter.ForeignKeyViolationException e) { + Log.w(TAG, "Failed due to foreign key constraint violations.", e); + EventBus.getDefault().post(new Status(0, Status.State.FAILURE_FOREIGN_KEY)); } catch (IOException e) { Log.w(TAG, e); EventBus.getDefault().post(new Status(0, Status.State.FAILURE_UNKNOWN)); @@ -99,6 +102,7 @@ final class NewDeviceServerTask implements ServerTask { IN_PROGRESS, SUCCESS, FAILURE_VERSION_DOWNGRADE, + FAILURE_FOREIGN_KEY, FAILURE_UNKNOWN } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceTransferFragment.java b/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceTransferFragment.java index 9baf718d67..5da93e0c4e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceTransferFragment.java +++ b/app/src/main/java/org/thoughtcrime/securesms/devicetransfer/newdevice/NewDeviceTransferFragment.java @@ -66,6 +66,9 @@ public final class NewDeviceTransferFragment extends DeviceTransferFragment { case FAILURE_VERSION_DOWNGRADE: abort(R.string.NewDeviceTransfer__cannot_transfer_from_a_newer_version_of_signal); break; + case FAILURE_FOREIGN_KEY: + abort(R.string.NewDeviceTransfer__failure_foreign_key); + break; case FAILURE_UNKNOWN: abort(); break; diff --git a/app/src/main/java/org/thoughtcrime/securesms/registration/fragments/RestoreBackupFragment.java b/app/src/main/java/org/thoughtcrime/securesms/registration/fragments/RestoreBackupFragment.java index df2fb7f2aa..bebf3863a9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/registration/fragments/RestoreBackupFragment.java +++ b/app/src/main/java/org/thoughtcrime/securesms/registration/fragments/RestoreBackupFragment.java @@ -303,6 +303,9 @@ public final class RestoreBackupFragment extends LoggingFragment { } catch (FullBackupImporter.DatabaseDowngradeException e) { Log.w(TAG, "Failed due to the backup being from a newer version of Signal.", e); return BackupImportResult.FAILURE_VERSION_DOWNGRADE; + } catch (FullBackupImporter.ForeignKeyViolationException e) { + Log.w(TAG, "Failed due to foreign key constraint violations.", e); + return BackupImportResult.FAILURE_FOREIGN_KEY; } catch (IOException e) { Log.w(TAG, e); return BackupImportResult.FAILURE_UNKNOWN; @@ -324,6 +327,9 @@ public final class RestoreBackupFragment extends LoggingFragment { case FAILURE_VERSION_DOWNGRADE: Toast.makeText(context, R.string.RegistrationActivity_backup_failure_downgrade, Toast.LENGTH_LONG).show(); break; + case FAILURE_FOREIGN_KEY: + Toast.makeText(context, R.string.RegistrationActivity_backup_failure_foreign_key, Toast.LENGTH_LONG).show(); + break; case FAILURE_UNKNOWN: Toast.makeText(context, R.string.RegistrationActivity_incorrect_backup_passphrase, Toast.LENGTH_LONG).show(); break; @@ -415,6 +421,7 @@ public final class RestoreBackupFragment extends LoggingFragment { private enum BackupImportResult { SUCCESS, FAILURE_VERSION_DOWNGRADE, + FAILURE_FOREIGN_KEY, FAILURE_UNKNOWN } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 655af88739..4a987a8c68 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -3488,6 +3488,8 @@ Enter backup passphrase Restore Cannot import backups from newer versions of Signal + + Backup contains malformed data Incorrect backup passphrase Checking… %d messages so far… @@ -3648,6 +3650,8 @@ Cannot transfer from a newer versions of Signal + + The transferred data was malformed Transferring data diff --git a/core-util/src/main/java/org/signal/core/util/SqlUtil.kt b/core-util/src/main/java/org/signal/core/util/SqlUtil.kt index 5946f9a93e..ce7afe1bc2 100644 --- a/core-util/src/main/java/org/signal/core/util/SqlUtil.kt +++ b/core-util/src/main/java/org/signal/core/util/SqlUtil.kt @@ -5,6 +5,7 @@ import android.text.TextUtils import androidx.annotation.VisibleForTesting import androidx.sqlite.db.SupportSQLiteDatabase import org.signal.core.util.logging.Log +import java.lang.Exception import java.util.LinkedList import java.util.Locale import java.util.stream.Collectors @@ -87,6 +88,27 @@ object SqlUtil { } } + /** + * Provides a list of all foreign key violations present. + * If a [targetTable] is specified, results will be limited to that table specifically. + * Otherwise, the check will be performed across all tables. + */ + @JvmStatic + @JvmOverloads + fun getForeignKeyViolations(db: SupportSQLiteDatabase, targetTable: String? = null): List { + val tableString = if (targetTable != null) "($targetTable)" else "" + + return db.query("PRAGMA foreign_key_check$tableString").readToList { cursor -> + val table = cursor.requireNonNullString("table") + ForeignKeyViolation( + table = table, + violatingRowId = cursor.requireLongOrNull("rowid"), + dependsOnTable = cursor.requireNonNullString("parent"), + column = getForeignKeyViolationColumn(db, table, cursor.requireLong("fkid")) + ) + } + } + @JvmStatic fun isEmpty(db: SupportSQLiteDatabase, table: String): Boolean { db.query("SELECT COUNT(*) FROM $table", null).use { cursor -> @@ -410,6 +432,21 @@ object SqlUtil { return Query(query, args.toTypedArray()) } + /** Helper that gets the specific column for a foreign key violation */ + private fun getForeignKeyViolationColumn(db: SupportSQLiteDatabase, table: String, id: Long): String? { + try { + db.query("PRAGMA foreign_key_list($table)").forEach { cursor -> + if (cursor.requireLong("id") == id) { + return cursor.requireString("from") + } + } + } catch (e: Exception) { + Log.w(TAG, "Failed to find violation details for id: $id") + } + + return null + } + class Query(val where: String, val whereArgs: Array) { infix fun and(other: Query): Query { return if (where.isNotEmpty() && other.where.isNotEmpty()) { @@ -422,6 +459,20 @@ object SqlUtil { } } + data class ForeignKeyViolation( + /** The table that declared the REFERENCES clause. */ + val table: String, + + /** The rowId of the message in [table] that violates the constraint. Will not be present if the table has now rowId. */ + val violatingRowId: Long?, + + /** The table that [table] has a dependency on. */ + val dependsOnTable: String, + + /** The column from [table] that has the constraint. A separate query needs to be made to get this, so it's best-effor. */ + val column: String? + ) + enum class CollectionOperator(val sql: String) { IN("IN"), NOT_IN("NOT IN")