Don't start transactions while holding locks. #272

briar-core/src/org/briarproject/plugins/ConnectionManagerImpl.java

@@ -2,6 +2,7 @@ package org.briarproject.plugins;
 
 import org.briarproject.api.TransportId;
 import org.briarproject.api.contact.ContactId;
+import org.briarproject.api.db.DbException;
 import org.briarproject.api.lifecycle.IoExecutor;
 import org.briarproject.api.plugins.ConnectionManager;
 import org.briarproject.api.plugins.ConnectionRegistry;
@@ -132,10 +133,14 @@ class ConnectionManagerImpl implements ConnectionManager {
 				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
 				disposeReader(true, false);
 				return;
+			} catch (DbException e) {
+				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+				disposeReader(true, false);
+				return;
 			}
 			if (ctx == null) {
 				LOG.info("Unrecognised tag");
-				disposeReader(true, false);
+				disposeReader(false, false);
 				return;
 			}
 			ContactId contactId = ctx.getContactId();
@@ -176,11 +181,17 @@ class ConnectionManagerImpl implements ConnectionManager {
 
 		public void run() {
 			// Allocate a stream context
-			StreamContext ctx = keyManager.getStreamContext(contactId,
-					transportId);
+			StreamContext ctx;
+			try {
+				ctx = keyManager.getStreamContext(contactId, transportId);
+			} catch (DbException e) {
+				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+				disposeWriter(true);
+				return;
+			}
 			if (ctx == null) {
 				LOG.warning("Could not allocate stream context");
-				disposeWriter(true);
+				disposeWriter(false);
 				return;
 			}
 			connectionRegistry.registerConnection(contactId, transportId);
@@ -232,10 +243,14 @@ class ConnectionManagerImpl implements ConnectionManager {
 				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
 				disposeReader(true, false);
 				return;
+			} catch (DbException e) {
+				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+				disposeReader(true, false);
+				return;
 			}
 			if (ctx == null) {
 				LOG.info("Unrecognised tag");
-				disposeReader(true, false);
+				disposeReader(false, false);
 				return;
 			}
 			contactId = ctx.getContactId();
@@ -261,11 +276,17 @@ class ConnectionManagerImpl implements ConnectionManager {
 
 		private void runOutgoingSession() {
 			// Allocate a stream context
-			StreamContext ctx = keyManager.getStreamContext(contactId,
-					transportId);
+			StreamContext ctx;
+			try {
+				ctx = keyManager.getStreamContext(contactId, transportId);
+			} catch (DbException e) {
+				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+				disposeWriter(true);
+				return;
+			}
 			if (ctx == null) {
 				LOG.warning("Could not allocate stream context");
-				disposeWriter(true);
+				disposeWriter(false);
 				return;
 			}
 			try {
@@ -320,11 +341,17 @@ class ConnectionManagerImpl implements ConnectionManager {
 
 		public void run() {
 			// Allocate a stream context
-			StreamContext ctx = keyManager.getStreamContext(contactId,
-					transportId);
+			StreamContext ctx;
+			try {
+				ctx = keyManager.getStreamContext(contactId, transportId);
+			} catch (DbException e) {
+				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+				disposeWriter(true);
+				return;
+			}
 			if (ctx == null) {
 				LOG.warning("Could not allocate stream context");
-				disposeWriter(true);
+				disposeWriter(false);
 				return;
 			}
 			connectionRegistry.registerConnection(contactId, transportId);
@@ -357,6 +384,10 @@ class ConnectionManagerImpl implements ConnectionManager {
 				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
 				disposeReader(true, true);
 				return;
+			} catch (DbException e) {
+				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+				disposeReader(true, true);
+				return;
 			}
 			// Unrecognised tags are suspicious in this case
 			if (ctx == null) {

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

@@ -22,7 +22,6 @@ import org.briarproject.api.system.Timer;
 import org.briarproject.api.transport.KeyManager;
 import org.briarproject.api.transport.StreamContext;
 
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -32,6 +31,7 @@ import java.util.logging.Logger;
 
 import javax.inject.Inject;
 
+import static java.util.logging.Level.INFO;
 import static java.util.logging.Level.WARNING;
 
 class KeyManagerImpl implements KeyManager, Service, EventListener {
@@ -65,31 +65,29 @@ class KeyManagerImpl implements KeyManager, Service, EventListener {
 
 	@Override
 	public boolean start() {
-		Map<TransportId, Integer> latencies =
+		Map<TransportId, Integer> transports =
 				new HashMap<TransportId, Integer>();
 		for (SimplexPluginFactory f : pluginConfig.getSimplexFactories())
-			latencies.put(f.getId(), f.getMaxLatency());
+			transports.put(f.getId(), f.getMaxLatency());
 		for (DuplexPluginFactory f : pluginConfig.getDuplexFactories())
-			latencies.put(f.getId(), f.getMaxLatency());
+			transports.put(f.getId(), f.getMaxLatency());
 		try {
-			Collection<Contact> contacts;
 			Transaction txn = db.startTransaction(false);
 			try {
-				contacts = db.getContacts(txn);
-				for (Entry<TransportId, Integer> e : latencies.entrySet())
+				for (Contact c : db.getContacts(txn))
+					if (c.isActive()) activeContacts.put(c.getId(), true);
+				for (Entry<TransportId, Integer> e : transports.entrySet())
 					db.addTransport(txn, e.getKey(), e.getValue());
+				for (Entry<TransportId, Integer> e : transports.entrySet()) {
+					TransportKeyManager m = new TransportKeyManager(db, crypto,
+							timer, clock, e.getKey(), e.getValue());
+					managers.put(e.getKey(), m);
+					m.start(txn);
+				}
 				txn.setComplete();
 			} finally {
 				db.endTransaction(txn);
 			}
-			for (Contact c : contacts)
-				if (c.isActive()) activeContacts.put(c.getId(), true);
-			for (Entry<TransportId, Integer> e : latencies.entrySet()) {
-				TransportKeyManager m = new TransportKeyManager(db, crypto,
-						timer, clock, e.getKey(), e.getValue());
-				managers.put(e.getKey(), m);
-				m.start();
-			}
 		} catch (DbException e) {
 			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
 			return false;
@@ -108,32 +106,43 @@ class KeyManagerImpl implements KeyManager, Service, EventListener {
 			m.addContact(txn, c, master, timestamp, alice);
 	}
 
-	public StreamContext getStreamContext(ContactId c, TransportId t) {
+	public StreamContext getStreamContext(ContactId c, TransportId t)
+			throws DbException {
 		// Don't allow outgoing streams to inactive contacts
 		if (!activeContacts.containsKey(c)) return null;
 		TransportKeyManager m = managers.get(t);
-		return m == null ? null : m.getStreamContext(c);
+		if (m == null) {
+			if (LOG.isLoggable(INFO)) LOG.info("No key manager for " + t);
+			return null;
+		}
+		StreamContext ctx = null;
+		Transaction txn = db.startTransaction(false);
+		try {
+			ctx = m.getStreamContext(txn, c);
+			txn.setComplete();
+		} finally {
+			db.endTransaction(txn);
+		}
+		return ctx;
 	}
 
-	public StreamContext getStreamContext(TransportId t, byte[] tag) {
+	public StreamContext getStreamContext(TransportId t, byte[] tag)
+			throws DbException {
 		TransportKeyManager m = managers.get(t);
-		if (m == null) return null;
-		StreamContext ctx = m.getStreamContext(tag);
-		if (ctx == null) return null;
-		// Activate the contact if not already active
-		if (!activeContacts.containsKey(ctx.getContactId())) {
-			try {
-				Transaction txn = db.startTransaction(false);
-				try {
-					db.setContactActive(txn, ctx.getContactId(), true);
-					txn.setComplete();
-				} finally {
-					db.endTransaction(txn);
-				}
-			} catch (DbException e) {
-				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
-				return null;
-			}
+		if (m == null) {
+			if (LOG.isLoggable(INFO)) LOG.info("No key manager for " + t);
+			return null;
+		}
+		StreamContext ctx = null;
+		Transaction txn = db.startTransaction(false);
+		try {
+			ctx = m.getStreamContext(txn, tag);
+			// Activate the contact if not already active
+			if (ctx != null && !activeContacts.containsKey(ctx.getContactId()))
+				db.setContactActive(txn, ctx.getContactId(), true);
+			txn.setComplete();
+		} finally {
+			db.endTransaction(txn);
 		}
 		return ctx;
 	}

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

@@ -60,46 +60,20 @@ class TransportKeyManager {
 		keys = new HashMap<ContactId, MutableTransportKeys>();
 	}
 
-	void start() {
+	void start(Transaction txn) throws DbException {
 		long now = clock.currentTimeMillis();
 		lock.lock();
 		try {
 			// Load the transport keys from the DB
-			Map<ContactId, TransportKeys> loaded;
-			try {
-				Transaction txn = db.startTransaction(true);
-				try {
-					loaded = db.getTransportKeys(txn, transportId);
-					txn.setComplete();
-				} finally {
-					db.endTransaction(txn);
-				}
-			} catch (DbException e) {
-				if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
-				return;
-			}
+			Map<ContactId, TransportKeys> loaded =
+					db.getTransportKeys(txn, transportId);
 			// Rotate the keys to the current rotation period
-			Map<ContactId, TransportKeys> rotated =
-					new HashMap<ContactId, TransportKeys>();
-			Map<ContactId, TransportKeys> current =
-					new HashMap<ContactId, TransportKeys>();
-			long rotationPeriod = now / rotationPeriodLength;
-			for (Entry<ContactId, TransportKeys> e : loaded.entrySet()) {
-				ContactId c = e.getKey();
-				TransportKeys k = e.getValue();
-				TransportKeys k1 = crypto.rotateTransportKeys(k,
-						rotationPeriod);
-				if (k1.getRotationPeriod() > k.getRotationPeriod())
-					rotated.put(c, k1);
-				current.put(c, k1);
-			}
+			RotationResult rotationResult = rotateKeys(loaded, now);
 			// Initialise mutable state for all contacts
-			for (Entry<ContactId, TransportKeys> e : current.entrySet())
-				addKeys(e.getKey(), new MutableTransportKeys(e.getValue()));
+			addKeys(rotationResult.current);
 			// Write any rotated keys back to the DB
-			updateTransportKeys(rotated);
-		} catch (DbException e) {
-			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+			if (!rotationResult.rotated.isEmpty())
+				db.updateTransportKeys(txn, rotationResult.rotated);
 		} finally {
 			lock.unlock();
 		}
@@ -107,6 +81,27 @@ class TransportKeyManager {
 		scheduleKeyRotation(now);
 	}
 
+	private RotationResult rotateKeys(Map<ContactId, TransportKeys> keys,
+			long now) {
+		RotationResult rotationResult = new RotationResult();
+		long rotationPeriod = now / rotationPeriodLength;
+		for (Entry<ContactId, TransportKeys> e : keys.entrySet()) {
+			ContactId c = e.getKey();
+			TransportKeys k = e.getValue();
+			TransportKeys k1 = crypto.rotateTransportKeys(k, rotationPeriod);
+			if (k1.getRotationPeriod() > k.getRotationPeriod())
+				rotationResult.rotated.put(c, k1);
+			rotationResult.current.put(c, k1);
+		}
+		return rotationResult;
+	}
+
+	// Locking: lock
+	private void addKeys(Map<ContactId, TransportKeys> m) {
+		for (Entry<ContactId, TransportKeys> e : m.entrySet())
+			addKeys(e.getKey(), new MutableTransportKeys(e.getValue()));
+	}
+
 	// Locking: lock
 	private void addKeys(ContactId c, MutableTransportKeys m) {
 		encodeTags(c, m.getPreviousIncomingKeys());
@@ -126,23 +121,21 @@ class TransportKeyManager {
 		}
 	}
 
-	private void updateTransportKeys(Map<ContactId, TransportKeys> rotated)
-		throws DbException {
-		if (!rotated.isEmpty()) {
-			Transaction txn = db.startTransaction(false);
-			try {
-				db.updateTransportKeys(txn, rotated);
-				txn.setComplete();
-			} finally {
-				db.endTransaction(txn);
-			}
-		}
-	}
-
 	private void scheduleKeyRotation(long now) {
 		TimerTask task = new TimerTask() {
 			public void run() {
-				rotateKeys();
+				try {
+					Transaction txn = db.startTransaction(false);
+					try {
+						rotateKeys(txn);
+						txn.setComplete();
+					} finally {
+						db.endTransaction(txn);
+					}
+				} catch (DbException e) {
+					if (LOG.isLoggable(WARNING))
+						LOG.log(WARNING, e.toString(), e);
+				}
 			}
 		};
 		long delay = rotationPeriodLength - now % rotationPeriodLength;
@@ -185,7 +178,8 @@ class TransportKeyManager {
 		}
 	}
 
-	StreamContext getStreamContext(ContactId c) {
+	StreamContext getStreamContext(Transaction txn, ContactId c)
+			throws DbException {
 		lock.lock();
 		try {
 			// Look up the outgoing keys for the contact
@@ -198,24 +192,16 @@ class TransportKeyManager {
 					outKeys.getStreamCounter());
 			// Increment the stream counter and write it back to the DB
 			outKeys.incrementStreamCounter();
-			Transaction txn = db.startTransaction(false);
-			try {
-				db.incrementStreamCounter(txn, c, transportId,
-						outKeys.getRotationPeriod());
-				txn.setComplete();
-			} finally {
-				db.endTransaction(txn);
-			}
+			db.incrementStreamCounter(txn, c, transportId,
+					outKeys.getRotationPeriod());
 			return ctx;
-		} catch (DbException e) {
-			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
-			return null;
 		} finally {
 			lock.unlock();
 		}
 	}
 
-	StreamContext getStreamContext(byte[] tag) {
+	StreamContext getStreamContext(Transaction txn, byte[] tag)
+			throws DbException {
 		lock.lock();
 		try {
 			// Look up the incoming keys for the tag
@@ -244,53 +230,33 @@ class TransportKeyManager {
 				inContexts.remove(new Bytes(removeTag));
 			}
 			// Write the window back to the DB
-			Transaction txn = db.startTransaction(false);
-			try {
-				db.setReorderingWindow(txn, tagCtx.contactId, transportId,
-						inKeys.getRotationPeriod(), window.getBase(),
-						window.getBitmap());
-				txn.setComplete();
-			} finally {
-				db.endTransaction(txn);
-			}
+			db.setReorderingWindow(txn, tagCtx.contactId, transportId,
+					inKeys.getRotationPeriod(), window.getBase(),
+					window.getBitmap());
 			return ctx;
-		} catch (DbException e) {
-			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
-			return null;
 		} finally {
 			lock.unlock();
 		}
 	}
 
-	private void rotateKeys() {
+	private void rotateKeys(Transaction txn) throws DbException {
 		long now = clock.currentTimeMillis();
 		lock.lock();
 		try {
 			// Rotate the keys to the current rotation period
-			Map<ContactId, TransportKeys> rotated =
+			Map<ContactId, TransportKeys> snapshot =
 					new HashMap<ContactId, TransportKeys>();
-			Map<ContactId, TransportKeys> current =
-					new HashMap<ContactId, TransportKeys>();
-			long rotationPeriod = now / rotationPeriodLength;
-			for (Entry<ContactId, MutableTransportKeys> e : keys.entrySet()) {
-				ContactId c = e.getKey();
-				TransportKeys k = e.getValue().snapshot();
-				TransportKeys k1 = crypto.rotateTransportKeys(k,
-						rotationPeriod);
-				if (k1.getRotationPeriod() > k.getRotationPeriod())
-					rotated.put(c, k1);
-				current.put(c, k1);
-			}
+			for (Entry<ContactId, MutableTransportKeys> e : keys.entrySet())
+				snapshot.put(e.getKey(), e.getValue().snapshot());
+			RotationResult rotationResult = rotateKeys(snapshot, now);
 			// Rebuild the mutable state for all contacts
 			inContexts.clear();
 			outContexts.clear();
 			keys.clear();
-			for (Entry<ContactId, TransportKeys> e : current.entrySet())
-				addKeys(e.getKey(), new MutableTransportKeys(e.getValue()));
+			addKeys(rotationResult.current);
 			// Write any rotated keys back to the DB
-			updateTransportKeys(rotated);
-		} catch (DbException e) {
-			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+			if (!rotationResult.rotated.isEmpty())
+				db.updateTransportKeys(txn, rotationResult.rotated);
 		} finally {
 			lock.unlock();
 		}
@@ -311,4 +277,14 @@ class TransportKeyManager {
 			this.streamNumber = streamNumber;
 		}
 	}
+
+	private static class RotationResult {
+
+		private final Map<ContactId, TransportKeys> current, rotated;
+
+		private RotationResult() {
+			current = new HashMap<ContactId, TransportKeys>();
+			rotated = new HashMap<ContactId, TransportKeys>();
+		}
+	}
 }