Skip to content

Commit 10a125c

Browse files
glasstigerclaude
andcommitted
Guard catch-up frame overflow, add biting tests
Two defensive spots in the delta symbol-dict path had no biting test, and one guard was incomplete (M3). Complete the catch-up frame-size guard (M3b). The int-overflow hardening covered ensureSentDictCapacity (the mirror growth) but not the single catch-up frame: with no server cap, frameLen = HEADER + varints + symbolsLen is an int and would wrap negative as symbolsLen approaches the mirror ceiling, feeding a bad Unsafe.malloc. sendDictCatchUp's budget already keeps each chunk under that bound, so this is unreachable at real cardinality -- but the guard must be local so a future caller cannot overflow it silently. Compute the size in long and fail loud (CatchUpSendException) before the malloc, matching the mirror-side guard. Cover accumulateSentDict's partial-overlap tail (M3a). A delta that straddles the mirror tip (deltaStart < sentDictCount < deltaEnd) must copy only the new tail, not drop the whole frame. The monotonic producer never emits a straddling delta in steady state, so reverting to the pre-fix drop-whole-frame guard passed every test; it is reachable on a torn-dict replay (mirror seeded smaller than a frame's coverage), where dropping the tail would leave the reconnect catch-up incomplete and shift server ids. Add a white-box test that drives the straddle directly. Both new tests fail with their production line reverted (the mirror stays at 1 id; the frame guard falls through to a negative malloc that throws IllegalArgumentException, not the clean terminal) and pass with it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 8853d8f commit 10a125c

2 files changed

Lines changed: 148 additions & 2 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2234,10 +2234,24 @@ private int sendDictCatchUp() {
22342234
* the caller turns it into a single, non-re-entrant reconnect.
22352235
*/
22362236
private void sendCatchUpChunk(int deltaStart, int deltaCount, long symbolsAddr, int symbolsLen) {
2237-
int payloadLen = NativeBufferWriter.varintSize(deltaStart)
2237+
// Compute the frame size in long and fail loud if it would overflow the int
2238+
// size math into a negative Unsafe.malloc. sendDictCatchUp already caps each
2239+
// chunk's symbol bytes under the budget, so this is unreachable at real
2240+
// cardinality -- but the mirror-side ensureSentDictCapacity guards the same
2241+
// math, and a future caller must not be able to overflow this one silently.
2242+
long payloadLenL = (long) NativeBufferWriter.varintSize(deltaStart)
22382243
+ NativeBufferWriter.varintSize(deltaCount)
22392244
+ symbolsLen;
2240-
int frameLen = QwpConstants.HEADER_SIZE + payloadLen;
2245+
long frameLenL = QwpConstants.HEADER_SIZE + payloadLenL;
2246+
if (frameLenL > MAX_SENT_DICT_BYTES) {
2247+
LineSenderException err = new LineSenderException(
2248+
"symbol dictionary catch-up frame exceeds the maximum size ["
2249+
+ "frameLen=" + frameLenL + ", max=" + MAX_SENT_DICT_BYTES + ']');
2250+
recordFatal(err);
2251+
throw new CatchUpSendException(err);
2252+
}
2253+
int payloadLen = (int) payloadLenL;
2254+
int frameLen = (int) frameLenL;
22412255
long frame = Unsafe.malloc(frameLen, MemoryTag.NATIVE_DEFAULT);
22422256
try {
22432257
Unsafe.getUnsafe().putByte(frame, (byte) 'Q');

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopCatchUpAlignmentTest.java

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,12 @@
4343
import java.lang.reflect.Method;
4444
import java.nio.charset.StandardCharsets;
4545
import java.nio.file.Paths;
46+
import java.util.ArrayList;
47+
import java.util.Arrays;
48+
import java.util.List;
4649

4750
import static org.junit.Assert.assertEquals;
51+
import static org.junit.Assert.assertTrue;
4852
import static org.junit.Assert.fail;
4953

5054
/**
@@ -211,6 +215,76 @@ public void testTransientCatchUpSendFailureIsRetriableNotTerminal() throws Excep
211215
});
212216
}
213217

218+
@Test
219+
public void testAccumulateSentDictPartialOverlapExtendsMirror() throws Exception {
220+
// M3: accumulateSentDict must handle a delta that STRADDLES the mirror tip
221+
// (deltaStart < sentDictCount < deltaStart+deltaCount) by copying only the
222+
// new tail, not dropping the whole frame. The monotonic producer never emits
223+
// a straddling delta in steady state (so the pre-fix drop-whole-frame guard
224+
// passed every test), but a torn-dict replay can seed the mirror smaller than
225+
// a frame's coverage. Seed the mirror with 1 symbol, feed a [0..2] delta, and
226+
// assert the mirror extends to all 3 -- pre-fix it stayed at 1, leaving the
227+
// reconnect catch-up incomplete and shifting server-side ids.
228+
TestUtils.assertMemoryLeak(() -> {
229+
CatchUpCapturingClient client = new CatchUpCapturingClient(0);
230+
try (CursorSendEngine engine = newEngine()) {
231+
CursorWebSocketSendLoop loop = newLoop(engine, client);
232+
try {
233+
seedMirror(loop, "aa"); // sentDictCount = 1, mirror holds "aa"
234+
int[] frameLen = new int[1];
235+
long frame = buildDeltaFrame(0, new String[]{"aa", "bb", "cc"}, frameLen);
236+
try {
237+
Method m = CursorWebSocketSendLoop.class.getDeclaredMethod(
238+
"accumulateSentDict", long.class, int.class, int.class);
239+
m.setAccessible(true);
240+
m.invoke(loop, frame, frameLen[0], 0);
241+
} finally {
242+
Unsafe.free(frame, frameLen[0], MemoryTag.NATIVE_DEFAULT);
243+
}
244+
assertEquals("straddling delta must extend the mirror to all 3 ids",
245+
3, readInt(loop, "sentDictCount"));
246+
assertEquals("mirror must hold the two new tail symbols after the "
247+
+ "already-held prefix, gap-free",
248+
Arrays.asList("aa", "bb", "cc"), readMirrorSymbols(loop));
249+
} finally {
250+
loop.close();
251+
}
252+
}
253+
});
254+
}
255+
256+
@Test
257+
public void testCatchUpChunkFrameSizeOverflowFailsLoud() throws Exception {
258+
// M3: sendDictCatchUp caps each chunk under the budget, so the single-frame
259+
// catch-up path cannot overflow its int frameLen at any real cardinality. The
260+
// guard must still be LOCAL -- a future caller must not be able to feed a
261+
// wrapped-negative frameLen to Unsafe.malloc. An oversized symbolsLen must
262+
// fail loud (CatchUpSendException) BEFORE the malloc; the guard fires before
263+
// symbolsAddr is read, so a dummy address is fine.
264+
TestUtils.assertMemoryLeak(() -> {
265+
CatchUpCapturingClient client = new CatchUpCapturingClient(0);
266+
try (CursorSendEngine engine = newEngine()) {
267+
CursorWebSocketSendLoop loop = newLoop(engine, client);
268+
try {
269+
Method m = CursorWebSocketSendLoop.class.getDeclaredMethod(
270+
"sendCatchUpChunk", int.class, int.class, long.class, int.class);
271+
m.setAccessible(true);
272+
// symbolsLen past the mirror ceiling: HEADER + varints + symbolsLen
273+
// overflows an int, so the guard must reject it before malloc.
274+
m.invoke(loop, 0, 1, 0L, Integer.MAX_VALUE - 4);
275+
fail("an overflowing catch-up frame size must fail loud, not malloc negative");
276+
} catch (InvocationTargetException e) {
277+
assertEquals("overflow must surface as CatchUpSendException",
278+
"CatchUpSendException", e.getCause().getClass().getSimpleName());
279+
assertTrue("message must name the frame-size guard: " + e.getCause().getMessage(),
280+
e.getCause().getMessage().contains("catch-up frame exceeds the maximum size"));
281+
} finally {
282+
loop.close();
283+
}
284+
}
285+
});
286+
}
287+
214288
private static void appendFrames(CursorSendEngine engine, int count) {
215289
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
216290
try {
@@ -226,6 +300,64 @@ private static void appendFrames(CursorSendEngine engine, int count) {
226300
}
227301
}
228302

303+
// Builds a QWP delta frame [12-byte header][deltaStart varint][deltaCount
304+
// varint][ [len varint][utf8] ... ] for the given symbols. accumulateSentDict
305+
// skips the header, so its content is irrelevant; the caller frees the frame.
306+
private static long buildDeltaFrame(int deltaStart, String[] symbols, int[] outLen) {
307+
int deltaCount = symbols.length;
308+
int size = 12 + varintSize(deltaStart) + varintSize(deltaCount);
309+
for (String s : symbols) {
310+
size += varintSize(s.getBytes(StandardCharsets.UTF_8).length)
311+
+ s.getBytes(StandardCharsets.UTF_8).length;
312+
}
313+
long addr = Unsafe.malloc(size, MemoryTag.NATIVE_DEFAULT);
314+
long p = writeVarint(addr + 12, deltaStart);
315+
p = writeVarint(p, deltaCount);
316+
for (String s : symbols) {
317+
byte[] b = s.getBytes(StandardCharsets.UTF_8);
318+
p = writeVarint(p, b.length);
319+
for (byte x : b) {
320+
Unsafe.getUnsafe().putByte(p++, x);
321+
}
322+
}
323+
outLen[0] = size;
324+
return addr;
325+
}
326+
327+
private static int readInt(CursorWebSocketSendLoop loop, String name) throws Exception {
328+
Field f = CursorWebSocketSendLoop.class.getDeclaredField(name);
329+
f.setAccessible(true);
330+
return f.getInt(loop);
331+
}
332+
333+
// Parses the loop's native sent-dictionary mirror ([len varint][utf8]...) back
334+
// into the symbol strings a reconnect catch-up would re-register.
335+
private static List<String> readMirrorSymbols(CursorWebSocketSendLoop loop) throws Exception {
336+
long addr = readLong(loop, "sentDictBytesAddr");
337+
int len = readInt(loop, "sentDictBytesLen");
338+
List<String> out = new ArrayList<>();
339+
long p = addr;
340+
long limit = addr + len;
341+
while (p < limit) {
342+
long l = 0;
343+
int shift = 0;
344+
while (p < limit) {
345+
byte b = Unsafe.getUnsafe().getByte(p++);
346+
l |= (long) (b & 0x7F) << shift;
347+
if ((b & 0x80) == 0) {
348+
break;
349+
}
350+
shift += 7;
351+
}
352+
byte[] bytes = new byte[(int) l];
353+
for (int i = 0; i < l; i++) {
354+
bytes[i] = Unsafe.getUnsafe().getByte(p++);
355+
}
356+
out.add(new String(bytes, StandardCharsets.UTF_8));
357+
}
358+
return out;
359+
}
360+
229361
// Delivers a 0-table STATUS_OK for {@code wireSeq} into the loop's response
230362
// handler, mimicking the server acking a catch-up frame (which carries no tables).
231363
private static void deliverOk(CursorWebSocketSendLoop loop, long wireSeq) throws Exception {

0 commit comments

Comments
 (0)