fix(storage): validate allocated page ranges and page indices on the write path - #950
Conversation
ffe89d2 to
6dac4e6
Compare
6dac4e6 to
3b07a94
Compare
|
Updated: force-pushed a reduced version after your CI caught a genuine defect in my first
That test is right and I was wrong: recovery replaying a checkpoint legitimately writes past the What is left is the single guard in One other thing from that run, for the record rather than as a claim about my change: |
|
@adsharma for some reason I cannot add you as a reviewer but please serve yourself to check and destroy my PR with your knowledge. |
|
Thank you for the contribution! The code looks correct to me. Minor nits - please fix. |
A page index becomes a file offset on the write path exactly as it does on the read path, but only reads validated it (LadybugDB#946). Nothing checked the ranges the allocator returned, so a corrupted free page list silently extended the data file instead of failing: on the database from LadybugDB#843, PageManager returned page 212,650,604 of a 2884-page file and a checkpoint column flush wrote there, leaving an 11.8 MB database reporting 892.2 GiB of apparent size (LadybugDB#948). Allocated blocks barely move, so nothing looks wrong until a copy or backup materialises the sparse hole, which filled a 926 GiB volume here. PageManager::allocatePageRange now rejects a range that is not inside the data file, naming the range and the file's page count. The free list is deserialized from the database file, so a corrupted entry surfaces here first, and nothing shrinks the data file, so a legitimate free entry is always inside it. The existing DASSERT on the extension path is kept and backed by a real check, since DASSERT is stripped in release builds. Zero-page allocations are exempt: they write nothing, and their start index legitimately sits at the current end of the file. Verified against the LadybugDB#948 reproducer (a content-scrubbed copy of the LadybugDB#843 database, https://github.com/fabzter/ladybug-948-repro): Page allocation from the free page list returned pages [212650604, 212650605), which are out of bounds for a data file with 2884 pages. The free page list may be corrupted. The file stays 11.8 MB instead of reaching 892 GiB, and the error identifies the corrupted free list rather than surfacing as a SIGSEGV. Checks for false positives: 2000-cycle insert+CHECKPOINT soaks on a clean store carrying FTS and HNSW indexes pass on two seeds, and a smoke covering DDL, 3000 inserts with strings up to 3 KB, deletes that free pages, reuse of those freed pages through the free list, and EXPORT/IMPORT all pass.
3b07a94 to
f11501d
Compare
Fixes #948.
Symptom
A single bad page index extends the data file far past its end. On the database from #843 an
11.8 MB file ends up reporting 892.2 GiB of apparent size while only ~12 MiB is allocated.
The database still opens and queries fine, so nothing looks wrong — until something copies or
archives it and the sparse hole is materialised. That is how I found it: one
shutil.copy2()filled a 926 GiB volume to 100% (523 MiB free) before failing with
ENOSPC. A backup tool,tar, or a container build would do the same.Reproduced identically on 0.19.1, 0.20.2 and main, always the same byte count.
Root cause
A page index becomes a file offset on the write path exactly as it does on the read path, but
only reads were validated (#946). Nothing checked the ranges the allocator handed out.
I traced the actual write with a backtrace on an instrumented build. It is not the shadow-replay
path I first suspected:
The range came from
PageManager::allocatePageRange, which returns whateverFreeSpaceManager::popFreePagesyields. That free list is deserialized from the database file, soa corrupted entry is handed out as if it were real: on this database, page 212,650,604 of a
2884-page file. The checkpoint flush then writes there and the file grows to 892 GiB.
This also closes a gap in #843: the guard's message proves that database's free page list is
corrupted, which is the missing link between the page corruption reported there and the file
inflation reported in #948.
Fix
PageManager::allocatePageRangerejects a range that is not inside the data file, naming therange and the file's page count. This is where a corrupted free list surfaces first, and nothing
shrinks the data file, so a legitimate free entry is always inside it. The existing
DASSERTonthe extension path is kept and backed by a real check, since
DASSERTis stripped in releasebuilds. Zero-page allocations are exempt — they write nothing, and their start index legitimately
sits at the current end of the file. Style and message shape follow #946.
An earlier revision of this PR also guarded the
FileHandlewrite paths and both shadow-replaywrites as defense in depth. I removed those, because your CI showed the recovery-path bound
was simply wrong:
FlakyCheckpointerTest.RecoverFromCheckpointApplyingShadowFailurelegitimatelyreplays a shadow page into data page 266 of a file that currently has 265 pages — extending the
data file is exactly what recovery is for, so there is no sound page-count bound available at
that point. I could not prove the spiller's use of
writePagesToFilesafe either. The remainingchange is the one place with an authoritative bound, and it is the place the fault actually
originates.
Verification
On the #948 reproducer (a content-scrubbed copy of the #843 database, published at
https://github.com/fabzter/ladybug-948-repro so this is reproducible without my private data):
The file stays 11.8 MB instead of reaching 892 GiB, and the error identifies the corrupted free
list instead of surfacing as a SIGSEGV.
False-positive checks (an earlier revision of this patch rejected zero-page allocations, which the
first of these caught):
CHECKPOINTsoak on a clean store carrying FTS and HNSW indexes, seeds 1and 2: both complete, row counts correct.
those freed pages through the free list (the path this patch guards), and
EXPORT/IMPORT:all pass.
I have not run the C++ suite locally —
BUILD_TESTSwas off in my build directory — so CI is thefirst full run. I did not add a unit test because provoking this needs a database whose free list
is already corrupt; happy to add a direct unit test of the guards if you would like one.
Relationship to the other open reports
push_backinOptimisticAllocatorthat corrupts the heap during parallel COPY. It touches onlyoptimistic_allocator.{h,cpp}, so there is no overlap with this change, and the two are complementary: Fix data race in OptimisticAllocator during parallel COPY #949 removes a cause of corruption, this fails fast when corrupted allocator state would otherwise reach the disk.