Skip to content

Commit a375a78

Browse files
committed
Fix SIGBUS risk when appending into recovered sparse SF segments
Recovery mmap'd the segment file's complete logical length RW and left appendCursor at lastGoodOffset, exposing the unvalidated tail as writable capacity. If that tail was sparse/unbacked (fallocate reservations do not survive external sparsification, and FilesFacade.allocate cannot retroactively fill holes below EOF), the next producer append stored straight into the hole — which under ENOSPC raises SIGBUS and aborts the JVM. MmapSegment.openExisting now maps only the validated prefix [0, lastGoodOffset), making recovered segments born full: the first append backpressures until the segment manager installs a hot spare (created with genuine block reservation and clean ENOSPC failure), then rotates onto it. Consumers are unaffected — they only read below publishedOffset(), which equals lastGoodOffset. Because a recovered segment's mapped size no longer reflects its on-disk footprint, sf_max_total_bytes accounting (register seed and trim decrement) now uses the new MmapSegment.fileBytes() — the file's logical length — instead of sizeBytes(). Regression tests: recovered sparse-tail segment must be born full and refuse tryAppend; a ring recovered around a sparse-tail active must backpressure the first append and resume with a contiguous FSN after spare rotation.
1 parent 9f3cb32 commit a375a78

5 files changed

Lines changed: 172 additions & 29 deletions

File tree

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

