Harden OnDiskInvertedLists mmap reader against malformed list sizes#5312
Harden OnDiskInvertedLists mmap reader against malformed list sizes#5312AgentGymLeader wants to merge 3 commits into
Conversation
Bring the OnDisk reader deserialization loop in read_ArrayInvertedLists()
to parity with the Array/Block reader guards already present in the codebase.
Previously the per-list accumulation used an unchecked multiply-then-add:
o += l.size * (sizeof(idx_t) + ails->code_size);
This is vulnerable to two classes of malformed input:
1. Integer overflow in the multiplication (a large l.size wraps to a
small byte count, causing subsequent mmap reads out of bounds).
2. Integer overflow in the addition (a crafted list sequence accumulates
o past the mapped file size without triggering the single pre-loop guard).
The replacement uses mul_no_overflow() and add_no_overflow() (both declared
in faiss/impl/FaissAssert.h and already in scope via existing FAISS_THROW_*
usage in this TU), and adds a per-iteration FAISS_THROW_IF_NOT_FMT guard
validating the running offset against ails->totsize after each list, with
diagnostic output of list index, offset, byte count, and mapped file size.
The pre-loop guard FAISS_THROW_IF_NOT(o <= ails->totsize) is preserved;
the new per-iteration check provides finer-grained diagnostics alongside it.
|
Hi @AgentGymLeader! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
@alibeklfc has imported this pull request. If you are a Meta employee, you can view this in D108627881. |
The mul_no_overflow() call exceeded the 80-column limit on one line; clang-format-21 (the CI's version) splits it to one argument per line under .clang-format BinPackArguments: false.
|
@alibeklfc merged this pull request in c428ca1. |
1 participant
What
OnDiskInvertedListsIOHook::read_ArrayInvertedLists()builds each inverted list'soffsetby accumulatingsize * (sizeof(idx_t) + code_size)over the deserializedsizesvector. The per-listsizevalues come straight from the serialized index and were not bounds-checked: the multiply could overflowsize_t, and the running offset was never re-validated against the mapped file size inside the loop. The only existing guard (FAISS_THROW_IF_NOT(o <= totsize)just above the loop) runs once and validates only the starting offset.As a result, a malformed index loaded via
read_index(path, IO_FLAG_MMAP)could leave one or morelists[i].offsetpointing past the end of the mmap'd region.get_codes()/get_ids()only guard againstINVALID_OFFSET, so at search time the IVF scan reads from an out-of-range pointer — an out-of-bounds read.Fix
Inside the offset-accumulation loop, this change:
mul_no_overflowforsize * (sizeof(idx_t) + code_size),add_no_overflowwhen advancing the running offset, ando <= totsizeon every iteration with a descriptive message.This mirrors the deserialization hardening already present for the Array and Block inverted-list readers in
faiss/impl/index_read.cpp(which usemul_no_overflow+ per-list byte-limit checks), bringing the OnDisk mmap reader to parity. Well-formed indexes are unaffected — the guards only reject inputs whose declared list sizes do not fit the mapped file.Verification
clang++ -std=c++17 -fsyntax-onlycleanly. The three helpers (mul_no_overflow,add_no_overflow,FAISS_THROW_IF_NOT_FMT) are already in scope viafaiss/impl/FaissAssert.hand used throughout this file.Reported via coordinated disclosure (huntr).