release electrum transport read lock during socket reads to avoid client request starvation
What the developer wrote
Authored by Craig Raw
release electrum transport read lock during socket reads to avoid client request starvation
What changed, and why it matters
This commit fixes a concurrency bug in Sparrow Wallet's Electrum server connection code. Previously, a single read lock was held continuously while waiting for data from the server, which could starve or block other client requests. The change releases that lock during socket reads and restructures the read loop so responses are delivered only when the lock is held. This is a reliability/performance fix that reduces the chance of the wallet becoming unresponsive or misbehaving when talking to an Electrum server, but it is not a direct theft-of-funds vulnerability.
Treat as a normal reliability fix. Reviewers may want to verify that the new lock/condition choreography has no missed signals, spurious wakeups, or races between close() and readInputLoop(), but the change appears to reduce rather than increase risk. No urgent security response is indicated by the diff alone.
Security signals we found
Concurrency/locking change in network transport layer
Potential denial-of-service or unresponsiveness due to lock starvation
No cryptographic, key-handling, or transaction-signing code modified
No input validation, deserialization, or memory-safety changes evident
Fix is defensive/reliability-oriented rather than a clear exploit patch
Evidence from the diff
TcpTransport.java previously held readLock for the entire duration of readInputLoop(), including blocking socket reads. readResponse() acquired the same lock and used Condition awaits to coordinate. The patch moves the BufferedReader creation outside the lock, releases readLock before the blocking readInputStream() calls, and introduces deliverResponse()/signalException() helpers that re-acquire the lock only to publish a completed response or exception. It also makes running and closed volatile, adds a running check in readResponse(), and makes close() signal all waiters and set flags before closing the socket. The stated goal is to avoid ‘client request starvation’ caused by holding the read lock during socket reads.
Changed components
src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.javaElectrum server TCP transport / JSON-RPC client read loopInspect captured patch +82 / −45
diff --git a/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java b/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java
index 1f7d052..2ce45f2 100644
--- a/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java
+++ b/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java
@@ -56,9 +56,9 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
private final Condition readingCondition = readLock.newCondition();
private final ReentrantLock clientRequestLock = new ReentrantLock();
- private boolean running = false;
+ private volatile boolean running = false;
private volatile boolean reading = true;
- private boolean closed = false;
+ private volatile boolean closed = false;
private boolean firstRead = true;
private int readTimeoutIndex;
private int requestIdCount = 1;
@@ -164,7 +164,7 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
firstRead = false;
}
- while(reading) {
+ while(reading && running) {
try {
readingCondition.await();
} catch(InterruptedException e) {
@@ -178,6 +178,10 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
throw new IOException("Error reading response: " + lastException.getMessage(), lastException);
}
+ if(!running) {
+ throw new IOException("Transport closed");
+ }
+
reading = true;
readingCondition.signal();
@@ -188,61 +192,85 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
}
public void readInputLoop() throws ServerException {
- readLock.lock();
- readReadySignal.countDown();
+ BufferedReader in;
+ try {
+ in = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8));
+ } catch(IOException e) {
+ if(!closed) {
+ log.error("Error opening socket inputstream", e);
+ }
+ if(running) {
+ signalException(e);
+ running = false;
+ }
+ return;
+ }
+ //Wait for first RPC request before starting to read. The lock must be acquired before
+ //signaling readiness so readResponse() blocks until we reach the atomic await/unlock.
+ readLock.lock();
try {
- try {
- //Don't start reading until first RPC request is sent
+ readReadySignal.countDown();
+ if(running) {
readingCondition.await();
- } catch(InterruptedException e) {
- Thread.currentThread().interrupt();
}
+ } catch(InterruptedException e) {
+ Thread.currentThread().interrupt();
+ return;
+ } finally {
+ readLock.unlock();
+ }
- BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8));
-
- while(running) {
- try {
- String received = readInputStream(in);
- wireLog.info("< " + received);
- if(isNotification(received)) {
- jsonRpcServer.handle(received, subscriptionService);
- } else {
- response = received;
- reading = false;
- readingCondition.signal();
- readingCondition.await();
- }
- } catch(InterruptedException e) {
- //Restore interrupt status and continue
- Thread.currentThread().interrupt();
- } catch(Exception e) {
+ while(running) {
+ try {
+ String received = readInputStream(in);
+ wireLog.info("< " + received);
+ if(isNotification(received)) {
+ jsonRpcServer.handle(received, subscriptionService);
+ } else {
+ deliverResponse(received);
+ }
+ } catch(InterruptedException e) {
+ //Restore interrupt status and continue
+ Thread.currentThread().interrupt();
+ } catch(Exception e) {
+ if(!closed) {
log.trace("Connection error while reading", e);
- if(running) {
- lastException = e;
- reading = false;
- readingCondition.signal();
- //Allow this thread to terminate as we will need to reconnect with a new transport anyway
- running = false;
- }
+ }
+ if(running) {
+ signalException(e);
+ //Allow this thread to terminate as we will need to reconnect with a new transport anyway
+ running = false;
}
}
- } catch(IOException e) {
- if(!closed) {
- log.error("Error opening socket inputstream", e);
- }
- if(running) {
- lastException = e;
- reading = false;
- readingCondition.signal();
- //Allow this thread to terminate as we will need to reconnect with a new transport anyway
- running = false;
+ }
+ }
+
+ private void deliverResponse(String received) throws InterruptedException {
+ readLock.lock();
+ try {
+ response = received;
+ reading = false;
+ readingCondition.signal();
+ while(!reading && running) {
+ readingCondition.await();
}
} finally {
readLock.unlock();
}
}
+ private void signalException(Exception e) {
+ readLock.lock();
+ try {
+ lastException = e;
+ reading = false;
+ readingCondition.signal();
+ } finally {
+ readLock.unlock();
+ }
+ }
+
protected String readInputStream(BufferedReader in) throws IOException {
String response = readLine(in);
@@ -303,10 +331,19 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
@Override
public void close() throws IOException {
+ running = false;
+ closed = true;
+
+ readLock.lock();
+ try {
+ readingCondition.signalAll();
+ } finally {
+ readLock.unlock();
+ }
+
if(socket != null) {
socket.close();
}
- closed = true;
}
@Override
Why this scored 31/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.