From 4d09abd0d3b2398f209feec2f8737410d5b9d289 Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Wed, 10 May 2023 15:34:50 -0400 Subject: [PATCH] Fix group state loss during concurrent updates. Storage sync and a message process can both attempt to create a group at the same time. Message processing caches the local group state and performs networking which allows the cached state to become stale. The state was being used to decide to call create instead of update and the create would fail silently as the group record already exists. This would cause state to not be persisted and result in odd double events. --- .../securesms/database/GroupTable.kt | 51 ++++++++++++------- .../securesms/database/RecipientTable.kt | 6 ++- .../securesms/groups/GroupManagerV2.java | 29 ++++++++--- .../v2/processing/GroupsV2StateProcessor.java | 8 ++- 4 files changed, 66 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt index d5b9a373a2..19df6bea3c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt @@ -60,6 +60,7 @@ import java.security.SecureRandom import java.util.Optional import java.util.UUID import java.util.stream.Collectors +import javax.annotation.CheckReturnValue class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseTable(context, databaseHelper), RecipientIdDatabaseReference { @@ -649,20 +650,23 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT } } - fun create(groupId: GroupId.V1, title: String?, members: Collection, avatar: SignalServiceAttachmentPointer?, relay: String?) { + @CheckReturnValue + fun create(groupId: GroupId.V1, title: String?, members: Collection, avatar: SignalServiceAttachmentPointer?, relay: String?): Boolean { if (groupExists(groupId.deriveV2MigrationGroupId())) { throw LegacyGroupInsertException(groupId) } - create(groupId, title, members, avatar, relay, null, null) + return create(groupId, title, members, avatar, relay, null, null) } - fun create(groupId: GroupId.Mms, title: String?, members: Collection) { - create(groupId, if (title.isNullOrEmpty()) null else title, members, null, null, null, null) + @CheckReturnValue + fun create(groupId: GroupId.Mms, title: String?, members: Collection): Boolean { + return create(groupId, if (title.isNullOrEmpty()) null else title, members, null, null, null, null) } @JvmOverloads - fun create(groupMasterKey: GroupMasterKey, groupState: DecryptedGroup, force: Boolean = false): GroupId.V2 { + @CheckReturnValue + fun create(groupMasterKey: GroupMasterKey, groupState: DecryptedGroup, force: Boolean = false): GroupId.V2? { val groupId = GroupId.v2(groupMasterKey) if (!force && getGroupV1ByExpectedV2(groupId).isPresent) { @@ -671,9 +675,11 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT Log.w(TAG, "Forcing the creation of a group even though we already have a V1 ID!") } - create(groupId, groupState.title, emptyList(), null, null, groupMasterKey, groupState) - - return groupId + return if (create(groupId = groupId, title = groupState.title, memberCollection = emptyList(), avatar = null, relay = null, groupMasterKey = groupMasterKey, groupState = groupState)) { + groupId + } else { + null + } } /** @@ -714,6 +720,7 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT /** * @param groupMasterKey null for V1, must be non-null for V2 (presence dictates group version). */ + @CheckReturnValue private fun create( groupId: GroupId, title: String?, @@ -722,7 +729,7 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT relay: String?, groupMasterKey: GroupMasterKey?, groupState: DecryptedGroup? - ) { + ): Boolean { val membershipValues = mutableListOf() val groupRecipientId = recipients.getOrInsertFromGroupId(groupId) val members: List = memberCollection.toSet().sorted() @@ -777,16 +784,20 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT } } - writableDatabase.withinTransaction { db -> - db.insert(TABLE_NAME, null, values) - SqlUtil.buildBulkInsert( - MembershipTable.TABLE_NAME, - arrayOf(MembershipTable.GROUP_ID, MembershipTable.RECIPIENT_ID), - membershipValues - ) - .forEach { - db.execSQL(it.where, it.whereArgs) - } + writableDatabase.beginTransaction() + try { + val result: Long = writableDatabase.insert(TABLE_NAME, null, values) + if (result < 1) { + Log.w(TAG, "Unable to create group, group record already exists") + return false + } + + for (query in SqlUtil.buildBulkInsert(MembershipTable.TABLE_NAME, arrayOf(MembershipTable.GROUP_ID, MembershipTable.RECIPIENT_ID), membershipValues)) { + writableDatabase.execSQL(query.where, query.whereArgs) + } + writableDatabase.setTransactionSuccessful() + } finally { + writableDatabase.endTransaction() } if (groupState != null && groupState.hasDisappearingMessagesTimer()) { @@ -799,6 +810,8 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT Recipient.live(groupRecipientId).refresh() notifyConversationListListeners() + + return true } fun update(groupId: GroupId.V1, title: String?, avatar: SignalServiceAttachmentPointer?) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt index d470f02fca..050fa9780a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt @@ -915,13 +915,17 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da val recipient = Recipient.externalGroupExact(groupId) Log.i(TAG, "Creating restore placeholder for $groupId") - groups.create( + val createdId = groups.create( masterKey, DecryptedGroup.newBuilder() .setRevision(GroupsV2StateProcessor.RESTORE_PLACEHOLDER_REVISION) .build() ) + if (createdId == null) { + Log.w(TAG, "Unable to create restore placeholder for $groupId, group already exists") + } + groups.setShowAsStoryState(groupId, insert.storySendMode.toShowAsStoryState()) updateExtras(recipient.id) { it.setHideStory(insert.shouldHideStory()) diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java index cd2470cf3c..791985d8be 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java @@ -294,10 +294,15 @@ final class GroupManagerV2 { throw new GroupChangeFailedException(e); } - GroupMasterKey masterKey = groupSecretParams.getMasterKey(); - GroupId.V2 groupId = groupDatabase.create(masterKey, decryptedGroup); - RecipientId groupRecipientId = SignalDatabase.recipients().getOrInsertFromGroupId(groupId); - Recipient groupRecipient = Recipient.resolved(groupRecipientId); + GroupMasterKey masterKey = groupSecretParams.getMasterKey(); + GroupId.V2 groupId = groupDatabase.create(masterKey, decryptedGroup); + + if (groupId == null) { + throw new GroupChangeFailedException("Unable to create group, group already exists"); + } + + RecipientId groupRecipientId = SignalDatabase.recipients().getOrInsertFromGroupId(groupId); + Recipient groupRecipient = Recipient.resolved(groupRecipientId); AvatarHelper.setAvatar(context, groupRecipientId, avatar != null ? new ByteArrayInputStream(avatar) : null); groupDatabase.onAvatarUpdated(groupId, avatar != null); @@ -946,8 +951,20 @@ final class GroupManagerV2 { } } } else { - groupDatabase.create(groupMasterKey, decryptedGroup); - Log.i(TAG, "Created local group with placeholder"); + GroupId.V2 groupId = groupDatabase.create(groupMasterKey, decryptedGroup); + if (groupId != null) { + Log.i(TAG, "Created local group with placeholder"); + } else { + Log.i(TAG, "Create placeholder failed, group suddenly present locally, attempting to apply change"); + if (decryptedChange != null) { + try { + groupsV2StateProcessor.forGroup(SignalStore.account().getServiceIds(), groupMasterKey) + .updateLocalGroupToRevision(decryptedChange.getRevision(), System.currentTimeMillis(), decryptedChange); + } catch (GroupNotAMemberException e) { + Log.w(TAG, "Unable to apply join change to existing group", e); + } + } + } } RecipientId groupRecipientId = SignalDatabase.recipients().getOrInsertFromGroupId(groupId); diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.java index 20ea7ffe1b..7505486e8f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.java @@ -21,11 +21,11 @@ import org.signal.storageservice.protos.groups.local.DecryptedPendingMember; import org.signal.storageservice.protos.groups.local.DecryptedPendingMemberRemoval; import org.signal.storageservice.protos.groups.local.DecryptedRequestingMember; import org.thoughtcrime.securesms.database.GroupTable; -import org.thoughtcrime.securesms.database.model.GroupRecord; import org.thoughtcrime.securesms.database.MessageTable; import org.thoughtcrime.securesms.database.RecipientTable; import org.thoughtcrime.securesms.database.SignalDatabase; import org.thoughtcrime.securesms.database.ThreadTable; +import org.thoughtcrime.securesms.database.model.GroupRecord; import org.thoughtcrime.securesms.database.model.databaseprotos.DecryptedGroupV2Context; import org.thoughtcrime.securesms.dependencies.ApplicationDependencies; import org.thoughtcrime.securesms.groups.GroupDoesNotExistException; @@ -554,7 +554,11 @@ public class GroupsV2StateProcessor { boolean needsAvatarFetch; if (inputGroupState.getLocalState() == null) { - groupDatabase.create(masterKey, newLocalState); + GroupId.V2 groupId = groupDatabase.create(masterKey, newLocalState); + if (groupId == null) { + Log.w(TAG, "Group create failed, trying to update"); + groupDatabase.update(masterKey, newLocalState); + } needsAvatarFetch = !TextUtils.isEmpty(newLocalState.getAvatar()); } else { groupDatabase.update(masterKey, newLocalState);