Skip to content

Commit acb32b9

Browse files
bluestreak01claude
andcommitted
fix(ilp): plug SF rotate-OOM double-free and on-disk leak
SegmentLog.rotate freed old.pathPtrNative and assigned the new sealed- path pointer in non-atomic order: free → assign-path → alloc → set sealed. If allocNativePath OOMed mid-sequence the segment was left in two simultaneously broken states: - pathPtrNative still held the freed pointer (the assignment never ran). On close() the segments-cleanup loop called freeNativePath on it again — a native-heap double-free that crashed the JVM with malloc free-list corruption (verified via the new red test on the unfixed code: surefire reported "The forked VM terminated without properly saying goodbye"). - sealed/lastSeqOnDisk were never set, so trim()'s !s.sealed guard silently skipped the segment. The .sfs file on disk was never reclaimed within the lifetime of the process. Fix: - Set old.pathPtrNative=0 immediately after the free so a subsequent OOM cannot leave a stale freed pointer in the field. - Mark sealed=true / lastSeqOnDisk=lastSeq BEFORE allocating the new pointer. After OOM the segment is still classified as sealed so trim can reclaim it. - trim() now handles the recovery case where pathPtrNative is 0 by falling back to ff.remove(path) (one-time per trim, acceptable — these recovery branches only fire after an OOM, not on the hot path). Test: testRotateOomLeavesSegmentInRecoverableSealedState (SegmentLogTest). Forces rotation under an injected allocNativePath OOM, then asserts (a) close() does not double-free, (b) trim() reaps the orphan .sfs file. On the unfixed code the JVM dies; on the fix 118 SF + adjacent tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 86b6e6f commit acb32b9

2 files changed

Lines changed: 136 additions & 6 deletions

File tree

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

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -274,9 +274,16 @@ public void trim(long ackedSeq) {
274274
ff.close(s.fd);
275275
s.fd = -1;
276276
}
277-
ff.remove(s.pathPtrNative);
278-
ff.freeNativePath(s.pathPtrNative);
279-
s.pathPtrNative = 0;
277+
if (s.pathPtrNative != 0) {
278+
ff.remove(s.pathPtrNative);
279+
ff.freeNativePath(s.pathPtrNative);
280+
s.pathPtrNative = 0;
281+
} else {
282+
// Recovery case: rotate's allocNativePath OOMed and left
283+
// pathPtrNative=0. Fall back to the String form, which
284+
// does its own one-shot encode/free internally.
285+
ff.remove(s.path);
286+
}
280287
bytesOnDiskCache -= s.writePos;
281288
} else {
282289
segments.setQuick(writeIdx++, s);
@@ -561,12 +568,21 @@ private void rotate() {
561568
if (ff.rename(old.path, sealedPath) != 0) {
562569
throw new SfException("failed to seal segment by rename " + old.path + " -> " + sealedPath);
563570
}
564-
// Path changed — free old native ptr and re-encode for the sealed name.
571+
// Filesystem is now in the sealed state. Update bookkeeping to match
572+
// BEFORE re-encoding the path pointer; if allocNativePath OOMs:
573+
// - the stale freed pointer must not be left in the field, or
574+
// close() walks segments and calls freeNativePath on it again
575+
// → native double-free.
576+
// - sealed/lastSeqOnDisk must already be set, or trim never sees
577+
// this segment (the !s.sealed guard skips it) → permanent
578+
// on-disk leak that survives until the next process restart.
579+
// trim() handles pathPtrNative==0 by falling back to ff.remove(path).
565580
ff.freeNativePath(old.pathPtrNative);
581+
old.pathPtrNative = 0;
566582
old.path = sealedPath;
567-
old.pathPtrNative = ff.allocNativePath(sealedPath);
568583
old.sealed = true;
569584
old.lastSeqOnDisk = lastSeq;
585+
old.pathPtrNative = ff.allocNativePath(sealedPath);
570586
createActive(lastSeq + 1);
571587
}
572588

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

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -637,6 +637,109 @@ public void testCreateActiveDoesNotLeakFdOnAllocNativePathOom() throws Exception
637637
});
638638
}
639639

