Skip to content

Commit b665b8c

Browse files
bluestreak01claude
andcommitted
Harden QWP client decoder, binds, and close paths
Tier 1 findings from the QWP client review, consolidated into one change so client and server wire formats stay aligned. Decoder (QwpResultBatchDecoder) - Non-delta parseSymbolColumn now rejects a dict size that is negative or exceeds rowCount, and rejects an entry length that is negative or above Integer.MAX_VALUE. Previously dictSize*8 could overflow int and bypass ensureOwnedEntriesAddr; a signed-wrap entryLen could slip the p + entryLen > limit check and advance p backwards through already- consumed bytes. - parseDeltaSymbolDict caps entryLen at Integer.MAX_VALUE before the int cast, computes newHeapPos in long space, and rejects growth beyond MAX_CONN_DICT_HEAP_BYTES. connDictSize is capped at MAX_CONN_DICT_SIZE. These guards keep a server that fails to emit CACHE_RESET from wrapping connDictHeapPos negative and letting copyMemory write past the heap. - GEOHASH decode rejects precisionBits outside [1, 60], mirroring the server's own range check. Bind encoder (QwpBindValues) - checkScale split into per-width variants (DECIMAL64 <= 18, DECIMAL128 <= 38, DECIMAL256 <= 76) so out-of-range scales are rejected at the type boundary instead of silently passing through under the shared MAX_SCALE cap. - setNull for DECIMAL64/128/256 emits the trailing scale byte and setNull for GEOHASH emits the precision_bits varint. The server reads both fields unconditionally before checking the null flag, so omitting them misframed every bind that followed in the batch. - setDecimal128(Decimal128) and setDecimal256(Decimal256) now preserve value.getScale() on the null-sentinel path. - New explicit helpers setNullDecimal64/128/256(int, int scale) and setNullGeohash(int, int precisionBits) let callers pin the type's scale/precision without a non-null value. - setGeohash masks value to precisionBits before emitting, so bits above the declared precision cannot leak into the top wire byte when precisionBits is not a multiple of 8. Buffer (QwpBatchBuffer) - ensureCapacity rejects a negative required length, starts doubling at max(current, 1) so a zero initial capacity can still grow, and computes the doubling in long space with a clamp at Integer.MAX_VALUE. The old int doubling spun forever at 0 and wrapped negative above 2^30. Concurrency (QwpQueryClient, QwpEgressIoThread) - close() is gated by an AtomicBoolean so concurrent or repeat calls no longer walk the shutdown body twice. - GenerationListener now owns its own AtomicReference<TerminalFailure> instead of sharing one across generations. An orphaned-but-in-flight listener's compareAndSet lands on a ref nobody reads, so a late callback from a dying I/O thread cannot poison the next connection. - releaseBuffer re-checks closed after the freeBuffers.offer and removes the buffer back out if closePool raced it; the buffer is closed in place rather than stranded in a drained-and-abandoned pool. ArrayBlockingQueue.remove gives us the atomicity we need to avoid a double close when closePool's drain beat us to the same buffer. Regression tests - QwpBindEncoderTest: new coverage for per-width scale rejection (DECIMAL64 > 18, DECIMAL128 > 38, DECIMAL256 accepts 76), the four typed setNullDecimal*/setNullGeohash helpers, the geohash masking at sub-byte and 60-bit precisions, and updated expectations for the NULL path of the convenience Decimal128/256 overloads and the exhaustive null-types walk. - QwpResultBatchDecoderHardeningTest: new frames for non-delta dict-size-above-rowCount, delta entry length > Integer.MAX_VALUE, and GEOHASH precision 0 / 61. - QwpBatchBufferTest: new file covering grow-from-zero, reject- negative, and bounded-time growth. - QwpQueryClientUnitTest: new concurrent-close stress test. - QwpEgressIoThreadCloseRaceTest: new file; 200 iterations racing releaseBuffer against closePool under assertMemoryLeak so a stranded buffer surfaces as a native-memory leak. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ee8876e commit b665b8c

