mirror
/
Signal-Android
kopia lustrzana https://github.com/ryukoposting/Signal-Android
1
0
Forkuj 0
Kod Zgłoszenia Packages Projekty Wydania Wiki Aktywność

Fix issues with StorageSyncV2 bookkeeping. 

1. I screwed up the comparators in the record processor. Pretty bad, glad this was caught.
2. Previously I was sort of keeping track of which local-only records were accounted for while I was merging, and then hoping everything worked out in the end. Now I just very directly take some set differences and retrieve the appropriate records, so it's clear that we should never fail certain validations.
3. Rev's the feature flag so we don't turn on something broken.
fork-5.53.8
Greyson Parrelli 2021-04-13 11:32:24 -04:00 zatwierdzone przez GitHub
rodzic fb316a22c6
commit 35c102aa98
Nie znaleziono w bazie danych klucza dla tego podpisu
ID klucza GPG: 4AEE18F83AFDEB23
14 zmienionych plików z 81 dodań i 40 usunięć
Pokaż statystyki Ściągnij plik aktualizacji Ściągnij plik porównania Expand all files Collapse all files

6
app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java
Wyświetl plik

@ -2737,7 +2737,7 @@ public class RecipientDatabase extends Database {
}
}
public void clearDirtyStateForRecords(@NonNull List<SignalStorageRecord> records) {
public void clearDirtyStateForStorageIds(@NonNull Collection<StorageId> ids) {
SQLiteDatabase db = databaseHelper.getWritableDatabase();
Preconditions.checkArgument(db.inTransaction(), "Database should already be in a transaction.");
@ -2747,8 +2747,8 @@ public class RecipientDatabase extends Database {
String query = STORAGE_SERVICE_ID + " = ?";
for (SignalRecord record : records) {
String[] args = SqlUtil.buildArgs(Base64.encodeBytes(record.getId().getRaw()));
for (StorageId id : ids) {
String[] args = SqlUtil.buildArgs(Base64.encodeBytes(id.getRaw()));
db.update(TABLE_NAME, values, query, args);
}
}

60
app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java
Wyświetl plik

@ -34,6 +34,7 @@ import org.thoughtcrime.securesms.storage.StorageSyncHelper.WriteOperationResult
import org.thoughtcrime.securesms.storage.StorageSyncModels;
import org.thoughtcrime.securesms.storage.StorageSyncValidations;
import org.thoughtcrime.securesms.transport.RetryLaterException;
import org.thoughtcrime.securesms.util.SetUtil;
import org.thoughtcrime.securesms.util.TextSecurePreferences;
import org.thoughtcrime.securesms.util.Util;
import org.whispersystems.libsignal.InvalidKeyException;
@ -54,10 +55,12 @@ import org.whispersystems.signalservice.internal.storage.protos.ManifestRecord;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.TimeUnit;
/**
@ -222,7 +225,6 @@ public class StorageSyncJobV2 extends BaseJob {
List<StorageId> localStorageIdsBeforeMerge = getAllLocalStorageIds(context, Recipient.self().fresh());
KeyDifferenceResult keyDifference = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), localStorageIdsBeforeMerge);
if (keyDifference.hasTypeMismatches()) {
Log.w(TAG, "[Remote Sync] Found type mismatches in the key sets! Scheduling a force push after this sync completes.");
needsForcePush = true;
@ -232,7 +234,6 @@ public class StorageSyncJobV2 extends BaseJob {
Log.i(TAG, "[Remote Sync] Retrieving records for key difference: " + keyDifference);
List<SignalStorageRecord> remoteOnly = accountManager.readStorageRecords(storageServiceKey, keyDifference.getRemoteOnlyKeys());
List<SignalStorageRecord> localOnly = buildLocalStorageRecords(context, self, keyDifference.getLocalOnlyKeys());
if (remoteOnly.size() != keyDifference.getRemoteOnlyKeys().size()) {
Log.w(TAG, "[Remote Sync] Could not find all remote-only records! Requested: " + keyDifference.getRemoteOnlyKeys().size() + ", Found: " + remoteOnly.size() + ". Scheduling a force push after this sync completes.");
@ -290,17 +291,26 @@ public class StorageSyncJobV2 extends BaseJob {
Log.i(TAG, "IDs : " + localStorageIdsBeforeMerge.size() + " IDs before merge, " + localStorageIdsAfterMerge.size() + " IDs after merge");
}
localOnly.removeAll(contactResult.getLocalMatches());
localOnly.removeAll(gv1Result.getLocalMatches());
localOnly.removeAll(gv2Result.getLocalMatches());
localOnly.removeAll(accountResult.getLocalMatches());
recipientDatabase.clearDirtyStateForRecords(localOnly);
Log.i(TAG, "[Remote Sync] After the conflict resolution, there are " + localOnly.size() + " local-only records remaining.");
//noinspection unchecked Stop yelling at my beautiful method signatures
mergeWriteOperation = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), localStorageIdsAfterMerge, localOnly, contactResult, gv1Result, gv2Result, accountResult);
mergeWriteOperation = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), localStorageIdsAfterMerge, contactResult, gv1Result, gv2Result, accountResult);
List<StorageId> postMergeLocalOnlyIds = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), mergeWriteOperation.getManifest().getStorageIds()).getLocalOnlyKeys();
List<StorageId> remoteInsertIds = Stream.of(mergeWriteOperation.getInserts()).map(SignalStorageRecord::getId).toList();
Set<StorageId> unhandledLocalOnlyIds = SetUtil.difference(postMergeLocalOnlyIds, remoteInsertIds);
if (unhandledLocalOnlyIds.size() > 0) {
Log.i(TAG, "[Remote Sync] After the conflict resolution, there are " + unhandledLocalOnlyIds.size() + " local-only records remaining that weren't otherwise inserted. Adding them as inserts.");
List<SignalStorageRecord> localOnly = buildLocalStorageRecords(context, self, unhandledLocalOnlyIds);
mergeWriteOperation = new WriteOperationResult(mergeWriteOperation.getManifest(),
Util.concatenatedList(mergeWriteOperation.getInserts(), localOnly),
mergeWriteOperation.getDeletes());
recipientDatabase.clearDirtyStateForStorageIds(unhandledLocalOnlyIds);
} else {
Log.i(TAG, "[Remote Sync] After the conflict resolution, there are no local-only records remaining.");
}
StorageSyncValidations.validate(mergeWriteOperation, remoteManifest, needsForcePush);
db.setTransactionSuccessful();
@ -409,7 +419,11 @@ public class StorageSyncJobV2 extends BaseJob {
DatabaseFactory.getStorageKeyDatabase(context).getAllKeys());
}
private static @NonNull List<SignalStorageRecord> buildLocalStorageRecords(@NonNull Context context, @NonNull Recipient self, @NonNull List<StorageId> ids) {
private static @NonNull List<SignalStorageRecord> buildLocalStorageRecords(@NonNull Context context, @NonNull Recipient self, @NonNull Collection<StorageId> ids) {
if (ids.isEmpty()) {
return Collections.emptyList();
}
RecipientDatabase recipientDatabase = DatabaseFactory.getRecipientDatabase(context);
StorageKeyDatabase storageKeyDatabase = DatabaseFactory.getStorageKeyDatabase(context);
@ -423,12 +437,12 @@ public class StorageSyncJobV2 extends BaseJob {
RecipientSettings settings = recipientDatabase.getByStorageId(id.getRaw());
if (settings != null) {
if (settings.getGroupType() == RecipientDatabase.GroupType.SIGNAL_V2 && settings.getSyncExtras().getGroupMasterKey() == null) {
Log.w(TAG, "Missing master key on gv2 recipient");
throw new MissingGv2MasterKeyError();
} else {
records.add(StorageSyncModels.localToRemoteRecord(settings));
}
} else {
Log.w(TAG, "Missing local recipient model! Type: " + id.getType());
throw new MissingRecipientModelError("Missing local recipient model! Type: " + id.getType());
}
break;
case ManifestRecord.Identifier.Type.ACCOUNT_VALUE:
@ -442,7 +456,7 @@ public class StorageSyncJobV2 extends BaseJob {
if (unknown != null) {
records.add(unknown);
} else {
Log.w(TAG, "Missing local unknown model! Type: " + id.getType());
throw new MissingUnknownModelError("Missing local unknown model! Type: " + id.getType());
}
break;
}
@ -451,6 +465,20 @@ public class StorageSyncJobV2 extends BaseJob {
return records;
}
private static final class MissingGv2MasterKeyError extends Error {}
private static final class MissingRecipientModelError extends Error {
public MissingRecipientModelError(String message) {
super(message);
}
}
private static final class MissingUnknownModelError extends Error {
public MissingUnknownModelError(String message) {
super(message);
}
}
public static final class Factory implements Job.Factory<StorageSyncJobV2> {
@Override
public @NonNull StorageSyncJobV2 create(@NonNull Parameters parameters, @NonNull Data data) {

2
app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java
Wyświetl plik

@ -141,7 +141,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
{
return 0;
} else {
return lhs.getAddress().getIdentifier().compareTo(rhs.getAddress().getIdentifier());
return 1;
}
}

5
app/src/main/java/org/thoughtcrime/securesms/storage/DefaultStorageRecordProcessor.java
Wyświetl plik

@ -45,7 +45,6 @@ abstract class DefaultStorageRecordProcessor<E extends SignalRecord> implements
public @NonNull Result<E> process(@NonNull Collection<E> remoteRecords, @NonNull StorageKeyGenerator keyGenerator) throws IOException {
List<E> remoteDeletes = new LinkedList<>();
List<StorageRecordUpdate<E>> remoteUpdates = new LinkedList<>();
List<E> localMatches = new LinkedList<>();
Set<E> matchedRecords = new TreeSet<>(this);
for (E remote : remoteRecords) {
@ -57,8 +56,6 @@ abstract class DefaultStorageRecordProcessor<E extends SignalRecord> implements
if (local.isPresent()) {
E merged = merge(remote, local.get(), keyGenerator);
localMatches.add(local.get());
if (matchedRecords.contains(local.get())) {
Log.w(TAG, "Multiple remote records map to the same local record! Marking this one for deletion. (Type: " + local.get().getClass().getSimpleName() + ")");
remoteDeletes.add(remote);
@ -79,7 +76,7 @@ abstract class DefaultStorageRecordProcessor<E extends SignalRecord> implements
}
}
return new Result<>(remoteUpdates, remoteDeletes, localMatches);
return new Result<>(remoteUpdates, remoteDeletes);
}
/**

2
app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java
Wyświetl plik

@ -117,7 +117,7 @@ public final class GroupV1RecordProcessor extends DefaultStorageRecordProcessor<
if (Arrays.equals(lhs.getGroupId(), rhs.getGroupId())) {
return 0;
} else {
return lhs.getGroupId()[0] - rhs.getGroupId()[0];
return 1;
}
}

2
app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java
Wyświetl plik

@ -117,7 +117,7 @@ public final class GroupV2RecordProcessor extends DefaultStorageRecordProcessor<
if (Arrays.equals(lhs.getMasterKeyBytes(), rhs.getMasterKeyBytes())) {
return 0;
} else {
return lhs.getMasterKeyBytes()[0] - rhs.getMasterKeyBytes()[0];
return 1;
}
}

10
app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java
Wyświetl plik

@ -23,12 +23,10 @@ public interface StorageRecordProcessor<E extends SignalRecord> {
final class Result<E extends SignalRecord> {
private final Collection<StorageRecordUpdate<E>> remoteUpdates;
private final Collection<E> remoteDeletes;
private final Collection<SignalStorageRecord> localMatches;
Result(@NonNull Collection<StorageRecordUpdate<E>> remoteUpdates, @NonNull Collection<E> remoteDeletes, @NonNull Collection<E> localMatches) {
Result(@NonNull Collection<StorageRecordUpdate<E>> remoteUpdates, @NonNull Collection<E> remoteDeletes) {
this.remoteDeletes = remoteDeletes;
this.remoteUpdates = remoteUpdates;
this.localMatches = Stream.of(localMatches).map(SignalRecord::asStorageRecord).toList();
}
public @NonNull Collection<E> getRemoteDeletes() {
@ -39,10 +37,6 @@ public interface StorageRecordProcessor<E extends SignalRecord> {
return remoteUpdates;
}
public @NonNull Collection<SignalStorageRecord> getLocalMatches() {
return localMatches;
}
public boolean isLocalOnly() {
return remoteUpdates.isEmpty() && remoteDeletes.isEmpty();
}
@ -61,7 +55,7 @@ public interface StorageRecordProcessor<E extends SignalRecord> {
builder.append("- ").append(update.toString()).append("\n");
}
return super.toString();
return builder.toString();
}
}
}

9
app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java
Wyświetl plik

@ -355,10 +355,9 @@ public final class StorageSyncHelper {
*/
public static @NonNull WriteOperationResult createWriteOperation(long currentManifestVersion,
@NonNull List<StorageId> allStorageKeys,
@NonNull List<SignalStorageRecord> localOnlyRecords,
@NonNull StorageRecordProcessor.Result<? extends SignalRecord>... results)
{
Set<SignalStorageRecord> inserts = new LinkedHashSet<>(localOnlyRecords);
Set<SignalStorageRecord> inserts = new LinkedHashSet<>();
Set<StorageId> deletes = new LinkedHashSet<>();
for (StorageRecordProcessor.Result<? extends SignalRecord> result : results) {
@ -682,9 +681,9 @@ public final class StorageSyncHelper {
private final List<SignalStorageRecord> inserts;
private final List<byte[]> deletes;
private WriteOperationResult(@NonNull SignalStorageManifest manifest,
@NonNull List<SignalStorageRecord> inserts,
@NonNull List<byte[]> deletes)
public WriteOperationResult(@NonNull SignalStorageManifest manifest,
@NonNull List<SignalStorageRecord> inserts,
@NonNull List<byte[]> deletes)
{
this.manifest = manifest;
this.inserts = inserts;

5
app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java
Wyświetl plik

@ -7,6 +7,7 @@ import com.annimon.stream.Stream;
import org.signal.core.util.logging.Log;
import org.thoughtcrime.securesms.recipients.Recipient;
import org.thoughtcrime.securesms.transport.RetryLaterException;
import org.thoughtcrime.securesms.util.Base64;
import org.thoughtcrime.securesms.util.SetUtil;
import org.whispersystems.libsignal.util.guava.Optional;
@ -66,10 +67,12 @@ public final class StorageSyncValidations {
Set<ByteBuffer> writeDeletes = Stream.of(result.getDeletes()).map(ByteBuffer::wrap).collect(Collectors.toSet());
if (writeInserts.size() > insertedIds.size()) {
Log.w(TAG, "WriteInserts: " + writeInserts.size() + ", InsertedIds: " + insertedIds.size());
throw new MoreInsertsThanExpectedError();
}
if (writeInserts.size() < insertedIds.size()) {
Log.w(TAG, "WriteInserts: " + writeInserts.size() + ", InsertedIds: " + insertedIds.size());
throw new LessInsertsThanExpectedError();
}
@ -78,10 +81,12 @@ public final class StorageSyncValidations {
}
if (writeDeletes.size() > deletedIds.size()) {
Log.w(TAG, "WriteDeletes: " + writeDeletes.size() + ", DeletedIds: " + deletedIds.size());
throw new MoreDeletesThanExpectedError();
}
if (writeDeletes.size() < deletedIds.size()) {
Log.w(TAG, "WriteDeletes: " + writeDeletes.size() + ", DeletedIds: " + deletedIds.size());
throw new LessDeletesThanExpectedError();
}

2
app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java
Wyświetl plik

@ -74,7 +74,7 @@ public final class FeatureFlags {
private static final String ANIMATED_STICKER_MIN_TOTAL_MEMORY = "android.animatedStickerMinTotalMemory";
private static final String MESSAGE_PROCESSOR_ALARM_INTERVAL = "android.messageProcessor.alarmIntervalMins";
private static final String MESSAGE_PROCESSOR_DELAY = "android.messageProcessor.foregroundDelayMs";
private static final String STORAGE_SYNC_V2 = "android.storageSyncV2";
private static final String STORAGE_SYNC_V2 = "android.storageSyncV2.2";
/**
* We will only store remote values for flags in this set. If you want a flag to be controllable

5
libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java
Wyświetl plik

@ -12,6 +12,7 @@ import org.whispersystems.signalservice.api.util.UuidUtil;
import org.whispersystems.signalservice.internal.storage.protos.AccountRecord;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
@ -64,6 +65,10 @@ public final class SignalAccountRecord implements SignalRecord {
SignalAccountRecord that = (SignalAccountRecord) other;
List<String> diff = new LinkedList<>();
if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) {
diff.add("ID");
}
if (!Objects.equals(this.givenName, that.givenName)) {
diff.add("GivenName");
}

5
libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java
Wyświetl plik

@ -10,6 +10,7 @@ import org.whispersystems.signalservice.api.util.UuidUtil;
import org.whispersystems.signalservice.internal.storage.protos.ContactRecord;
import org.whispersystems.signalservice.internal.storage.protos.ContactRecord.IdentityState;
import java.util.Arrays;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
@ -56,6 +57,10 @@ public final class SignalContactRecord implements SignalRecord {
SignalContactRecord that = (SignalContactRecord) other;
List<String> diff = new LinkedList<>();
if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) {
diff.add("ID");
}
if (!Objects.equals(this.getAddress().getNumber(), that.getAddress().getNumber())) {
diff.add("E164");
}

4
libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java
Wyświetl plik

@ -40,6 +40,10 @@ public final class SignalGroupV1Record implements SignalRecord {
SignalGroupV1Record that = (SignalGroupV1Record) other;
List<String> diff = new LinkedList<>();
if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) {
diff.add("ID");
}
if (!Arrays.equals(this.groupId, that.groupId)) {
diff.add("MasterKey");
}

4
libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java
Wyświetl plik

@ -42,6 +42,10 @@ public final class SignalGroupV2Record implements SignalRecord {
SignalGroupV2Record that = (SignalGroupV2Record) other;
List<String> diff = new LinkedList<>();
if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) {
diff.add("ID");
}
if (!Arrays.equals(this.getMasterKeyBytes(), that.getMasterKeyBytes())) {
diff.add("MasterKey");
}