640+
/**
641+
* Regression test for {@code rotate}'s mid-reseal OOM window.
642+
* <p>
643+
* Production order at lines 564-570 (pre-fix):
644+
* <pre>
645+
* ff.freeNativePath(old.pathPtrNative); // ptr freed
646+
* old.path = sealedPath;
647+
* old.pathPtrNative = ff.allocNativePath(sealedPath); // CAN throw OOM
648+
* old.sealed = true;
649+
* old.lastSeqOnDisk = lastSeq;
650+
* </pre>
651+
* If {@code allocNativePath} throws after the freed pointer is left in
652+
* the field and before {@code sealed/lastSeqOnDisk} are set:
653+
* <ul>
654+
* <li><b>native double-free on close:</b> {@code SegmentLog.close()}
655+
* walks {@code segments} and calls {@code freeNativePath} on the
656+
* stale freed pointer.</li>
657+
* <li><b>permanent on-disk leak:</b> {@code trim()}'s {@code !s.sealed}
658+
* guard skips the segment, so the {@code .sfs} file on disk is
659+
* never reclaimed within the lifetime of this process. Even after
660+
* restart it would re-replay forever (no ACK ever advances past
661+
* its lastSeq because the in-memory state lost it).</li>
662+
* </ul>
663+
* <p>
664+
* The fix sets {@code pathPtrNative=0} immediately after the free and
665+
* marks {@code sealed=true; lastSeqOnDisk=lastSeq} BEFORE allocating
666+
* the new pointer. {@code trim()} falls back to {@code ff.remove(path)}
667+
* when {@code pathPtrNative} is 0.
668+
*/
669+
@Test
670+
public void testRotateOomLeavesSegmentInRecoverableSealedState() throws Exception {
671+
TestUtils.assertMemoryLeak(() -> {
672+
FdTrackingFacade tracker = new FdTrackingFacade();
673+
// maxBytes = HEADER_SIZE + FRAME_HEADER_SIZE + 16 = 48; first
674+
// append fits the active segment exactly, the second forces
675+
// rotation.
676+
final long maxBytes = 48;
677+
final int payloadSize = 16;
678+
679+
long buf = Unsafe.malloc(payloadSize, MemoryTag.NATIVE_DEFAULT);
680+
try {
681+
for (int i = 0; i < payloadSize; i++) {
682+
Unsafe.getUnsafe().putByte(buf + i, (byte) i);
683+
}
684+
685+
SegmentLog log = SegmentLog.open(tmpDir, tracker, maxBytes, 1024, false);
686+
try {
687+
long s0 = log.append(buf, payloadSize);
688+
assertEquals(0L, s0);
689+
690+
// Arm the OOM at the rotate's allocNativePath(sealedPath).
691+
tracker.failNextSealedAllocNativePath = true;
692+
try {
693+
log.append(buf, payloadSize);
694+
fail("expected OOM during rotate's allocNativePath(sealedPath)");
695+
} catch (Throwable expected) {
696+
String msg = expected.getMessage() == null ? "" : expected.getMessage();
697+
String causeMsg = expected.getCause() == null
698+
|| expected.getCause().getMessage() == null
699+
? "" : expected.getCause().getMessage();
700+
assertTrue("wrong failure: " + expected,
701+
msg.contains("simulated") || msg.contains("OOM")
702+
|| causeMsg.contains("simulated") || causeMsg.contains("OOM"));
703+
}
704+
705+
// The segment is sealed on disk and must be classified
706+
// as sealed in memory so trim() can reclaim it. Drop
707+
// every acked seq up to and including the (now-sealed)
708+
// segment's lastSeq, then assert the file is gone.
709+
log.trim(0);
710+
} finally {
711+
// close() walks the segments list and frees pathPtrNative
712+
// for each. Under the bug the rotated segment's stale
713+
// freed pointer would be passed to freeNativePath again
714+
// → native double-free. The fix sets pathPtrNative=0
715+
// after the original free so close() skips it.
716+
log.close();
717+
}
718+
719+
// No .sfs file should remain after trim().
720+
long find = Files.findFirst(tmpDir);
721+
if (find != 0) {
722+
try {
723+
int rc = 1;
724+
while (rc > 0) {
725+
String name = Files.utf8ToString(Files.findName(find));
726+
if (name != null && name.endsWith(".sfs")) {
727+
fail("sealed .sfs file leaked after trim: " + name
728+
+ " — rotate's mid-OOM left the segment unsealed in "
729+
+ "memory so trim's !s.sealed guard skipped it");
730+
}
731+
rc = Files.findNext(find);
732+
}
733+
} finally {
734+
Files.findClose(find);
735+
}
736+
}
737+
} finally {
738+
Unsafe.free(buf, payloadSize, MemoryTag.NATIVE_DEFAULT);
739+
}
740+
});
741+
}
742+
640743
@Test
641744
public void testCreateActiveDoesNotLeakFdOnFsyncFailure() throws Exception {
642745
TestUtils.assertMemoryLeak(() -> {
@@ -1024,15 +1127,26 @@ private static class FdTrackingFacade implements FilesFacade {
10241127
// ACTIVE_SUFFIX. Simulates an OOM at the exact moment between
10251128
// openCleanRW and the try-block in createActive. Auto-reset.
10261129
volatile boolean failNextActiveAllocNativePath;
1130+
// Set true to fault the NEXT allocNativePath whose path ends in
1131+
// SEALED_SUFFIX. Simulates an OOM in the rotate-then-reseal path
1132+
// after the file rename succeeded but before the new pointer is
1133+
// installed. Auto-reset.
1134+
volatile boolean failNextSealedAllocNativePath;
10271135

10281136
@Override
10291137
public long allocNativePath(String path) {
1030-
// ".sfa" is SegmentLog.ACTIVE_SUFFIX (package-private, hardcoded here).
1138+
// ".sfa" / ".sfs" are SegmentLog.{ACTIVE,SEALED}_SUFFIX
1139+
// (package-private, hardcoded here).
10311140
if (failNextActiveAllocNativePath && path.endsWith(".sfa")) {
10321141
failNextActiveAllocNativePath = false;
10331142
throw CairoException.nonCritical()
10341143
.put("simulated OOM in allocNativePath: ").put(path);
10351144
}
1145+
if (failNextSealedAllocNativePath && path.endsWith(".sfs")) {
1146+
failNextSealedAllocNativePath = false;
1147+
throw CairoException.nonCritical()
1148+
.put("simulated OOM in allocNativePath: ").put(path);
1149+
}
10361150
return FilesFacade.INSTANCE.allocNativePath(path);
10371151
}
10381152

0 commit comments

Comments
 (0)