From 95670937c38e9818984c975308c9c046f8068604 Mon Sep 17 00:00:00 2001
From: Torsten Grote <t@grobox.de>
Date: Tue, 4 Oct 2016 17:40:50 -0300
Subject: [PATCH] Fix regression in IntroduceeManager
This was happening when the remote response arrives before the local
response is made and thus the local response needs to be send with the
ACK following. The problem was that we ACK was sent before the response
which is not allowed and resulted in the session being aborted by the
introducee. This was happening, because recursion is hard ;)
The fix is only restarting another protocol engine to send the ACK
after the first run has been completed.
An integration test was added to prevent such regression in the future
and to test this code path.
---
.../IntroductionIntegrationTest.java | 56 ++++++++++++++++++-
.../introduction/IntroduceeManager.java | 25 ++++++---
2 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/briar-android-tests/src/test/java/org/briarproject/introduction/IntroductionIntegrationTest.java b/briar-android-tests/src/test/java/org/briarproject/introduction/IntroductionIntegrationTest.java
index af71da19ec..364040e7ce 100644
--- a/briar-android-tests/src/test/java/org/briarproject/introduction/IntroductionIntegrationTest.java
+++ b/briar-android-tests/src/test/java/org/briarproject/introduction/IntroductionIntegrationTest.java
@@ -442,6 +442,54 @@ public class IntroductionIntegrationTest extends BriarTestCase {
}
}
+ @Test
+ public void testResponseAndAckInOneSession() throws Exception {
+ startLifecycles();
+
+ addDefaultIdentities();
+ addDefaultContacts();
+ addListeners(true, true);
+ addTransportProperties();
+
+ // make introduction
+ long time = clock.currentTimeMillis();
+ Contact introducee1 = contactManager0.getContact(contactId1);
+ Contact introducee2 = contactManager0.getContact(contactId2);
+ introductionManager0
+ .makeIntroduction(introducee1, introducee2, "Hi!", time);
+
+ // sync first request message
+ deliverMessage(sync0, contactId0, sync1, contactId1, "0 to 1");
+ eventWaiter.await(TIMEOUT, 1);
+ assertTrue(listener1.requestReceived);
+
+ // sync first response
+ deliverMessage(sync1, contactId1, sync0, contactId0, "1 to 0");
+ eventWaiter.await(TIMEOUT, 1);
+ assertTrue(listener0.response1Received);
+
+ // don't let 2 answer the request right away
+ // to have the response arrive first
+ listener2.answerRequests = false;
+
+ // sync second request message and first response
+ deliverMessage(sync0, contactId0, sync2, contactId2, 2, "0 to 2");
+ eventWaiter.await(TIMEOUT, 1);
+ assertTrue(listener2.requestReceived);
+
+ // answer request manually
+ introductionManager2
+ .acceptIntroduction(contactId0, listener2.sessionId, time);
+
+ // sync second response and ACK and make sure there is no abort
+ deliverMessage(sync2, contactId2, sync0, contactId0, 2, "2 to 0");
+ eventWaiter.await(TIMEOUT, 1);
+ assertTrue(listener0.response2Received);
+ assertFalse(listener0.aborted);
+
+ stopLifecycles();
+ }
+
@Test
public void testIntroductionToSameContact() throws Exception {
startLifecycles();
@@ -1169,6 +1217,8 @@ public class IntroductionIntegrationTest extends BriarTestCase {
private volatile boolean requestReceived = false;
private volatile boolean succeeded = false;
private volatile boolean aborted = false;
+ private volatile boolean answerRequests = true;
+ private volatile SessionId sessionId;
private final int introducee;
private final boolean accept;
@@ -1194,10 +1244,10 @@ public class IntroductionIntegrationTest extends BriarTestCase {
requestReceived = true;
IntroductionRequest ir = introEvent.getIntroductionRequest();
ContactId contactId = introEvent.getContactId();
- SessionId sessionId = ir.getSessionId();
+ sessionId = ir.getSessionId();
long time = clock.currentTimeMillis();
try {
- if (introducee == 1) {
+ if (introducee == 1 && answerRequests) {
if (accept) {
introductionManager1
.acceptIntroduction(contactId, sessionId,
@@ -1207,7 +1257,7 @@ public class IntroductionIntegrationTest extends BriarTestCase {
.declineIntroduction(contactId, sessionId,
time);
}
- } else if (introducee == 2) {
+ } else if (introducee == 2 && answerRequests) {
if (accept) {
introductionManager2
.acceptIntroduction(contactId, sessionId,
diff --git a/briar-core/src/org/briarproject/introduction/IntroduceeManager.java b/briar-core/src/org/briarproject/introduction/IntroduceeManager.java
index a01734cb13..4456a5671c 100644
--- a/briar-core/src/org/briarproject/introduction/IntroduceeManager.java
+++ b/briar-core/src/org/briarproject/introduction/IntroduceeManager.java
@@ -32,6 +32,7 @@ import org.briarproject.api.sync.GroupId;
import org.briarproject.api.sync.Message;
import org.briarproject.api.sync.MessageId;
import org.briarproject.api.system.Clock;
+import org.jetbrains.annotations.Nullable;
import java.io.IOException;
import java.security.GeneralSecurityException;
@@ -243,7 +244,7 @@ class IntroduceeManager {
result) throws DbException, FormatException {
// perform actions based on new local state
- performTasks(txn, result.localState);
+ BdfDictionary followUpAction = performTasks(txn, result.localState);
// save new local state
MessageId storageId =
@@ -269,13 +270,21 @@ class IntroduceeManager {
db.deleteMessage(txn, messageId);
db.deleteMessageMetadata(txn, messageId);
}
+
+ // process follow up action at the end if available
+ if (followUpAction != null) {
+ IntroduceeEngine engine = new IntroduceeEngine();
+ processStateUpdate(txn, null,
+ engine.onLocalAction(result.localState, followUpAction));
+ }
}
- private void performTasks(Transaction txn, BdfDictionary localState)
+ @Nullable
+ private BdfDictionary performTasks(Transaction txn, BdfDictionary localState)
throws FormatException, DbException {
if (!localState.containsKey(TASK) || localState.get(TASK) == NULL_VALUE)
- return;
+ return null;
// remember task and remove it from localState
long task = localState.getLong(TASK);
@@ -285,7 +294,7 @@ class IntroduceeManager {
if (localState.getBoolean(EXISTS)) {
// we have this contact already, so do not perform actions
LOG.info("We have this contact already, do not add");
- return;
+ return null;
}
// figure out who takes which role by comparing public keys
@@ -348,10 +357,9 @@ class IntroduceeManager {
BdfDictionary localAction = new BdfDictionary();
localAction.put(TYPE, TYPE_ACK);
- // start engine and process its state update
- IntroduceeEngine engine = new IntroduceeEngine();
- processStateUpdate(txn, null,
- engine.onLocalAction(localState, localAction));
+ // return follow up action to start engine
+ // and process its state update again
+ return localAction;
}
// we sent and received an ACK, so activate contact
@@ -394,6 +402,7 @@ class IntroduceeManager {
contactManager.removeContact(txn, contactId);
}
}
+ return null;
}
private SecretKey deriveSecretKey(byte[] publicKeyBytes,
--
GitLab