Skip to content

Commit 40c6a28

Browse files
glasstigerclaude
andcommitted
Harden persisted symbol-dict edge paths
Fold in low-severity hardening found in review; none changes behavior on any reachable input. PersistedSymbolDict: - appendRawEntries fails loudly (rather than flushing an uninitialised scratch tail and mis-advancing size) if a caller ever passes an (addr, len, count) triple whose entries do not fill len; the sole caller derives both from one beginMessage, so this cannot fire today. - openExisting sets entriesLen alongside the malloc, so the catch frees the right size if the copy walk throws -- the comment already claimed this held. - ensureScratch grows in long so scratchCap*2 cannot overflow negative past ~1 GB, matching ensureSentDictCapacity. - Two varint decode loops gain the shift > 35 bound the other decoders already carry. Close a file-descriptor leak in the rmDir test helpers: Files.walk returns a Stream backed by an open directory handle, so wrap it in try-with-resources (PersistedSymbolDictTest calls rmDir 13 times). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c065ad4 commit 40c6a28

4 files changed

Lines changed: 70 additions & 30 deletions

File tree

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

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,12 @@ public synchronized void appendRawEntries(long addr, int len, int count) {
230230
break;
231231
}
232232
shift += 7;
233+
if (shift > 35) {
234+
// A canonical entry-length varint is <= 5 bytes; a longer
235+
// continuation run is corrupt. The downstream entryEnd > srcLimit
236+
// check then rejects it. Matches decodeVarint / readVarintAt.
237+
break;
238+
}
233239
}
234240
long entryEnd = src + symLen; // src is just past the len varint
235241
if (entryEnd > srcLimit) {
@@ -242,6 +248,16 @@ public synchronized void appendRawEntries(long addr, int len, int count) {
242248
dst += wireSpan + CRC_SIZE;
243249
src = entryEnd;
244250
}
251+
if (src != srcLimit) {
252+
// The count entries did not consume exactly len bytes -- a caller passed an
253+
// inconsistent (addr, len, count) triple. Writing outLen would flush an
254+
// uninitialised scratch tail and mis-advance size, so fail loudly. The sole
255+
// caller derives count and len from one beginMessage, so this cannot fire
256+
// today.
257+
throw new IllegalStateException("raw symbol-dict entries under-filled the buffer to "
258+
+ FILE_NAME + " [count=" + count + ", len=" + len
259+
+ ", consumed=" + (int) (src - addr) + ']');
260+
}
245261
long written = Files.write(fd, scratchAddr, outLen, appendOffset);
246262
if (written != outLen) {
247263
throw new IllegalStateException("short write to " + FILE_NAME
@@ -503,6 +519,9 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
503519
// open. A second no-alloc walk over the already-validated region.
504520
if (wireLen > 0) {
505521
entriesAddr = Unsafe.malloc(wireLen, MemoryTag.NATIVE_DEFAULT);
522+
// Record the length alongside the malloc so the catch below frees the
523+
// right size (not 0) if this copy walk ever throws.
524+
entriesLen = wireLen;
506525
long dst = entriesAddr;
507526
int p = HEADER_SIZE;
508527
for (int i = 0; i < count; i++) {
@@ -516,14 +535,16 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
516535
break;
517536
}
518537
shift += 7;
538+
if (shift > 35) {
539+
break; // corrupt run; these entries were CRC-validated above
540+
}
519541
}
520542
int wireSpan = (vp - p) + (int) symLen; // [len][utf8], no CRC
521543
Unsafe.getUnsafe().copyMemory(buf + p, dst, wireSpan);
522544
dst += wireSpan;
523545
p += wireSpan + CRC_SIZE; // skip the entry's CRC
524546
}
525547
}
526-
entriesLen = wireLen;
527548
Unsafe.free(buf, len, MemoryTag.NATIVE_DEFAULT);
528549
buf = 0L;
529550
// Drop any torn/stale trailing bytes so a LATER, shorter append cannot
@@ -601,8 +622,15 @@ private void ensureScratch(int required) {
601622
if (scratchCap >= required) {
602623
return;
603624
}
604-
int newCap = Math.max(required, Math.max(256, scratchCap * 2));
605-
scratchAddr = Unsafe.realloc(scratchAddr, scratchCap, newCap, MemoryTag.NATIVE_DEFAULT);
606-
scratchCap = newCap;
625+
// Double in long: scratchCap * 2 as an int overflows negative past ~1 GB and
626+
// would make the realloc size negative. required is bounded by one frame's
627+
// entries (the server batch cap), so this never actually caps -- it mirrors the
628+
// long-math growth in CursorWebSocketSendLoop.ensureSentDictCapacity.
629+
long newCap = Math.max(required, Math.max(256L, (long) scratchCap * 2));
630+
if (newCap > Integer.MAX_VALUE - 8) {
631+
newCap = Integer.MAX_VALUE - 8;
632+
}
633+
scratchAddr = Unsafe.realloc(scratchAddr, scratchCap, (int) newCap, MemoryTag.NATIVE_DEFAULT);
634+
scratchCap = (int) newCap;
607635
}
608636
}

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/SelfSufficientFramesTest.java

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.util.Comparator;
3939
import java.util.concurrent.TimeUnit;
4040
import java.util.concurrent.atomic.AtomicLong;
41+
import java.util.stream.Stream;
4142

