Skip to content

Commit 94e3b1d

Browse files
glasstigerclaude
andcommitted
Guard the symbol-dict reopen against a >2GB file
open() cast the on-disk file length to int before reading the whole file into one native buffer. A .symbol-dict that grew past 2GB (an extreme ~100M+ distinct symbols on one slot) therefore reopened with a negative malloc, a malloc(0) followed by a 4-byte out-of-bounds read, or a small-positive prefix whose validLen<len path then destructively truncated the multi-GB file. open() now rejects a length above Integer.MAX_VALUE up front and recreates the dictionary empty -- fail-clean, matching every other unreadable-file case, so the sender falls back to full self-sufficient frames. Thread a FilesFacade through PersistedSymbolDict (production keeps FilesFacade.INSTANCE, so behavior is unchanged; callers use the INSTANCE-default overloads) so a test can fault-inject a failing truncate. Add testTruncateFailureRecreatesEmpty: a torn-tail file whose tail truncate fails is recreated empty rather than trusted over stale bytes, covering the previously untested fail-clean branch the per-entry-CRC hardening added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 40c6a28 commit 94e3b1d

2 files changed

Lines changed: 254 additions & 26 deletions

File tree

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

Lines changed: 68 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import io.questdb.client.cutlass.qwp.client.GlobalSymbolDictionary;
2828
import io.questdb.client.cutlass.qwp.client.NativeBufferWriter;
2929
import io.questdb.client.std.Crc32c;
30-
import io.questdb.client.std.Files;
30+
import io.questdb.client.std.FilesFacade;
3131
import io.questdb.client.std.MemoryTag;
3232
import io.questdb.client.std.ObjList;
3333
import io.questdb.client.std.QuietCloseable;
@@ -120,6 +120,10 @@ public final class PersistedSymbolDict implements QuietCloseable {
120120
static final byte VERSION = 2; // v2 appended the per-entry CRC-32C
121121
private static final Logger LOG = LoggerFactory.getLogger(PersistedSymbolDict.class);
122122
private final int fd;
123+
// Filesystem seam. Production is FilesFacade.INSTANCE (straight to Files);
124+
// tests inject a fault facade to exercise recovery I/O failures (a truncate
125+
// that cannot drop a torn tail, a short write) without a real broken disk.
126+
private final FilesFacade ff;
123127
private long appendOffset;
124128
private boolean closed;
125129
// In-memory copy of the entry region [len][utf8]... exactly as on disk,
@@ -136,7 +140,8 @@ public final class PersistedSymbolDict implements QuietCloseable {
136140
private int scratchCap;
137141
private int size;
138142

139-
private PersistedSymbolDict(int fd, long appendOffset, int size, long loadedEntriesAddr, int loadedEntriesLen) {
143+
private PersistedSymbolDict(FilesFacade ff, int fd, long appendOffset, int size, long loadedEntriesAddr, int loadedEntriesLen) {
144+
this.ff = ff;
140145
this.fd = fd;
141146
this.appendOffset = appendOffset;
142147
this.size = size;
@@ -153,17 +158,40 @@ private PersistedSymbolDict(int fd, long appendOffset, int size, long loadedEntr
153158
* so a broken side-file degrades gracefully rather than aborting the sender.
154159
*/
155160
public static PersistedSymbolDict open(String slotDir) {
161+
return open(FilesFacade.INSTANCE, slotDir);
162+
}
163+
164+
/**
165+
* Facade-aware variant of {@link #open(String)}. Production passes
166+
* {@link FilesFacade#INSTANCE}; tests inject a fault facade to drive recovery
167+
* I/O failures (e.g. a truncate that cannot drop a torn tail).
168+
*/
169+
public static PersistedSymbolDict open(FilesFacade ff, String slotDir) {
156170
String filePath = slotDir + "/" + FILE_NAME;
157-
long existing = Files.exists(filePath) ? Files.length(filePath) : -1L;
171+
long existing = ff.exists(filePath) ? ff.length(filePath) : -1L;
172+
// A dictionary that legitimately grew past Integer.MAX_VALUE cannot be
173+
// reopened: openExisting reads it into ONE int-sized native buffer, and
174+
// the (int) cast of a >2GB length is either negative (malloc rejects it),
175+
// exactly zero (getInt then reads 4 bytes past a zero-size allocation), or
176+
// a small positive prefix (whose validLen < len branch would then
177+
// DESTRUCTIVELY truncate the multi-GB file). Recreate empty instead --
178+
// fail-clean, exactly like every other unreadable-file case here, so the
179+
// sender falls back to full self-sufficient frames. Reaching this needs
180+
// ~100M+ distinct symbols on one slot (far past realistic symbol
181+
// cardinality); the guard keeps the read/write size boundary safe anyway.
182+
if (existing > Integer.MAX_VALUE) {
183+
LOG.warn("symbol dict {} too large ({} bytes) to reopen; recreating empty", filePath, existing);
184+
return openFresh(ff, filePath);
185+
}
158186
if (existing >= HEADER_SIZE) {
159-
PersistedSymbolDict d = openExisting(filePath, existing);
187+
PersistedSymbolDict d = openExisting(ff, filePath, existing);
160188
if (d != null) {
161189
return d;
162190
}
163191
// Fall through to a clean re-create: a header/parse failure on an
164192
// existing file means it cannot be trusted for delta replay.
165193
}
166-
return openFresh(filePath);
194+
return openFresh(ff, filePath);
167195
}
168196

169197
/**
@@ -183,7 +211,14 @@ public static PersistedSymbolDict open(String slotDir) {
183211
* does.
184212
*/
185213
public static PersistedSymbolDict openClean(String slotDir) {
186-
return openFresh(slotDir + "/" + FILE_NAME);
214+
return openClean(FilesFacade.INSTANCE, slotDir);
215+
}
216+
217+
/**
218+
* Facade-aware variant of {@link #openClean(String)}.
219+
*/
220+
public static PersistedSymbolDict openClean(FilesFacade ff, String slotDir) {
221+
return openFresh(ff, slotDir + "/" + FILE_NAME);
187222
}
188223

189224
/**
@@ -194,7 +229,14 @@ public static PersistedSymbolDict openClean(String slotDir) {
194229
* failed delete cannot leave a stale dictionary a new session would trust.
195230
*/
196231
public static void removeOrphan(String slotDir) {
197-
Files.remove(slotDir + "/" + FILE_NAME);
232+
removeOrphan(FilesFacade.INSTANCE, slotDir);
233+
}
234+
235+
/**
236+
* Facade-aware variant of {@link #removeOrphan(String)}.
237+
*/
238+
public static void removeOrphan(FilesFacade ff, String slotDir) {
239+
ff.remove(slotDir + "/" + FILE_NAME);
198240
}
199241

200242
/**
@@ -258,7 +300,7 @@ public synchronized void appendRawEntries(long addr, int len, int count) {
258300
+ FILE_NAME + " [count=" + count + ", len=" + len
259301
+ ", consumed=" + (int) (src - addr) + ']');
260302
}
261-
long written = Files.write(fd, scratchAddr, outLen, appendOffset);
303+
long written = ff.write(fd, scratchAddr, outLen, appendOffset);
262304
if (written != outLen) {
263305
throw new IllegalStateException("short write to " + FILE_NAME
264306
+ " [expected=" + outLen + ", actual=" + written + ']');
@@ -290,7 +332,7 @@ public synchronized void appendSymbol(CharSequence symbol) {
290332
Utf8s.strCpyUtf8(symbol, p, utf8Len);
291333
}
292334
Unsafe.getUnsafe().putInt(scratchAddr + wireLen, Crc32c.update(Crc32c.INIT, scratchAddr, wireLen));
293-
long written = Files.write(fd, scratchAddr, recLen, appendOffset);
335+
long written = ff.write(fd, scratchAddr, recLen, appendOffset);
294336
if (written != recLen) {
295337
throw new IllegalStateException("short write to " + FILE_NAME
296338
+ " [expected=" + recLen + ", actual=" + written + ']');
@@ -333,7 +375,7 @@ public synchronized void appendSymbols(GlobalSymbolDictionary dict, int from, in
333375
Unsafe.getUnsafe().putInt(entryStart + wireLen, Crc32c.update(Crc32c.INIT, entryStart, wireLen));
334376
len += wireLen + CRC_SIZE;
335377
}
336-
long written = Files.write(fd, scratchAddr, len, appendOffset);
378+
long written = ff.write(fd, scratchAddr, len, appendOffset);
337379
if (written != len) {
338380
throw new IllegalStateException("short write to " + FILE_NAME
339381
+ " [expected=" + len + ", actual=" + written + ']');
@@ -362,7 +404,7 @@ public synchronized void close() {
362404
scratchCap = 0;
363405
}
364406
if (fd >= 0) {
365-
Files.close(fd);
407+
ff.close(fd);
366408
}
367409
}
368410

@@ -456,8 +498,8 @@ private static long[] decodeVarint(long buf, int pos, int limit) {
456498
return null;
457499
}
458500

459-
private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
460-
int fd = Files.openRW(filePath);
501+
private static PersistedSymbolDict openExisting(FilesFacade ff, String filePath, long fileLen) {
502+
int fd = ff.openRW(filePath);
461503
if (fd < 0) {
462504
LOG.warn("symbol dict {} could not be opened (rc={}); recreating", filePath, fd);
463505
return null;
@@ -468,13 +510,13 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
468510
try {
469511
int len = (int) fileLen;
470512
buf = Unsafe.malloc(len, MemoryTag.NATIVE_DEFAULT);
471-
long read = Files.read(fd, buf, len, 0);
513+
long read = ff.read(fd, buf, len, 0);
472514
if (read != len
473515
|| Unsafe.getUnsafe().getInt(buf) != FILE_MAGIC
474516
|| Unsafe.getUnsafe().getByte(buf + 4) != VERSION) {
475517
LOG.warn("symbol dict {} unreadable, bad magic or unknown version; recreating", filePath);
476518
Unsafe.free(buf, len, MemoryTag.NATIVE_DEFAULT);
477-
Files.close(fd);
519+
ff.close(fd);
478520
return null;
479521
}
480522
// Parse complete, CRC-valid entries after the header; stop at the first
@@ -555,15 +597,15 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
555597
// open() then recreates it empty (fail-clean) rather than risk a silent
556598
// misattribution.
557599
long validLen = HEADER_SIZE + diskConsumed;
558-
if (validLen < len && !Files.truncate(fd, validLen)) {
600+
if (validLen < len && !ff.truncate(fd, validLen)) {
559601
LOG.warn("symbol dict {} could not drop its torn/stale tail (truncate failed); recreating", filePath);
560602
if (entriesAddr != 0L) {
561603
Unsafe.free(entriesAddr, entriesLen, MemoryTag.NATIVE_DEFAULT);
562604
}
563-
Files.close(fd);
605+
ff.close(fd);
564606
return null;
565607
}
566-
return new PersistedSymbolDict(fd, validLen, count, entriesAddr, entriesLen);
608+
return new PersistedSymbolDict(ff, fd, validLen, count, entriesAddr, entriesLen);
567609
} catch (Throwable t) {
568610
if (buf != 0L) {
569611
Unsafe.free(buf, (int) fileLen, MemoryTag.NATIVE_DEFAULT);
@@ -575,14 +617,14 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
575617
if (entriesAddr != 0L) {
576618
Unsafe.free(entriesAddr, entriesLen, MemoryTag.NATIVE_DEFAULT);
577619
}
578-
Files.close(fd);
620+
ff.close(fd);
579621
LOG.warn("symbol dict {} recovery failed ({}); recreating", filePath, String.valueOf(t));
580622
return null;
581623
}
582624
}
583625

584-
private static PersistedSymbolDict openFresh(String filePath) {
585-
int fd = Files.openCleanRW(filePath);
626+
private static PersistedSymbolDict openFresh(FilesFacade ff, String filePath) {
627+
int fd = ff.openCleanRW(filePath);
586628
if (fd < 0) {
587629
LOG.warn("symbol dict {} could not be created (rc={}); proceeding without it", filePath, fd);
588630
return null;
@@ -595,10 +637,10 @@ private static PersistedSymbolDict openFresh(String filePath) {
595637
Unsafe.getUnsafe().putByte(hdr + 5, (byte) 0);
596638
Unsafe.getUnsafe().putByte(hdr + 6, (byte) 0);
597639
Unsafe.getUnsafe().putByte(hdr + 7, (byte) 0);
598-
long written = Files.write(fd, hdr, HEADER_SIZE, 0);
640+
long written = ff.write(fd, hdr, HEADER_SIZE, 0);
599641
if (written != HEADER_SIZE) {
600-
Files.close(fd);
601-
Files.remove(filePath);
642+
ff.close(fd);
643+
ff.remove(filePath);
602644
LOG.warn("symbol dict {} header write failed; proceeding without it", filePath);
603645
return null;
604646
}
@@ -607,15 +649,15 @@ private static PersistedSymbolDict openFresh(String filePath) {
607649
// throwing; the Unsafe puts target a valid 8-byte buffer and an 8-byte
608650
// malloc cannot realistically OOM), but close the fd against a future
609651
// edit so it cannot leak -- mirroring openExisting's error handling.
610-
Files.close(fd);
652+
ff.close(fd);
611653
LOG.warn("symbol dict {} creation failed ({}); proceeding without it", filePath, String.valueOf(t));
612654
return null;
613655
} finally {
614656
if (hdr != 0L) {
615657
Unsafe.free(hdr, HEADER_SIZE, MemoryTag.NATIVE_DEFAULT);
616658
}
617659
}
618-
return new PersistedSymbolDict(fd, HEADER_SIZE, 0, 0L, 0);
660+
return new PersistedSymbolDict(ff, fd, HEADER_SIZE, 0, 0L, 0);
619661
}
620662

621663
private void ensureScratch(int required) {

0 commit comments

Comments
 (0)