Skip to content

Commit a85a194

Browse files
bluestreak01claude
andcommitted
Fix recv early return and restore zero-copy unwrap
recv() returned 0 from the OK branch whenever SSLEngine.unwrap consumed input but produced no plaintext (for example after a TLS 1.3 NewSessionTicket). The premature return left buffered records sitting in the input buffer, so callers fell back to ioWait and could time out despite having decryptable data on hand. Keep looping instead so the next record is unwrapped. While in there, restore the zero-copy fast path: SSLEngine now decrypts straight into the caller's buffer through a placeholder ByteBuffer repointed at recv() time. The internal buffer is used only when a single TLS record does not fit in the caller's buffer, in which case it grows as needed and any leftover plaintext is drained on subsequent recv() calls. Add tests for the empty-OK loop continuation, the spill-and-grow path, and the existing overflow-then-drain path; share SSLEngine stub boilerplate via a StubSslEngine base. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent cfae5fd commit a85a194

2 files changed

Lines changed: 267 additions & 78 deletions

File tree

core/src/main/java/io/questdb/client/network/JavaTlsClientSocket.java

Lines changed: 47 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ public X509Certificate[] getAcceptedIssuers() {
7777
private static final int STATE_EMPTY = 0;
7878
private static final int STATE_PLAINTEXT = 1;
7979
private static final int STATE_TLS = 2;
80+
private final ByteBuffer callerOutputBuffer;
8081
private final Socket delegate;
8182
private final Logger log;
8283
private final ClientTlsConfiguration tlsConfig;
@@ -95,18 +96,19 @@ public X509Certificate[] getAcceptedIssuers() {
9596
this.log = log;
9697
this.tlsConfig = tlsConfig;
9798

98-
// wrapInputBuffer are just placeholders. we set the internal address, capacity and limit in send() and recv().
99-
// so read/write from/to a buffer supplied by the caller and avoid unnecessary memory copies.
100-
// also, handshake does not to read/write from/to these buffers so it does not matter if they have capacity = 0
101-
// during handshake.
99+
// wrapInputBuffer and callerOutputBuffer are just placeholders: their address, capacity and
100+
// limit are reset to point at the caller's buffer in send() and recv() respectively, so the
101+
// SSLEngine can read/write the caller's memory directly without an extra copy. They have
102+
// capacity = 0 during handshake because handshake does not touch them.
102103
this.wrapInputBuffer = ByteBuffer.allocateDirect(0);
103-
this.unwrapOutputBuffer = ByteBuffer.allocateDirect(0);
104+
this.callerOutputBuffer = ByteBuffer.allocateDirect(0);
104105

105-
// wrapOutputBuffer and unwrapInputBuffer are crated with capacity 0. why?
106-
// we allocate the actual memory only when starting a new TLS session.
107-
// this way we can reuse the same ByteBuffer instances for multiple TLS sessions.
106+
// wrapOutputBuffer, unwrapInputBuffer and unwrapOutputBuffer all back internal allocations
107+
// that are only created when starting a new TLS session, so the ByteBuffer instances can be
108+
// reused across sessions.
108109
this.wrapOutputBuffer = ByteBuffer.allocateDirect(0);
109110
this.unwrapInputBuffer = ByteBuffer.allocateDirect(0);
111+
this.unwrapOutputBuffer = ByteBuffer.allocateDirect(0);
110112
}
111113

112114
@Override
@@ -169,24 +171,36 @@ public int recv(long bufferPtr, int bufferLen) {
169171
assert sslEngine != null;
170172

171173
try {
174+
// Pending plaintext from a previous spill is held in the internal buffer. Drain it first.
172175
if (unwrapOutputBuffer.position() != 0) {
173176
return drainUnwrapOutputBuffer(bufferPtr, bufferLen);
174177
}
175178

179+
// Fast path: have the SSLEngine decrypt straight into the caller's buffer. We only fall
180+
// back to the internal spill buffer if a single TLS record does not fit in the caller's
181+
// buffer, in which case we drain the spill buffer to the caller and return.
182+
resetBufferToPointer(callerOutputBuffer, bufferPtr, bufferLen);
183+
ByteBuffer output = callerOutputBuffer;
184+
boolean spilling = false;
185+
176186
for (; ; ) {
177187
int n = readFromSocket();
178188
assert unwrapInputBuffer.position() == 0 : "unwrapInputBuffer is not compacted";
179189
int bytesAvailable = unwrapInputBuffer.limit();
180190
if (n < 0 && bytesAvailable == 0) {
191+
if (output.position() != 0) {
192+
return spilling ? drainUnwrapOutputBuffer(bufferPtr, bufferLen) : output.position();
193+
}
181194
return n;
182195
}
183-
184196
if (bytesAvailable == 0) {
185-
// nothing to unwrap, we are done
197+
if (output.position() != 0) {
198+
return spilling ? drainUnwrapOutputBuffer(bufferPtr, bufferLen) : output.position();
199+
}
186200
return 0;
187201
}
188202

189-
SSLEngineResult result = sslEngine.unwrap(unwrapInputBuffer, unwrapOutputBuffer);
203+
SSLEngineResult result = sslEngine.unwrap(unwrapInputBuffer, output);
190204

191205
// compact the TLS buffer
192206
int bytesConsumed = result.bytesConsumed();
@@ -199,35 +213,42 @@ public int recv(long bufferPtr, int bufferLen) {
199213

200214
switch (result.getStatus()) {
201215
case BUFFER_UNDERFLOW:
202-
if (unwrapOutputBuffer.position() != 0) {
203-
return drainUnwrapOutputBuffer(bufferPtr, bufferLen);
216+
if (output.position() != 0) {
217+
return spilling ? drainUnwrapOutputBuffer(bufferPtr, bufferLen) : output.position();
204218
}
205-
// we need more data to unwrap, let's return control to the caller
206219
return 0;
207220
case BUFFER_OVERFLOW:
208-
if (unwrapOutputBuffer.position() == 0) {
209-
// not even a single byte was written to the output buffer even the buffer is empty
210-
// apparently the output buffer cannot fit even a single TLS record. let's grow it and try again!
221+
if (output.position() != 0) {
222+
// Output already has plaintext: hand it to the caller and let the
223+
// unprocessed record be picked up on the next recv() call.
224+
return spilling ? drainUnwrapOutputBuffer(bufferPtr, bufferLen) : output.position();
225+
}
226+
if (spilling) {
227+
// Internal buffer cannot fit a single record either: grow and retry.
211228
growUnwrapOutputBuffer();
212-
break;
229+
} else {
230+
// Caller's buffer cannot fit a single record. Switch to the internal
231+
// spill buffer for this record (and any further records that fit), then
232+
// drain to the caller.
233+
output = unwrapOutputBuffer;
234+
spilling = true;
213235
}
214-
return drainUnwrapOutputBuffer(bufferPtr, bufferLen);
236+
break;
215237
case OK:
216-
if (unwrapOutputBuffer.position() != 0) {
217-
return drainUnwrapOutputBuffer(bufferPtr, bufferLen);
218-
}
219-
return 0;
238+
// Plaintext (if any) is accumulating in `output`. Keep looping so we either
239+
// batch more records or hit BUFFER_UNDERFLOW / BUFFER_OVERFLOW.
240+
break;
220241
case CLOSED:
221242
log.debug("SSL engine closed");
222243
// We received a TLS close notification from the server. We don't expect any further data from this connection.
223244
// If we have some previously unwrapped data then let's return it so the caller has a chance to process them.
224245
// If a caller calls recv() again and we have no remaining plaintext to return, we will return -1 so the
225246
// caller learned that the connection is closed.
226247
// If we have no plaintext data to return now then we can immediately indicate that we are done with the connection.
227-
if (unwrapOutputBuffer.position() == 0) {
228-
return -1;
248+
if (output.position() != 0) {
249+
return spilling ? drainUnwrapOutputBuffer(bufferPtr, bufferLen) : output.position();
229250
}
230-
return drainUnwrapOutputBuffer(bufferPtr, bufferLen);
251+
return -1;
231252
}
232253
}
233254
} catch (SSLException e) {

0 commit comments

Comments
 (0)