Skip to content

Commit 0b4ab3f

Browse files
glasstigerclaude
andcommitted
Harden symbol-dict resource teardown paths
Three defensive fixes on currently-unreachable teardown paths, each guarding a future edit from a native-memory hazard: - PersistedSymbolDict.close() now nulls loadedEntriesAddr/Len after freeing them (like scratchAddr), so a post-close read of the non-closed-guarded getters cannot dereference freed memory. - CursorWebSocketSendLoop resets sentDictCount alongside the buffer in both mirror-free sites, keeping the mirror all-or-nothing. start() has no closed guard, so a hypothetical close()-then-start() would otherwise observe a non-zero count against a freed buffer and drive setWireBaselineWithCatchUp into a null-mirror catch-up. - PersistedSymbolDict.openFresh() now closes the fd on any throw (the header malloc moves inside the try, plus a catch), mirroring openExisting; previously a throw in the header-write body freed the scratch buffer but leaked the fd. Tests: testCloseNullsLoadedEntries asserts the getters read 0 after close; the mirror-leak test now also asserts sentDictCount is reset. Both fail before their fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 8cfd7bd commit 0b4ab3f

4 files changed

Lines changed: 58 additions & 2 deletions

File tree

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,6 +907,11 @@ public synchronized void close() {
907907
sentDictBytesAddr = 0;
908908
sentDictBytesCapacity = 0;
909909
sentDictBytesLen = 0;
910+
// Reset the count alongside the buffer so the mirror stays all-or-
911+
// nothing: a hypothetical close()-then-start() (start() has no closed
912+
// guard) must not observe a non-zero sentDictCount against a freed
913+
// buffer and drive setWireBaselineWithCatchUp into a null-mirror catch-up.
914+
sentDictCount = 0;
910915
}
911916
}
912917

@@ -1793,6 +1798,7 @@ private void ioLoop() {
17931798
sentDictBytesAddr = 0;
17941799
sentDictBytesCapacity = 0;
17951800
sentDictBytesLen = 0;
1801+
sentDictCount = 0; // keep the mirror all-or-nothing (see close())
17961802
}
17971803
shutdownLatch.countDown();
17981804
// Failed-stop hand-off (see delegateEngineClose): the owner could

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,11 @@ public synchronized void close() {
282282
closed = true;
283283
if (loadedEntriesAddr != 0L) {
284284
Unsafe.free(loadedEntriesAddr, loadedEntriesLen, MemoryTag.NATIVE_DEFAULT);
285+
// Null after freeing (like scratchAddr below) so a future accessor that
286+
// reads loadedEntriesAddr()/loadedEntriesLen() post-close cannot
287+
// dereference freed native memory; the getters are not closed-guarded.
288+
loadedEntriesAddr = 0L;
289+
loadedEntriesLen = 0;
285290
}
286291
if (scratchAddr != 0L) {
287292
Unsafe.free(scratchAddr, scratchCap, MemoryTag.NATIVE_DEFAULT);
@@ -437,8 +442,9 @@ private static PersistedSymbolDict openFresh(String filePath) {
437442
LOG.warn("symbol dict {} could not be created (rc={}); proceeding without it", filePath, fd);
438443
return null;
439444
}
440-
long hdr = Unsafe.malloc(HEADER_SIZE, MemoryTag.NATIVE_DEFAULT);
445+
long hdr = 0L;
441446
try {
447+
hdr = Unsafe.malloc(HEADER_SIZE, MemoryTag.NATIVE_DEFAULT);
442448
Unsafe.getUnsafe().putInt(hdr, FILE_MAGIC);
443449
Unsafe.getUnsafe().putByte(hdr + 4, VERSION);
444450
Unsafe.getUnsafe().putByte(hdr + 5, (byte) 0);
@@ -451,8 +457,18 @@ private static PersistedSymbolDict openFresh(String filePath) {
451457
LOG.warn("symbol dict {} header write failed; proceeding without it", filePath);
452458
return null;
453459
}
460+
} catch (Throwable t) {
461+
// Unreachable today (Files.write is native and returns -1 rather than
462+
// throwing; the Unsafe puts target a valid 8-byte buffer and an 8-byte
463+
// malloc cannot realistically OOM), but close the fd against a future
464+
// edit so it cannot leak -- mirroring openExisting's error handling.
465+
Files.close(fd);
466+
LOG.warn("symbol dict {} creation failed ({}); proceeding without it", filePath, String.valueOf(t));
467+
return null;
454468
} finally {
455-
Unsafe.free(hdr, HEADER_SIZE, MemoryTag.NATIVE_DEFAULT);
469+
if (hdr != 0L) {
470+
Unsafe.free(hdr, HEADER_SIZE, MemoryTag.NATIVE_DEFAULT);
471+
}
456472
}
457473
return new PersistedSymbolDict(fd, HEADER_SIZE, 0, 0L, 0);
458474
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,15 @@ public void testSeededMirrorFreedWhenLoopClosedWithoutStart() throws Exception {
8989
0, 0, 1);
9090
// Close without start(): the ctor-seeded mirror is this
9191
// thread's to free, since the I/O loop never ran.
92+
Assert.assertTrue("precondition: the ctor seeded a non-empty mirror",
93+
readInt(loop, "sentDictCount") > 0);
9294
loop.close();
95+
// close() must reset sentDictCount alongside freeing the buffer,
96+
// so the mirror stays all-or-nothing: a hypothetical post-close
97+
// start() (no closed guard) cannot read a stale count against a
98+
// freed buffer and drive a null-mirror catch-up.
99+
Assert.assertEquals("close() must reset sentDictCount to 0",
100+
0, readInt(loop, "sentDictCount"));
93101
}
94102
});
95103
} finally {

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,32 @@ public void testBadMagicIsRecreatedEmpty() throws Exception {
228228
});
229229
}
230230

231+
@Test
232+
public void testCloseNullsLoadedEntries() throws Exception {
233+
// close() must null loadedEntriesAddr/Len after freeing them (like
234+
// scratchAddr), so an accidental post-close read of the getters cannot
235+
// dereference freed native memory. Pre-fix the pointer survived close()
236+
// non-zero.
237+
assertMemoryLeak(() -> {
238+
Path dir = Files.createTempDirectory("qwp-symdict");
239+
try {
240+
PersistedSymbolDict d = PersistedSymbolDict.open(dir.toString());
241+
d.appendSymbol("AAPL");
242+
d.close();
243+
244+
// Reopen so recovery loads the entries into native memory.
245+
PersistedSymbolDict re = PersistedSymbolDict.open(dir.toString());
246+
Assert.assertTrue("recovery must load entries into native memory",
247+
re.loadedEntriesAddr() != 0L && re.loadedEntriesLen() > 0);
248+
re.close();
249+
Assert.assertEquals("close() must null loadedEntriesAddr", 0L, re.loadedEntriesAddr());
250+
Assert.assertEquals("close() must null loadedEntriesLen", 0, re.loadedEntriesLen());
251+
} finally {
252+
rmDir(dir);
253+
}
254+
});
255+
}
256+
231257
@Test
232258
public void testEmptySymbolRoundTrips() throws Exception {
233259
assertMemoryLeak(() -> {

0 commit comments

Comments
 (0)