From 7e3d3625aa8e66af97e898c1914dd234fd742a81 Mon Sep 17 00:00:00 2001
From: akwizgran <akwizgran@users.sourceforge.net>
Date: Thu, 31 Mar 2016 11:02:52 +0100
Subject: [PATCH] Don't allow reentrant transactions.
The database's transaction lock is reentrant, meaning that a thread that's already holding the lock can acquire it again. This would allow a thread that already has a transaction in progress to start another transaction, which could cause transaction isolation issues and/or lock timeouts on the database's internal locks.
Check that the current thread isn't already holding the lock when starting a transaction.
---
.../db/DatabaseComponentImpl.java | 7 ++-
.../db/DatabaseComponentImplTest.java | 50 +++++++++++++++++++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/briar-core/src/org/briarproject/db/DatabaseComponentImpl.java b/briar-core/src/org/briarproject/db/DatabaseComponentImpl.java
index d001599035..4247040468 100644
--- a/briar-core/src/org/briarproject/db/DatabaseComponentImpl.java
+++ b/briar-core/src/org/briarproject/db/DatabaseComponentImpl.java
@@ -59,7 +59,6 @@ import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.logging.Logger;
@@ -80,7 +79,8 @@ class DatabaseComponentImpl<T> implements DatabaseComponent {
private final EventBus eventBus;
private final ShutdownManager shutdown;
private final AtomicBoolean closed = new AtomicBoolean(false);
- private final ReadWriteLock lock = new ReentrantReadWriteLock(true);
+ private final ReentrantReadWriteLock lock =
+ new ReentrantReadWriteLock(true);
private volatile int shutdownHandle = -1;
@@ -119,6 +119,9 @@ class DatabaseComponentImpl<T> implements DatabaseComponent {
}
public Transaction startTransaction(boolean readOnly) throws DbException {
+ // Don't allow reentrant locking
+ if (lock.getReadHoldCount() > 0) throw new IllegalStateException();
+ if (lock.getWriteHoldCount() > 0) throw new IllegalStateException();
if (readOnly) lock.readLock().lock();
else lock.writeLock().lock();
try {
diff --git a/briar-tests/src/org/briarproject/db/DatabaseComponentImplTest.java b/briar-tests/src/org/briarproject/db/DatabaseComponentImplTest.java
index 02531d08d1..eed704adde 100644
--- a/briar-tests/src/org/briarproject/db/DatabaseComponentImplTest.java
+++ b/briar-tests/src/org/briarproject/db/DatabaseComponentImplTest.java
@@ -65,6 +65,7 @@ import static org.briarproject.api.transport.TransportConstants.REORDERING_WINDO
import static org.briarproject.db.DatabaseConstants.MAX_OFFERED_MESSAGES;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.fail;
public class DatabaseComponentImplTest extends BriarTestCase {
@@ -1447,4 +1448,53 @@ public class DatabaseComponentImplTest extends BriarTestCase {
context.assertIsSatisfied();
}
+
+ @Test(expected = IllegalStateException.class)
+ public void testCannotStartReadTransactionDuringReadTransaction()
+ throws Exception {
+ testCannotStartTransactionDuringTransaction(true, true);
+ }
+
+ @Test(expected = IllegalStateException.class)
+ public void testCannotStartWriteTransactionDuringReadTransaction()
+ throws Exception {
+ testCannotStartTransactionDuringTransaction(true, false);
+ }
+
+ @Test(expected = IllegalStateException.class)
+ public void testCannotStartReadTransactionDuringWriteTransaction()
+ throws Exception {
+ testCannotStartTransactionDuringTransaction(false, true);
+ }
+
+ @Test(expected = IllegalStateException.class)
+ public void testCannotStartWriteTransactionDuringWriteTransaction()
+ throws Exception {
+ testCannotStartTransactionDuringTransaction(false, false);
+ }
+
+ private void testCannotStartTransactionDuringTransaction(
+ boolean firstTxnReadOnly, boolean secondTxnReadOnly)
+ throws Exception {
+ Mockery context = new Mockery();
+ @SuppressWarnings("unchecked")
+ final Database<Object> database = context.mock(Database.class);
+ final ShutdownManager shutdown = context.mock(ShutdownManager.class);
+ final EventBus eventBus = context.mock(EventBus.class);
+
+ context.checking(new Expectations() {{
+ oneOf(database).startTransaction();
+ will(returnValue(new Object()));
+ }});
+
+ DatabaseComponent db = createDatabaseComponent(database, eventBus,
+ shutdown);
+
+ assertNotNull(db.startTransaction(firstTxnReadOnly));
+ try {
+ db.startTransaction(secondTxnReadOnly);
+ } finally {
+ context.assertIsSatisfied();
+ }
+ }
}
--
GitLab