Harden GridStore loader against malformed files (SIGFPE + OOB read)#1078
Open
robert-kofler42 wants to merge 1 commit into
Open
Harden GridStore loader against malformed files (SIGFPE + OOB read)#1078robert-kofler42 wants to merge 1 commit into
robert-kofler42 wants to merge 1 commit into
Conversation
robert-kofler42
requested review from
SuperOptimizer and
hendrikschilling
as code owners
|
Someone is attempting to deploy a commit to the scroll Team on Vercel. A member of the Team first needs to authorize it. |
The GridStore loader read sizing and offset fields straight from untrusted file bytes with no validation, so a corrupt/truncated file could crash, read out of bounds, or exhaust memory rather than fail cleanly: - cell_size is read into an int and used as a divisor when computing grid_size_. A zero value (or a >INT_MAX value that reads as negative) caused an integer divide-by-zero / SIGFPE in load_mmap. - bounds.width can be up to INT_MAX (a valid u32), so the signed (bounds.width + cell_size - 1) ceil-divide overflowed (UB, traps on UBSan). - The v3 bucket-index array and path-offset list are read on demand via read_be_u32_at, which does no bounds checking, and the offsets were stored unvalidated. A bucket range with end_idx < start_idx underflowed the unsigned count and drove an out-of-bounds read loop; offsets past EOF read off the end of the mapping. - The legacy v1/v2 path did grid_bucket_descriptors_.resize(num_buckets) with num_buckets straight from the header and no file-size cross-check, so a 44-byte file claiming ~4.3e9 buckets drove a ~64 GiB allocation / OOM before any per-bucket end check. Fixes: - Reject cell_size <= 0 and negative bounds before the divide; do the ceil-divide in 64-bit to avoid signed overflow. - For v3, validate at load that the bucket-index array and paths_offset lie within the file; in get_bucket_offsets bound-check the index, reject inverted ranges, and validate the path-offset region before reading. - For v1/v2, require the descriptor region (>= num_buckets*4 bytes at buckets_offset) to fit in the file before the resize. uint64 math avoids hostile-input overflow throughout. Valid files are unaffected (existing GridStore tests still pass). Adds test_gridstore_malformed (6 cases): valid file still loads; cell_size=0 throws (was SIGFPE/signal 8); buckets_offset and paths_offset past EOF throw; a v1 header with huge num_buckets throws instead of OOM; width near INT_MAX loads without overflow UB.
robert-kofler42
force-pushed
the
harden/gridstore-loader
branch
from
36a0820 to
d5fabe5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
The v3
GridStoreloader (GridStore.cpp) reads sizing and offset fields straight from untrusted file bytes with no validation, so a corrupt or truncated.gridfile could crash or read out of bounds instead of failing cleanly:cell_size→ SIGFPE.cell_sizeis read into anintand used as a divisor when computinggrid_size_(GridStore.cpp:516). A zero value (or a>INT_MAXvalue that reads as negative) is an integer divide-by-zero / SIGFPE inload_mmap.read_be_u32_at, which does no bounds checking, andbuckets_offset/paths_offsetwere stored unvalidated. A bucket range withend_idx < start_idxunderflows the unsignedcount = end_idx - start_idxand drives an out-of-bounds read loop; offsets past EOF read off the end of the mapping.Fix
cell_size <= 0and negative bounds inload_mmapbefore the divide.num_buckets+1u32s atbuckets_offset) andpaths_offsetlie within the file.get_bucket_offsets(v3): bound-check the bucket index, reject inverted ranges (end_idx < start_idx), and validate the path-offset region against the file size before reading.uint64math avoids hostile-input overflow.Valid files are unaffected.
Verification
Adds
test_gridstore_malformed: builds a valid file, corrupts one header word, and checks the loader throwsstd::runtime_errorinstead of crashing.test_gridstore/test_gridstore_branchesstill pass (hardening doesn't reject good files).cell_size=0case is a SIGFPE (signal 8) on the unfixed loader and a clean throw after the fix — verified by building the same test against the pre-fixGridStore.cpp(crashes) and the fixed one (4/4 pass).