Merge branch '254-key-rotation-crash' into 'master'

Don't reschedule the same TimerTask. #254

Also added some unit tests for TransportKeyManager (part of #205). Fixes #254.

See merge request !108

briar-core/src/org/briarproject/transport/TransportKeyManager.java

@@ -27,7 +27,7 @@ import static org.briarproject.api.transport.TransportConstants.MAX_CLOCK_DIFFER
 import static org.briarproject.api.transport.TransportConstants.TAG_LENGTH;
 import static org.briarproject.util.ByteUtils.MAX_32_BIT_UNSIGNED;
 
-class TransportKeyManager extends TimerTask {
+class TransportKeyManager {
 
 	private static final Logger LOG =
 			Logger.getLogger(TransportKeyManager.class.getName());
@@ -97,21 +97,14 @@ class TransportKeyManager extends TimerTask {
 			for (Entry<ContactId, TransportKeys> e : current.entrySet())
 				addKeys(e.getKey(), new MutableTransportKeys(e.getValue()));
 			// Write any rotated keys back to the DB
-			Transaction txn = db.startTransaction();
-			try {
-				db.updateTransportKeys(txn, rotated);
-				txn.setComplete();
-			} finally {
-				db.endTransaction(txn);
-			}
+			updateTransportKeys(rotated);
 		} catch (DbException e) {
 			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
 		} finally {
 			lock.unlock();
 		}
 		// Schedule the next key rotation
-		long delay = rotationPeriodLength - now % rotationPeriodLength;
-		timer.schedule(this, delay);
+		scheduleKeyRotation(now);
 	}
 
 	// Locking: lock
@@ -133,6 +126,29 @@ class TransportKeyManager extends TimerTask {
 		}
 	}
 
+	private void updateTransportKeys(Map<ContactId, TransportKeys> rotated)
+		throws DbException {
+		if (!rotated.isEmpty()) {
+			Transaction txn = db.startTransaction();
+			try {
+				db.updateTransportKeys(txn, rotated);
+				txn.setComplete();
+			} finally {
+				db.endTransaction(txn);
+			}
+		}
+	}
+
+	private void scheduleKeyRotation(long now) {
+		TimerTask task = new TimerTask() {
+			public void run() {
+				rotateKeys();
+			}
+		};
+		long delay = rotationPeriodLength - now % rotationPeriodLength;
+		timer.schedule(task, delay);
+	}
+
 	void addContact(ContactId c, SecretKey master, long timestamp,
 			boolean alice) {
 		lock.lock();
@@ -230,6 +246,7 @@ class TransportKeyManager extends TimerTask {
 			}
 			// Remove tags for any stream numbers removed from the window
 			for (long streamNumber : change.getRemoved()) {
+				if (streamNumber == tagCtx.streamNumber) continue;
 				byte[] removeTag = new byte[TAG_LENGTH];
 				crypto.encodeTag(removeTag, inKeys.getTagKey(), streamNumber);
 				inContexts.remove(new Bytes(removeTag));
@@ -253,8 +270,7 @@ class TransportKeyManager extends TimerTask {
 		}
 	}
 
-	@Override
-	public void run() {
+	private void rotateKeys() {
 		long now = clock.currentTimeMillis();
 		lock.lock();
 		try {
@@ -280,21 +296,14 @@ class TransportKeyManager extends TimerTask {
 			for (Entry<ContactId, TransportKeys> e : current.entrySet())
 				addKeys(e.getKey(), new MutableTransportKeys(e.getValue()));
 			// Write any rotated keys back to the DB
-			Transaction txn = db.startTransaction();
-			try {
-				db.updateTransportKeys(txn, rotated);
-				txn.setComplete();
-			} finally {
-				db.endTransaction(txn);
-			}
+			updateTransportKeys(rotated);
 		} catch (DbException e) {
 			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
 		} finally {
 			lock.unlock();
 		}
 		// Schedule the next key rotation
-		long delay = rotationPeriodLength - now % rotationPeriodLength;
-		timer.schedule(this, delay);
+		scheduleKeyRotation(now);
 	}
 
 	private static class TagContext {

briar-tests/src/org/briarproject/transport/ReorderingWindowTest.java

 package org.briarproject.transport;
 
 import org.briarproject.BriarTestCase;
-import org.briarproject.api.transport.TransportConstants;
 import org.briarproject.transport.ReorderingWindow.Change;
-import org.junit.Assert;
 import org.junit.Test;
-import org.briarproject.BriarTestCase;
-import org.junit.Test;
-
-import java.util.Collection;
-
-import static org.briarproject.api.transport.TransportConstants.REORDERING_WINDOW_SIZE;
-import static org.briarproject.util.ByteUtils.MAX_32_BIT_UNSIGNED;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 import java.util.Arrays;
 import java.util.Collections;
@@ -24,6 +10,7 @@ import java.util.Random;
 
 import static org.briarproject.api.transport.TransportConstants.REORDERING_WINDOW_SIZE;
 import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
 
 public class ReorderingWindowTest extends BriarTestCase {
 
@@ -46,7 +33,8 @@ public class ReorderingWindowTest extends BriarTestCase {
 		Change change = window.setSeen(0L);
 		// The window should slide by one element
 		assertEquals(1L, window.getBase());
-		assertEquals(Collections.singletonList((long) REORDERING_WINDOW_SIZE), change.getAdded());
+		assertEquals(Collections.singletonList((long) REORDERING_WINDOW_SIZE),
+				change.getAdded());
 		assertEquals(Collections.singletonList(0L), change.getRemoved());
 		// All elements in the window should be unseen
 		assertArrayEquals(bitmap, window.getBitmap());
@@ -76,7 +64,8 @@ public class ReorderingWindowTest extends BriarTestCase {
 		Change change = window.setSeen(aboveMidpoint);
 		// The window should slide by one element
 		assertEquals(1L, window.getBase());
-		assertEquals(Collections.singletonList((long) REORDERING_WINDOW_SIZE), change.getAdded());
+		assertEquals(Collections.singletonList((long) REORDERING_WINDOW_SIZE),
+				change.getAdded());
 		assertEquals(Arrays.asList(0L, aboveMidpoint), change.getRemoved());
 		// The highest element below the midpoint should be seen
 		bitmap[bitmap.length / 2 - 1] = (byte) 0x01; // 0000 0001