10 files changed

Lines changed: 944 additions & 59 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpBatchBuffer.java

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,31 @@ public long getScratchAddr() {
9393
}
9494

9595
private void ensureCapacity(int required) {
96+
if (required < 0) {
97+
// A negative request cannot be honoured. Reject loudly rather than
98+
// silently wrapping through the doubling loop.
99+
throw new IllegalArgumentException("QwpBatchBuffer required capacity must be non-negative: " + required);
100+
}
96101
if (required <= scratchCapacity) return;
97-
int newCap = scratchCapacity;
98-
while (newCap < required) newCap *= 2;
99-
scratchAddr = Unsafe.realloc(scratchAddr, scratchCapacity, newCap, MemoryTag.NATIVE_DEFAULT);
100-
scratchCapacity = newCap;
102+
// Start the doubling at max(current, 1) so a buffer constructed with
103+
// initialCapacity=0 can still grow (0 *= 2 would spin forever). Cap the
104+
// double step against Integer.MAX_VALUE because `newCap *= 2` wraps
105+
// negative once newCap passes 2^30, at which point the while loop
106+
// could never reach `required` and would spin indefinitely.
107+
long newCap = Math.max(scratchCapacity, 1);
108+
while (newCap < required) {
109+
newCap <<= 1;
110+
if (newCap > Integer.MAX_VALUE) {
111+
newCap = Integer.MAX_VALUE;
112+
break;
113+
}
114+
}
115+
if (newCap < required) {
116+
throw new OutOfMemoryError("QwpBatchBuffer required capacity " + required
117+
+ " exceeds Integer.MAX_VALUE");
118+
}
119+
int capped = (int) newCap;
120+
scratchAddr = Unsafe.realloc(scratchAddr, scratchCapacity, capped, MemoryTag.NATIVE_DEFAULT);
121+
scratchCapacity = capped;
101122
}
102123
}

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpBindValues.java