Lines changed: 55 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,11 @@ public final class MmapSegment implements QuietCloseable {
7070
private final FilesFacade filesFacade;
7171
private final String path;
7272
private final long sizeBytes;
73+
// fileBytes: logical size of the backing file — the segment's on-disk
74+
// footprint for sf_max_total_bytes accounting. Equals sizeBytes for
75+
// created and memory-backed segments; for recovered segments it can
76+
// exceed sizeBytes because recovery maps only the validated prefix.
77+
private final long fileBytes;
7378
// memoryBacked: true when the segment buffer lives in malloc'd native
7479
// memory rather than an mmap'd file. The "non-SF async" path uses
7580
// memory-backed segments — same cursor architecture, no disk involvement.
@@ -104,13 +109,14 @@ public final class MmapSegment implements QuietCloseable {
104109
private final long tornTailBytes;
105110

106111
private MmapSegment(String path, int fd, long mmapAddress, long sizeBytes,
107-
long baseSeq, long initialCursor, long frameCount,
112+
long fileBytes, long baseSeq, long initialCursor, long frameCount,
108113
boolean memoryBacked, long tornTailBytes, FilesFacade filesFacade) {
109114
this.path = path;
110115
this.filesFacade = filesFacade;
111116
this.fd = fd;
112117
this.mmapAddress = mmapAddress;
113118
this.sizeBytes = sizeBytes;
119+
this.fileBytes = fileBytes;
114120
this.baseSeq = baseSeq;
115121
this.appendCursor = initialCursor;
116122
this.publishedCursor = initialCursor;
@@ -200,7 +206,7 @@ public static MmapSegment create(FilesFacade ff, long pathPtr, String displayPat
200206
Unsafe.getUnsafe().putLong(addr + 8, baseSeq);
201207
Unsafe.getUnsafe().putLong(addr + 16, Os.currentTimeMicros());
202208
return new MmapSegment(
203-
displayPath, fd, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, false, 0L, ff);
209+
displayPath, fd, addr, sizeBytes, sizeBytes, baseSeq, HEADER_SIZE, 0, false, 0L, ff);
204210
} catch (Throwable t) {
205211
if (addr != Files.FAILED_MMAP_ADDRESS) {
206212
Files.munmap(addr, sizeBytes, MemoryTag.MMAP_DEFAULT);
@@ -238,21 +244,30 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) {
238244
Unsafe.getUnsafe().putLong(addr + 8, baseSeq);
239245
Unsafe.getUnsafe().putLong(addr + 16, Os.currentTimeMicros());
240246
return new MmapSegment(
241-
null, -1, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, true, 0L, null);
247+
null, -1, addr, sizeBytes, sizeBytes, baseSeq, HEADER_SIZE, 0, true, 0L, null);
242248
} catch (Throwable t) {
243249
Unsafe.free(addr, sizeBytes, MemoryTag.NATIVE_DEFAULT);
244250
throw t;
245251
}
246252
}
247253

248254
/**
249-
* Opens an existing segment file for recovery. mmaps it RW, validates the
250-
* header magic / version, then scans frames forward verifying each CRC.
251-
* The first bad CRC (or a frame whose declared length runs past the file
252-
* end) is treated as a torn tail; both cursors are positioned at the
253-
* start of that frame. Returns the segment ready for further appends.
255+
* Opens an existing segment file for recovery. Validates the header magic
256+
* / version, then scans frames forward verifying each CRC. The first bad
257+
* CRC (or a frame whose declared length runs past the file end) is treated
258+
* as a torn tail; both cursors are positioned at the start of that frame.
254259
* Throws {@link MmapSegmentException} on header validation failure.
255260
* <p>
261+
* The returned segment maps ONLY the validated prefix
262+
* {@code [0, lastGoodOffset)} and is therefore born full:
263+
* {@link #tryAppend} returns -1 and the caller rotates onto a freshly
264+
* created segment before the next append. The tail past the last good
265+
* frame is never mapped — it may be sparse/unbacked ({@code create()}'s
266+
* block reservation does not survive external sparsification, and
267+
* {@link FilesFacade#allocate} cannot retroactively fill holes below EOF),
268+
* and a store into an unbacked page under ENOSPC raises SIGBUS, aborting
269+
* the JVM.
270+
* <p>
256271
* If recovery observes a torn tail (the bytes at the bail-out position
257272
* are non-zero, indicating an attempted-but-failed frame write rather
258273
* than clean unwritten space), a {@code WARN} is emitted with the byte
@@ -303,11 +318,23 @@ public static MmapSegment openExisting(FilesFacade ff, String path) {
303318
"short read before stable file EOF during recovery: " + path
304319
+ " size=" + fileSize);
305320
}
306-
mapSize = Math.min(fileSize, finalFileSize);
307-
if (mapSize < HEADER_SIZE) {
321+
long logicalSize = Math.min(fileSize, finalFileSize);
322+
if (logicalSize < HEADER_SIZE) {
308323
throw new MmapSegmentException("file shorter than header after recovery scan: "
309-
+ path + " size=" + mapSize);
324+
+ path + " size=" + logicalSize);
310325
}
326+
// Map ONLY the validated prefix [0, lastGoodOffset). The tail past
327+
// the last good frame may be sparse/unbacked, and a store into an
328+
// unbacked page under ENOSPC raises SIGBUS and aborts the JVM.
329+
// FilesFacade.allocate cannot make that tail safe at recovery time
330+
// (it reserves [currentSize, target) only — pre-existing holes
331+
// below EOF are not retroactively filled). Mapping through
332+
// lastGoodOffset makes the recovered segment born full
333+
// (capacityRemaining() == 0), so the producer rotates onto a
334+
// freshly created — and genuinely block-reserved — segment before
335+
// the next append. Consumers are unaffected: they only read below
336+
// publishedOffset(), which equals lastGoodOffset.
337+
mapSize = recovered.lastGoodOffset;
311338
addr = Files.mmap(fd, mapSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT);
312339
if (addr == Files.FAILED_MMAP_ADDRESS) {
313340
throw new MmapSegmentException("mmap failed for " + path);
@@ -317,23 +344,24 @@ public static MmapSegment openExisting(FilesFacade ff, String path) {
317344
fd,
318345
addr,
319346
mapSize,
347+
logicalSize,
320348
recovered.baseSeq,
321349
recovered.lastGoodOffset,
322350
recovered.frameCount,
323351
false,
324-
Math.min(recovered.tornTailBytes, mapSize - recovered.lastGoodOffset),
352+
Math.min(recovered.tornTailBytes, logicalSize - recovered.lastGoodOffset),
325353
ff
326354
);
327355
if (segment.tornTailBytes() > 0) {
328356
LOG.warn("SF segment {}: torn tail of {} bytes at offset {} "
329357
+ "(file size {}, frames recovered {}). "
330-
+ "Recovery will overwrite this region on next append; "
358+
+ "The torn region is excluded from the recovered mapping and "
331359
+ "frames past the tear (if any) are discarded. "
332360
+ "Investigate disk health or unexpected writer crash.",
333361
path,
334362
segment.tornTailBytes(),
335363
segment.publishedOffset(),
336-
mapSize,
364+
logicalSize,
337365
segment.frameCount());
338366
}
339367
// Ownership of fd and addr transfers to the returned segment.
@@ -443,6 +471,19 @@ public long sizeBytes() {
443471
return sizeBytes;
444472
}
445473

474+
/**
475+
* Logical size of the backing file in bytes — the segment's on-disk
476+
* footprint for {@code sf_max_total_bytes} cap accounting. Equals
477+
* {@link #sizeBytes()} for created and memory-backed segments; for
478+
* recovered segments it can exceed {@link #sizeBytes()} because recovery
479+
* maps only the validated prefix (SIGBUS hardening in
480+
* {@link #openExisting}) while the file keeps its full logical length
481+
* until trim unlinks it.
482+
*/
483+
public long fileBytes() {
484+
return fileBytes;
485+
}
486+
446487
/**
447488
* Appends one frame: writes {@code [crc32c | u32 payloadLen | payload]}
448489
* starting at the current append cursor, then advances both cursors

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -744,7 +744,11 @@ private void serviceRing0(RingEntry e) {
744744
trim = e.ring.drainTrimmable();
745745
if (registered && trim != null) {
746746
for (int i = 0, n = trim.size(); i < n; i++) {
747-
totalBytes -= trim.get(i).sizeBytes();
747+
// fileBytes(), not sizeBytes(): must mirror the register-time
748+
// seed (SegmentRing.totalSegmentBytes), which accounts the
749+
// on-disk footprint — recovered segments map only their
750+
// validated prefix but occupy the full file length on disk.
751+
totalBytes -= trim.get(i).fileBytes();
748752
}
749753
}
750754
}

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

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,11 @@ public SegmentRing(MmapSegment initialActive, long maxBytesPerSegment) {
139139
* directory, opens each via {@link MmapSegment#openExisting}, and
140140
* arranges them by baseSeq:
141141
* <ul>
142-
* <li>Highest-baseSeq segment becomes the active (further appends land
143-
* there until it fills, at which point normal rotation kicks in).</li>
142+
* <li>Highest-baseSeq segment becomes the active. Recovered segments
143+
* are mapped only through their validated prefix (SIGBUS hardening
144+
* in {@link MmapSegment#openExisting}) and are therefore born full:
145+
* the first append backpressures until the manager installs a hot
146+
* spare, then rotates onto it.</li>
144147
* <li>All others become sealed segments awaiting ACK and trim.</li>
145148
* </ul>
146149
* Returns {@code null} if the directory is empty or contains no
@@ -291,10 +294,12 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) {
291294
+ " check disk health");
292295
}
293296
}
294-
// The newest segment becomes the active. Even if it's full, that's OK:
295-
// the next appendOrFsn returns BACKPRESSURE_NO_SPARE, the manager
296-
// installs a hot spare, the producer rotates. Same fast path as a
297-
// mid-life ring.
297+
// The newest segment becomes the active. Recovered segments are
298+
// born full (mapped only through the validated prefix -- see the
299+
// SIGBUS hardening in MmapSegment.openExisting), so the next
300+
// appendOrFsn returns BACKPRESSURE_NO_SPARE, the manager installs
301+
// a hot spare, the producer rotates. Same fast path as a mid-life
302+
// ring whose active just filled.
298303
int last = opened.size() - 1;
299304
MmapSegment active = opened.get(last);
300305
opened.remove(last);
@@ -682,21 +687,25 @@ public synchronized int snapshotSealedSegments(MmapSegment[] target) {
682687
}
683688

684689
/**
685-
* Total mmap'd bytes the ring currently owns: active + hot spare (if
690+
* Total on-disk bytes the ring currently owns: active + hot spare (if
686691
* installed) + every sealed segment. Used by {@code SegmentManager}
687692
* to seed its {@code totalBytes} accounting at register time and to
688-
* reverse the contribution at deregister time. Synchronized against
689-
* rotation so we never read a half-resized sealed list.
693+
* reverse the contribution at deregister time. Uses
694+
* {@link MmapSegment#fileBytes()} — the file's logical length — rather
695+
* than the mapped size, because recovered segments map only their
696+
* validated prefix while the file keeps its full length until trim
697+
* unlinks it. Synchronized against rotation so we never read a
698+
* half-resized sealed list.
690699
*/
691700
public synchronized long totalSegmentBytes() {
692701
long total = 0L;
693702
MmapSegment a = active;
694-
if (a != null) total += a.sizeBytes();
703+
if (a != null) total += a.fileBytes();
695704
MmapSegment hs = hotSpare;
696-
if (hs != null) total += hs.sizeBytes();
705+
if (hs != null) total += hs.fileBytes();
697706
for (int i = 0, n = sealedSegments.size(); i < n; i++) {
698707
MmapSegment s = sealedSegments.get(i);
699-
if (s != null) total += s.sizeBytes();
708+
if (s != null) total += s.fileBytes();
700709
}
701710
return total;
702711
}

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment;
2828
import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegmentException;
29+
import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentRing;
2930
import io.questdb.client.std.Files;
3031
import io.questdb.client.std.FilesFacade;
3132
import io.questdb.client.std.MemoryTag;
@@ -91,6 +92,84 @@ public void testRecoveryKeepsFramesBeforeUnbackedTail() throws Exception {
9192
});
9293
}
9394

