Validate inverted list entry sizes against deserialization byte limit (#5117)#5117
Closed
scsiguy wants to merge 8 commits into
Closed
Validate inverted list entry sizes against deserialization byte limit (#5117)#5117scsiguy wants to merge 8 commits into
scsiguy wants to merge 8 commits into
Conversation
Contributor
|
@scsiguy has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101260923. |
scsiguy
added a commit
to scsiguy/faiss
that referenced
this pull request
Apr 17, 2026
…facebookresearch#5117) Summary: Add deserialization byte limit checks before vector::resize calls in read_InvertedLists_up() for both ArrayInvertedListsPanorama ("ilpn") and ArrayInvertedLists ("ilar") paths. Previously, per-list sizes read from serialized data were used directly in .resize() calls without checking against get_deserialization_vector_byte_limit(). The READVECTOR macro enforces this limit, but explicit .resize() calls bypassed it. Also add mul_no_overflow protection for the ilpn codes allocation (num_elems * code_size) which previously had no overflow check. Differential Revision: D101260923
Summary: Add utility functions to FaissException.h for capturing and rethrowing exceptions across OpenMP parallel region boundaries, and migrate IndexIVF to use them. New helpers in FaissException.h: - omp_capture_exception(ex): Captures the current exception into a shared exception_ptr using #pragma omp critical. Only records the first exception. - omp_capture_exception(ex, cleanup): Overload with a per-thread cleanup callable (e.g. setting an interrupt flag) that runs inside the critical section. - omp_rethrow_if_exception(ex): Rethrows the captured exception on the main thread after the parallel region completes. The exception_ptr approach preserves the full exception during capture and preserves the original exception type on rethrow. IndexIVF migration (search, search_preassigned, range_search_preassigned): - Replace std::mutex + std::string exception_string with std::exception_ptr + omp_capture_exception/omp_rethrow_if_exception. - Replace bool interrupt with std::atomic<bool> (the previous non-atomic bool was read without synchronization in the omp for loop while being written inside a mutex-guarded catch block — undefined behavior). Use relaxed atomic loads for the interrupt short-circuit check since there is no way to fully close the race between interrupt being set and a thread being in the middle of computation. - Replace catch (const std::exception&) with catch (...) to capture all exception types. - Original exception type is now preserved on rethrow instead of being reformatted into a FaissException wrapping demangle_cpp_symbol + what(). - Replace InterruptCallback::is_interrupted() + manual flag set with InterruptCallback::check(). check() throws on interrupt, which is captured by omp_capture_exception and sets the interrupt flag via the cleanup lambda. This eliminates the separate post-region interrupt check — all error paths now flow through the unified exception capture/rethrow mechanism. - range_search_preassigned has no interrupt short-circuit check in its loops, so it uses omp_capture_exception(ex) without a cleanup lambda and has no interrupt variable. - Wrap all #pragma omp for loop bodies in try/catch blocks so exceptions are caught within the worksharing construct (required by the OpenMP specification). The outer try/catch at the #pragma omp parallel level remains as a safety net for code outside worksharing constructs. - Remove redundant try/catch from scan_one_list and scan_list_func lambdas since all call sites are now protected by the loop body try/catch. Differential Revision: D101233059
) Summary: Pull Request resolved: facebookresearch#5105 Exceptions thrown inside OpenMP worksharing constructs in IndexFlatCodes::search call std::terminate because the OpenMP specification does not allow exceptions to escape parallel regions. This is triggered by corrupt serialized index data that causes allocation failures or assertion errors in GenericFlatCodesDistanceComputer, but can occur with any exception thrown during the search loop. Wrap the OpenMP parallel body in IndexFlatCodes::Run_search_with_decompress with per-iteration try/catch blocks that capture exceptions via std::exception_ptr and re-throw them on the main thread after the parallel region completes. The DC constructor is also wrapped separately since it runs before the worksharing loop. SingleResultHandler is moved from stack to heap allocation (std::unique_ptr) because the constructor is now inside a try/catch block. If a stack-allocated object's constructor throws, the object never comes into existence, but OpenMP requires all threads to participate in the worksharing loop that follows — any reference to the uninitialized object would be undefined behavior. With std::unique_ptr, the variable starts as nullptr before the try block, remains nullptr if construction fails, and the interrupt flag ensures all loop iterations skip via continue without dereferencing it. Differential Revision: D101008838
Summary: Apply the same OpenMP exception-safety fix from IndexFlatCodes::search to IndexNNDescent::search and IndexNSG::search. Both methods have identical OMP structure: a parallel region constructs per-thread VisitedTable and DistanceComputer objects, then a worksharing loop calls the graph search function. Exceptions thrown inside either the constructor or the loop body (e.g. from corrupt deserialized graph state) call std::terminate because OpenMP does not allow exceptions to escape worksharing constructs. Wrap the per-thread setup and the per-iteration loop body in try/catch blocks that capture exceptions via std::exception_ptr and re-throw on the main thread after the parallel region completes. Potential exception sources in the OMP region: - VisitedTable(ntotal): std::bad_alloc if ntotal is corrupt/huge. ntotal is validated as >= 0 during deserialization but has no upper bound. - storage_distance_computer(storage): may throw from get_distance_computer() if the storage index has corrupt state. - nndescent.search() / nsg.search(): throws FaissException if the graph has not been built (has_built check), or std::bad_alloc from internal vector allocations with corrupt search_L. Differential Revision: D101031002
Summary: Add null-quantizer guards to IndexIVF entry points (search, range_search, add_with_ids, train_q1) and to initialize_IVFPQ_precomputed_table. These prevent the null-deref crashes found by the fuzzer when a serialized IVF index contains a "null" fourcc for the quantizer sub-index. The guards are placed at the usage sites rather than in read_ivf_header because some callers intentionally deserialize IVF indexes with a null quantizer (serializing the coarse quantizer separately to avoid double-serialization, then reattaching it after reading). Differential Revision: D101236489
Summary: Add validation that IndexHNSW2Level (fourcc "IHN2") has storage of an appropriate type, both at deserialization time and at search time. IndexHNSW2Level::search() uses dynamic_cast to dispatch between Index2Layer and IndexIVFPQ storage types. When storage is null or a different type (e.g. IndexFlat from corrupt serialized data, or a programmatically misconfigured index), the dynamic_cast returns nullptr which is then unconditionally dereferenced, causing a segfault. Deserialization-time fix: After reading the HNSW storage sub-index for IHN2, validate that storage is non-null and is either Index2Layer or IndexIVFPQ. Search-time defense-in-depth: Add a FAISS_THROW_IF_NOT check on the dynamic_cast result in IndexHNSW2Level::search() before dereferencing. This protects against programmatically constructed indexes that bypass deserialization validation. Differential Revision: D101243603
…uerying untrained indexes Summary: Add FAISS_THROW_IF_NOT(is_trained) to IndexIVF::search(), IndexIVF::search_preassigned(), IndexIVF::range_search(), and IndexIVF::range_search_preassigned(), mirroring the existing check in IndexScalarQuantizer::search(). This prevents querying untrained IVF indexes deserialized from corrupt data where the ScalarQuantizer trained vector is empty. The existing deserialization validation in read_ScalarQuantizer (D98924927) correctly allows untrained indexes (is_trained=false with empty trained) to be deserialized, since these are legitimately produced by index_factory before training. However, IndexIVF search methods lacked the is_trained guard that IndexScalarQuantizer::search() has, allowing a deserialized untrained IndexIVFScalarQuantizer to be queried, which causes null-deref in QuantizerTemplate when it indexes into the empty trained vector. Differential Revision: D101243973
Summary: Add deserialization-time validation for VectorTransform dimension invariants that are enforced by constructors but not by deserialization: 1. NormalizationTransform (VNrm): Require d_in == d_out. The constructor enforces this (both set to d), but deserialization reads them independently. A crafted file with d_in > d_out causes memcpy in apply_noalloc to overflow the output buffer (allocated as n * d_out floats but copied as n * d_in). 2. CenteringTransform (VCnt): Same d_in == d_out invariant. 3. IndexPreTransform (IxPT) chain consistency: Validate that chain[0].d_in == index.d, chain[i].d_in == chain[i-1].d_out for consecutive transforms, and chain.back().d_out == sub_index.d. Without this, mismatched dimensions between transforms cause out-of-bounds reads when one transform produces fewer elements than the next expects. Differential Revision: D101244181
scsiguy
added a commit
to scsiguy/faiss
that referenced
this pull request
Apr 18, 2026
…facebookresearch#5117) Summary: Add deserialization byte limit checks before vector::resize calls in read_InvertedLists_up() for both ArrayInvertedListsPanorama ("ilpn") and ArrayInvertedLists ("ilar") paths. Previously, per-list sizes read from serialized data were used directly in .resize() calls without checking against get_deserialization_vector_byte_limit(). The READVECTOR macro enforces this limit, but explicit .resize() calls bypassed it. Also add mul_no_overflow protection for the ilpn codes allocation (num_elems * code_size) which previously had no overflow check. Reviewed By: mnorris11 Differential Revision: D101260923
meta-codesync
Bot
changed the title
scsiguy
force-pushed
the
export-D101260923
branch
from
309e49e to
b8d8e53
Compare
scsiguy
added a commit
to scsiguy/faiss
that referenced
this pull request
Apr 18, 2026
…facebookresearch#5117) Summary: Add deserialization byte limit checks before vector::resize calls in read_InvertedLists_up() for both ArrayInvertedListsPanorama ("ilpn") and ArrayInvertedLists ("ilar") paths. Previously, per-list sizes read from serialized data were used directly in .resize() calls without checking against get_deserialization_vector_byte_limit(). The READVECTOR macro enforces this limit, but explicit .resize() calls bypassed it. Also add mul_no_overflow protection for the ilpn codes allocation (num_elems * code_size) which previously had no overflow check. Reviewed By: mnorris11 Differential Revision: D101260923
…facebookresearch#5117) Summary: Pull Request resolved: facebookresearch#5117 Add deserialization byte limit checks before vector::resize calls in read_InvertedLists_up() for both ArrayInvertedListsPanorama ("ilpn") and ArrayInvertedLists ("ilar") paths. Previously, per-list sizes read from serialized data were used directly in .resize() calls without checking against get_deserialization_vector_byte_limit(). The READVECTOR macro enforces this limit, but explicit .resize() calls bypassed it. Also add mul_no_overflow protection for the ilpn codes allocation (num_elems * code_size) which previously had no overflow check. Reviewed By: mnorris11 Differential Revision: D101260923
scsiguy
force-pushed
the
export-D101260923
branch
from
b8d8e53 to
9537ab5
Compare
Contributor
|
This pull request has been merged in 817ecf9. |
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.
Summary:
Add deserialization byte limit checks before vector::resize calls in read_InvertedLists_up() for both ArrayInvertedListsPanorama ("ilpn") and ArrayInvertedLists ("ilar") paths. Previously, per-list sizes read from serialized data were used directly in .resize() calls without checking against get_deserialization_vector_byte_limit(). The READVECTOR macro enforces this limit, but explicit .resize() calls bypassed it.
Also add mul_no_overflow protection for the ilpn codes allocation (num_elems * code_size) which previously had no overflow check.
Reviewed By: mnorris11
Differential Revision: D101260923