Lines changed: 139 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,22 @@
6060
*/
6161
public final class QwpBindValues implements QuietCloseable {
6262

63+
/**
64+
* Maximum scale for a DECIMAL128 bind. DECIMAL128 stores up to 38 digits of
65+
* precision, so scale above 38 is mathematically invalid even though the
66+
* wire format can carry any byte value.
67+
*/
68+
private static final int DECIMAL128_MAX_SCALE = 38;
69+
/**
70+
* Maximum scale for a DECIMAL256 bind. DECIMAL256 is the widest decimal
71+
* type and its scale cap matches {@link Decimals#MAX_SCALE} (76).
72+
*/
73+
private static final int DECIMAL256_MAX_SCALE = Decimals.MAX_SCALE;
74+
/**
75+
* Maximum scale for a DECIMAL64 bind. DECIMAL64 stores up to 18 digits of
76+
* precision, so scale above 18 is mathematically invalid.
77+
*/
78+
private static final int DECIMAL64_MAX_SCALE = 18;
6379
/**
6480
* Maximum GEOHASH precision in bits, matching the server's
6581
* {@code ColumnType.GEOLONG_MAX_BITS} check.
@@ -108,7 +124,7 @@ public QwpBindValues setDate(int index, long millisSinceEpoch) {
108124
}
109125

110126
public QwpBindValues setDecimal128(int index, int scale, long lo, long hi) {
111-
checkScale(scale);
127+
checkScale128(scale);
112128
advance(index);
113129
writeHeader(QwpConstants.TYPE_DECIMAL128, false);
114130
writer.putByte((byte) scale);
@@ -120,18 +136,21 @@ public QwpBindValues setDecimal128(int index, int scale, long lo, long hi) {
120136
/**
121137
* Convenience overload that takes the scale and limbs from the supplied
122138
* {@link Decimal128}. If the value is NULL (Decimal128's canonical NULL
123-
* sentinel), an explicit DECIMAL128 NULL is encoded; otherwise the scale
124-
* and 128-bit unscaled value are encoded as-is.
139+
* sentinel), an explicit DECIMAL128 NULL is encoded preserving
140+
* {@code value.getScale()}; a {@code null} reference encodes with scale 0.
125141
*/
126142
public QwpBindValues setDecimal128(int index, Decimal128 value) {
127-
if (value == null || Decimal128.isNull(value.getHigh(), value.getLow())) {
128-
return setNull(index, QwpConstants.TYPE_DECIMAL128);
143+
if (value == null) {
144+
return setNullDecimal128(index, 0);
145+
}
146+
if (Decimal128.isNull(value.getHigh(), value.getLow())) {
147+
return setNullDecimal128(index, value.getScale());
129148
}
130149
return setDecimal128(index, value.getScale(), value.getLow(), value.getHigh());
131150
}
132151

133152
public QwpBindValues setDecimal256(int index, int scale, long ll, long lh, long hl, long hh) {
134-
checkScale(scale);
153+
checkScale256(scale);
135154
advance(index);
136155
writeHeader(QwpConstants.TYPE_DECIMAL256, false);
137156
writer.putByte((byte) scale);
@@ -145,17 +164,21 @@ public QwpBindValues setDecimal256(int index, int scale, long ll, long lh, long
145164
/**
146165
* Convenience overload that takes the scale and limbs from the supplied
147166
* {@link Decimal256}. If the value is NULL (Decimal256's canonical NULL
148-
* sentinel), an explicit DECIMAL256 NULL is encoded.
167+
* sentinel), an explicit DECIMAL256 NULL is encoded preserving
168+
* {@code value.getScale()}; a {@code null} reference encodes with scale 0.
149169
*/
150170
public QwpBindValues setDecimal256(int index, Decimal256 value) {
151-
if (value == null || Decimal256.isNull(value.getHh(), value.getHl(), value.getLh(), value.getLl())) {
152-
return setNull(index, QwpConstants.TYPE_DECIMAL256);
171+
if (value == null) {
172+
return setNullDecimal256(index, 0);
173+
}
174+
if (Decimal256.isNull(value.getHh(), value.getHl(), value.getLh(), value.getLl())) {
175+
return setNullDecimal256(index, value.getScale());
153176
}
154177
return setDecimal256(index, value.getScale(), value.getLl(), value.getLh(), value.getHl(), value.getHh());
155178
}
156179

157180
public QwpBindValues setDecimal64(int index, int scale, long unscaledValue) {
158-
checkScale(scale);
181+
checkScale64(scale);
159182
advance(index);
160183
writeHeader(QwpConstants.TYPE_DECIMAL64, false);
161184
writer.putByte((byte) scale);
@@ -181,19 +204,21 @@ public QwpBindValues setFloat(int index, float value) {
181204
* Encodes a GEOHASH bind with the given precision (in bits) and packed
182205
* value. The value is stored little-endian in {@code ceil(precisionBits / 8)}
183206
* bytes. Precision must be in {@code [1, 60]}.
207+
* <p>
208+
* {@code value} is masked to {@code precisionBits} before encoding, so bits
209+
* above the declared precision cannot leak into the top wire byte (which
210+
* would otherwise pass through when {@code precisionBits} is not a multiple
211+
* of 8).
184212
*/
185213
public QwpBindValues setGeohash(int index, int precisionBits, long value) {
186-
if (precisionBits < GEOHASH_MIN_BITS || precisionBits > GEOHASH_MAX_BITS) {
187-
throw new IllegalArgumentException(
188-
"GEOHASH precision must be in [" + GEOHASH_MIN_BITS + ", " + GEOHASH_MAX_BITS
189-
+ "], got " + precisionBits);
190-
}
214+
checkGeohashPrecision(precisionBits);
191215
advance(index);
192216
writeHeader(QwpConstants.TYPE_GEOHASH, false);
193217
writer.putVarint(precisionBits);
218+
long masked = maskGeohashBits(value, precisionBits);
194219
int byteCount = (precisionBits + 7) >>> 3;
195220
for (int b = 0; b < byteCount; b++) {
196-
writer.putByte((byte) (value >>> (b * 8)));
221+
writer.putByte((byte) (masked >>> (b * 8)));
197222
}
198223
return this;
199224
}
@@ -226,14 +251,83 @@ public QwpBindValues setLong256(int index, long l0, long l1, long l2, long l3) {
226251
* Binds an explicit NULL with the given QWP wire type. The type code must
227252
* be one of the supported scalar bind types; ARRAY, BINARY, and IPv4 are
228253
* rejected because the server decoder does not accept them as binds.
254+
* <p>
255+
* DECIMAL64/128/256 NULLs are encoded with scale 0 and GEOHASH NULLs with
256+
* precision {@value #GEOHASH_MIN_BITS} bit; use {@link #setNullDecimal64},
257+
* {@link #setNullDecimal128}, {@link #setNullDecimal256}, or
258+
* {@link #setNullGeohash} when the scale/precision matters (it becomes part
259+
* of the bound variable's type on the server).
229260
*/
230261
public QwpBindValues setNull(int index, byte qwpTypeCode) {
231262
checkBindType(qwpTypeCode);
263+
if (qwpTypeCode == QwpConstants.TYPE_DECIMAL64) {
264+
return setNullDecimal64(index, 0);
265+
}
266+
if (qwpTypeCode == QwpConstants.TYPE_DECIMAL128) {
267+
return setNullDecimal128(index, 0);
268+
}
269+
if (qwpTypeCode == QwpConstants.TYPE_DECIMAL256) {
270+
return setNullDecimal256(index, 0);
271+
}
272+
if (qwpTypeCode == QwpConstants.TYPE_GEOHASH) {
273+
return setNullGeohash(index, GEOHASH_MIN_BITS);
274+
}
232275
advance(index);
233276
writeHeader(qwpTypeCode, true);
234277
return this;
235278
}
236279

280+
/**
281+
* Binds an explicit NULL with DECIMAL128 type and the given scale. The
282+
* server reads the scale byte regardless of null, so the scale must be
283+
* supplied even for NULL (it becomes part of the bound variable's type).
284+
*/
285+
public QwpBindValues setNullDecimal128(int index, int scale) {
286+
checkScale128(scale);
287+
advance(index);
288+
writeHeader(QwpConstants.TYPE_DECIMAL128, true);
289+
writer.putByte((byte) scale);
290+
return this;
291+
}
292+
293+
/**
294+
* Binds an explicit NULL with DECIMAL256 type and the given scale. See
295+
* {@link #setNullDecimal128} for the rationale.
296+
*/
297+
public QwpBindValues setNullDecimal256(int index, int scale) {
298+
checkScale256(scale);
299+
advance(index);
300+
writeHeader(QwpConstants.TYPE_DECIMAL256, true);
301+
writer.putByte((byte) scale);
302+
return this;
303+
}
304+
305+
/**
306+
* Binds an explicit NULL with DECIMAL64 type and the given scale. See
307+
* {@link #setNullDecimal128} for the rationale.
308+
*/
309+
public QwpBindValues setNullDecimal64(int index, int scale) {
310+
checkScale64(scale);
311+
advance(index);
312+
writeHeader(QwpConstants.TYPE_DECIMAL64, true);
313+
writer.putByte((byte) scale);
314+
return this;
315+
}
316+
317+
/**
318+
* Binds an explicit NULL with GEOHASH type and the given precision (bits).
319+
* The server reads the precision_bits varint regardless of null, so
320+
* precision must be supplied even for NULL (it becomes part of the bound
321+
* variable's type).
322+
*/
323+
public QwpBindValues setNullGeohash(int index, int precisionBits) {
324+
checkGeohashPrecision(precisionBits);
325+
advance(index);
326+
writeHeader(QwpConstants.TYPE_GEOHASH, true);
327+
writer.putVarint(precisionBits);
328+
return this;
329+
}
330+
237331
public QwpBindValues setShort(int index, short value) {
238332
advance(index);
239333
writeHeader(QwpConstants.TYPE_SHORT, false);
@@ -351,6 +445,14 @@ public void reset() {
351445
expectedNextIndex = 0;
352446
}
353447

448+
private static void checkGeohashPrecision(int precisionBits) {
449+
if (precisionBits < GEOHASH_MIN_BITS || precisionBits > GEOHASH_MAX_BITS) {
450+
throw new IllegalArgumentException(
451+
"GEOHASH precision must be in [" + GEOHASH_MIN_BITS + ", " + GEOHASH_MAX_BITS
452+
+ "], got " + precisionBits);
453+
}
454+
}
455+
354456
private static boolean isAscii(CharSequence value, int charLen) {
355457
for (int i = 0; i < charLen; i++) {
356458
if (value.charAt(i) >= 0x80) {
@@ -360,6 +462,10 @@ private static boolean isAscii(CharSequence value, int charLen) {
360462
return true;
361463
}
362464

465+
private static long maskGeohashBits(long value, int precisionBits) {
466+
return precisionBits >= 64 ? value : value & ((1L << precisionBits) - 1L);
467+
}
468+
363469
private void advance(int index) {
364470
if (index != expectedNextIndex) {
365471
throw new IllegalStateException(
@@ -400,10 +506,24 @@ private void checkBindType(byte type) {
400506
}
401507
}
402508

403-
private void checkScale(int scale) {
404-
if (scale < 0 || scale > Decimals.MAX_SCALE) {
509+
private void checkScale128(int scale) {
510+
if (scale < 0 || scale > DECIMAL128_MAX_SCALE) {
511+
throw new IllegalArgumentException(
512+
"DECIMAL128 scale must be in [0, " + DECIMAL128_MAX_SCALE + "], got " + scale);
513+
}
514+
}
515+
516+
private void checkScale256(int scale) {
517+
if (scale < 0 || scale > DECIMAL256_MAX_SCALE) {
518+
throw new IllegalArgumentException(
519+
"DECIMAL256 scale must be in [0, " + DECIMAL256_MAX_SCALE + "], got " + scale);
520+
}
521+
}
522+
523+
private void checkScale64(int scale) {
524+
if (scale < 0 || scale > DECIMAL64_MAX_SCALE) {
405525
throw new IllegalArgumentException(
406-
"scale must be in [0, " + Decimals.MAX_SCALE + "], got " + scale);
526+
"DECIMAL64 scale must be in [0, " + DECIMAL64_MAX_SCALE + "], got " + scale);
407527
}
408528
}
409529

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpEgressIoThread.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,19 @@ public void releaseBuffer(QwpBatchBuffer buffer) {
247247
// surfaces as a broken buffer rather than a slow native-memory leak.
248248
if (!freeBuffers.offer(buffer)) {
249249
buffer.close();
250+
return;
250251
}
251252
pendingRelease.offer(RELEASE_TOKEN);
253+
// Close race: closePool may have set closed AND drained freeBuffers
254+
// between our closed-check above and our offer. In that window the
255+
// buffer landed in a cleared-and-abandoned queue with no consumer.
256+
// Re-check after offer: if closed is now true and the buffer is
257+
// still in freeBuffers, remove it and close in place. remove() is
258+
// atomic on ArrayBlockingQueue, so if closePool's drain beat us to
259+
// it, remove returns false and we skip the double-close.
260+
if (closed && freeBuffers.remove(buffer)) {
261+
buffer.close();
262+
}
252263
}
253264

254265
/**

0 commit comments

Comments
 (0)