95+
/**
96+
* Regression for the recovered-sparse-tail SIGBUS: recovery used to mmap
97+
* the file's complete logical size RW and leave the sparse tail as
98+
* writable capacity, so the next append stored straight into the unbacked
99+
* hole — which under ENOSPC raises SIGBUS and aborts the JVM.
100+
* {@link FilesFacade#allocate} cannot retroactively fill holes below EOF,
101+
* so no recovery-time reservation can make that tail safe; instead the
102+
* fix maps only the validated prefix. The recovered segment must be born
103+
* full and refuse appends, forcing rotation onto a freshly created,
104+
* genuinely block-reserved segment.
105+
*/
106+
@Test
107+
public void testRecoveredSparseSegmentIsBornFullAndRefusesAppends() throws Exception {
108+
TestUtils.assertMemoryLeak(() -> {
109+
final String path = tmpDir + "/seg-sparse-no-append.sfa";
110+
final long page = Files.PAGE_SIZE;
111+
final int payloadLen = (int) (page - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE);
112+
long boundary = writeSegment(path, 5L, new int[]{payloadLen});
113+
assertEquals("frame must fill exactly one page", page, boundary);
114+
punchSparseTail(path, page);
115+
116+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
117+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
118+
assertEquals("recovered data must remain readable", 1L, seg.frameCount());
119+
assertEquals("scan must stop at the unbacked-page boundary", page, seg.publishedOffset());
120+
assertEquals("mapping must cover only the validated prefix", page, seg.sizeBytes());
121+
assertTrue("recovered segment must be born full", seg.isFull());
122+
assertEquals("no payload capacity may remain over the sparse tail",
123+
0L, seg.capacityRemaining());
124+
assertEquals("append into the unbacked tail must be refused",
125+
-1L, seg.tryAppend(buf, 16));
126+
} finally {
127+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
128+
}
129+
});
130+
}
131+
132+
/**
133+
* Recovery-to-producer path for the same regression: a ring recovered
134+
* around a sparse-tail active must backpressure the first append (never
135+
* touching the hole) and resume cleanly — with a contiguous FSN sequence
136+
* — once a genuinely allocated hot spare is installed and rotated in.
137+
*/
138+
@Test
139+
public void testRecoveredRingRotatesBeforeAppendingPastSparseTail() throws Exception {
140+
TestUtils.assertMemoryLeak(() -> {
141+
final String path = tmpDir + "/sf-0.sfa";
142+
final long page = Files.PAGE_SIZE;
143+
final int payloadLen = (int) (page - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE);
144+
long boundary = writeSegment(path, 0L, new int[]{payloadLen});
145+
assertEquals("frame must fill exactly one page", page, boundary);
146+
punchSparseTail(path, page);
147+
148+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
149+
try {
150+
try (SegmentRing recovered = SegmentRing.openExisting(tmpDir, SEGMENT_BYTES)) {
151+
assertTrue("ring must recover from the on-disk segment", recovered != null);
152+
assertEquals(1L, recovered.getActive().frameCount());
153+
assertEquals(1L, recovered.nextSeqHint());
154+
// First append must NOT write into the sparse tail — the
155+
// born-full active backpressures until a spare arrives.
156+
assertEquals(SegmentRing.BACKPRESSURE_NO_SPARE,
157+
recovered.appendOrFsn(buf, 16));
158+
// create() pre-allocates real blocks, so the rotated-in
159+
// spare is safe to store into.
160+
recovered.installHotSpare(
161+
MmapSegment.create(tmpDir + "/sf-1.sfa", 1L, SEGMENT_BYTES));
162+
assertEquals("append must resume with a contiguous FSN",
163+
1L, recovered.appendOrFsn(buf, 16));
164+
assertEquals("append must land in the rotated-in spare",
165+
1L, recovered.getActive().baseSeq());
166+
}
167+
} finally {
168+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
169+
}
170+
});
171+
}
172+
94173
/**
95174
* The harder case: a frame whose 8-byte header sits on a backed page but
96175
* whose payload reaches into the unbacked hole (a torn write leaves a real

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,8 @@ public void testOpenExistingRecoversActivePlusSealed() throws Exception {
294294
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
295295
try {
296296
// Write three segments with FSN ranges 0..3, 4..7, 8..9 (last
297-
// partially full so the recovered ring has appendable room).
297+
// partially full; recovery still maps it born-full — see the
298+
// SIGBUS hardening in MmapSegment.openExisting).
298299
MmapSegment s0 = MmapSegment.create(tmpDir + "/r0.sfa", 0, segSize);
299300
for (int i = 0; i < 4; i++) s0.tryAppend(buf, 16);
300301
s0.close();
@@ -319,8 +320,17 @@ public void testOpenExistingRecoversActivePlusSealed() throws Exception {
319320
assertEquals(4, recovered.getSealedSegments().get(1).baseSeq());
320321
// nextSeq must continue past the recovered frames.
321322
assertEquals(10, recovered.nextSeqHint());
322-
// Further appends land into the active and assign FSN 10.
323+
// Recovered segments are mapped only through their validated
324+
// prefix and are born full: the first append backpressures
325+
// (never writing into the potentially unbacked tail) until a
326+
// spare is installed, then rotates and assigns FSN 10.
327+
assertEquals(SegmentRing.BACKPRESSURE_NO_SPARE,
328+
recovered.appendOrFsn(buf, 16));
329+
recovered.installHotSpare(
330+
MmapSegment.create(tmpDir + "/r3.sfa", 10, segSize));
323331
assertEquals(10, recovered.appendOrFsn(buf, 16));
332+
assertEquals("append must land in the rotated-in spare",
333+
10, recovered.getActive().baseSeq());
324334
}
325335
} finally {
326336
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);

0 commit comments

Comments
 (0)