Validate code_size against compute_code_size in RaBitQ flat deserialization - #5432
Open
AlexMelanFromRingo wants to merge 1 commit into
Open
Conversation
…zation The flat RaBitQ readers (fourcc Ixrq/Ixrr) take rabitq.code_size straight from the file and use it as the per-vector stride, without checking it against compute_code_size(d, nb_bits). The IVF RaBitQ readers (Iwrq/Iwrr) already recompute code_size after reading, so only the flat readers trust the stored value. decode_core() strides by code_size but locates the per-code factor block at offset (d + 7) / 8, derived from d rather than code_size. A file with a too-small code_size therefore makes search()/sa_decode() read the factor block past the end of the codes buffer. This is independent of the codes.size() == ntotal * code_size buffer-length check: a buffer sized to ntotal * (forged code_size) satisfies that check yet still overruns, because the factor read within the final code slot extends past it. Reproduced under AddressSanitizer via the public read_index + sa_decode API (d=8, forged code_size=1 vs real 9, codes buffer = ntotal bytes): heap-buffer-overflow READ in RaBitQuantizer::decode_core, on the codes buffer allocated in read_index_up. With this change read_index instead rejects the file with "IndexRaBitQ code_size mismatch: stored 1 vs derived 9". Validate the stored code_size against compute_code_size in both flat arms using the existing validate_code_size_match helper. Add RaBitQCodeSizeFieldMismatch_Ixrq / _Ixrr regression tests, and fix the push_rabitq / push_rabitq_multibit test helpers to emit a valid code_size (they previously wrote a placeholder that only passed because the reader did not validate it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
@bshethmeta has imported this pull request. If you are a Meta employee, you can view this in D112827997. |
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
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.
Summary
The flat RaBitQ readers (
fourcc("Ixrq")/fourcc("Ixrr")inread_index,faiss/impl/index_read.cpp) takerabitq.code_sizestraight from the file and use it as the per-vector stride, without checking it againstcompute_code_size(d, nb_bits). The IVF RaBitQ readers (Iwrq/Iwrr) already recomputecode_sizeafter reading (ivrq->rabitq.code_size = ivrq->rabitq.compute_code_size(...)), so only the flat readers trust the stored value.RaBitQuantizer::decode_corestrides bycode_sizebut locates the per-code factor block at offset(d + 7) / 8, derived fromdrather thancode_size. A file with a too-smallcode_sizetherefore makessearch()/sa_decode()read the factor block past the end of thecodesbuffer.This is independent of the
codes.size() == ntotal * code_sizebuffer-length check (added for these arms in #5293): acodesbuffer sized tontotal * (forged code_size)satisfies that check yet still overruns, because the factor read within the final code slot extends past the buffer end.Reproduce (heap OOB under AddressSanitizer, public API only)
A self-contained program that builds a crafted
Ixrqpayload (d=8, forgedcode_size=1vs real9,codesbuffer =ntotalbytes so thecodes.size()check passes), thenread_index+sa_decode:With this patch,
read_indexrejects the file up front:Fix
Validate the stored
code_sizeagainstcompute_code_size(d, nb_bits)in both flat arms, using the existingvalidate_code_size_matchhelper (this is the scalar stored-vs-derived case that helper is for).Also add
RaBitQCodeSizeFieldMismatch_Ixrq/_Ixrrregression tests, and fix thepush_rabitq/push_rabitq_multibittest helpers to emit a validcode_size— they previously wrote a placeholder that only passed because the reader did not validate it.Validated locally: new tests pass with the change and fail without it; the full
ReadIndexDeserializesuite passes; and the ASAN reproduction above is rejected instead of overrunning.Related to #5293 (same flat readers, complementary buffer-length check). The two are independent: #5293 bounds
codes.size(); this PR fixescode_sizeitself, which #5293 alone does not cover.🤖 Generated with Claude Code