4243
import static io.questdb.client.test.tools.TestUtils.assertMemoryLeak;
4344

@@ -351,15 +352,18 @@ private static void rmDir(Path dir) {
351352
if (dir == null || !Files.exists(dir)) {
352353
return;
353354
}
354-
Files.walk(dir)
355-
.sorted(Comparator.reverseOrder())
356-
.forEach(p -> {
357-
try {
358-
Files.deleteIfExists(p);
359-
} catch (IOException ignored) {
360-
// best-effort
361-
}
362-
});
355+
// try-with-resources: Files.walk returns a Stream backed by an open
356+
// directory handle that must be closed, or each rmDir leaks a descriptor.
357+
try (Stream<Path> walk = Files.walk(dir)) {
358+
walk.sorted(Comparator.reverseOrder())
359+
.forEach(p -> {
360+
try {
361+
Files.deleteIfExists(p);
362+
} catch (IOException ignored) {
363+
// best-effort
364+
}
365+
});
366+
}
363367
} catch (IOException ignored) {
364368
// best-effort
365369
}

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

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.nio.file.Path;
3939
import java.util.Comparator;
4040
import java.util.concurrent.TimeUnit;
41+
import java.util.stream.Stream;
4142

4243
import static io.questdb.client.test.tools.TestUtils.assertMemoryLeak;
4344

@@ -199,15 +200,18 @@ private static void rmDir(Path dir) throws IOException {
199200
if (dir == null || !Files.exists(dir)) {
200201
return;
201202
}
202-
Files.walk(dir)
203-
.sorted(Comparator.reverseOrder())
204-
.forEach(p -> {
205-
try {
206-
Files.deleteIfExists(p);
207-
} catch (IOException ignored) {
208-
// best-effort
209-
}
210-
});
203+
// try-with-resources: Files.walk returns a Stream backed by an open
204+
// directory handle that must be closed, or each rmDir leaks a descriptor.
205+
try (Stream<Path> walk = Files.walk(dir)) {
206+
walk.sorted(Comparator.reverseOrder())
207+
.forEach(p -> {
208+
try {
209+
Files.deleteIfExists(p);
210+
} catch (IOException ignored) {
211+
// best-effort
212+
}
213+
});
214+
}
211215
}
212216

213217
private static class SilentHandler implements TestWebSocketServer.WebSocketServerHandler {

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

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import java.nio.file.Path;
3838
import java.nio.file.StandardOpenOption;
3939
import java.util.Comparator;
40+
import java.util.stream.Stream;
4041

4142
import static io.questdb.client.test.tools.TestUtils.assertMemoryLeak;
4243

@@ -503,14 +504,17 @@ private static void rmDir(Path dir) {
503504
if (dir == null || !Files.exists(dir)) {
504505
return;
505506
}
506-
Files.walk(dir)
507-
.sorted(Comparator.reverseOrder())
508-
.forEach(p -> {
509-
try {
510-
Files.deleteIfExists(p);
511-
} catch (IOException ignored) {
512-
}
513-
});
507+
// try-with-resources: Files.walk returns a Stream backed by an open
508+
// directory handle that must be closed, or each rmDir leaks a descriptor.
509+
try (Stream<Path> walk = Files.walk(dir)) {
510+
walk.sorted(Comparator.reverseOrder())
511+
.forEach(p -> {
512+
try {
513+
Files.deleteIfExists(p);
514+
} catch (IOException ignored) {
515+
}
516+
});
517+
}
514518
} catch (IOException ignored) {
515519
}
516520
}

0 commit comments

Comments
 (0)