Validate SVS storage_kind via shared helper at all deserialization read sites (#5204)

Summary:
Pull Request resolved: #5204

read_index_up reads the SVS storage_kind field from two distinct fourcc paths — the IndexSVSVamana family (ILVQ/ISVL/ISVD/ISV2) and the IndexSVSIVF family (ISIQ/ISIL/ISID).  The Vamana branch read the value into a plain int, range-checked it against [0, SVS_count), and then cast to SVSStorageKind, so a malformed payload was rejected at the deserialization boundary with a clear error.  The IVF branch read the value directly into the SVSStorageKind enum field with no range check.  A corrupt or maliciously constructed payload could store an out-of-range enum value and continue reading several more fields; the bad value was only noticed later from to_svs_storage_kind() inside IndexSVSIVF::deserialize_impl, after the SVS runtime load had been entered, producing a less actionable error from a deeper site.

Replace the open-coded Vamana check and the missing IVF check with a single static read_svs_storage_kind(IOReader*) helper in index_read.cpp.  The helper performs the four-byte read and the [0, SVS_count) validation in one place and returns the validated enum, so each call site collapses to a one-line assignment.  Future SVS read sites cannot diverge in their validation strategy without going around this helper.  The helper preserves the existing "invalid SVS storage_kind=N (must be in [0, M))" error message, so no externally-visible error string changes.

Reviewed By: mnorris11

Differential Revision: D104481541

fbshipit-source-id: 7e67d84b93ed142fcac39336c6bd6fdeda48cdb5

Author: scsiguy

faiss/impl/index_read.cpp

@@ -86,6 +86,24 @@ namespace faiss {
 namespace {
 size_t deserialization_loop_limit_ = 0;
 size_t deserialization_vector_byte_limit_ = uint64_t{1} << 40; // 1 TB
+
+#ifdef FAISS_ENABLE_SVS
+// Read and validate an SVSStorageKind from the stream. Centralizes the
+// [0, SVS_count) range check so every SVS read site rejects out-of-range
+// values uniformly at the deserialization boundary, instead of letting
+// to_svs_storage_kind() surface the failure later from inside an SVS
+// runtime load.
+SVSStorageKind read_svs_storage_kind(IOReader* f) {
+    int sk;
+    READ1(sk);
+    FAISS_THROW_IF_NOT_FMT(
+            sk >= 0 && sk < static_cast<int>(SVS_count),
+            "invalid SVS storage_kind=%d (must be in [0, %d))",
+            sk,
+            static_cast<int>(SVS_count));
+    return static_cast<SVSStorageKind>(sk);
+}
+#endif // FAISS_ENABLE_SVS
 } // namespace
 
 size_t get_deserialization_loop_limit() {
@@ -2556,14 +2574,7 @@ std::unique_ptr<Index> read_index_up(IOReader* f, int io_flags) {
         READ1(svs->prune_to);
         READ1(svs->use_full_search_history);
 
-        int sk;
-        READ1(sk);
-        FAISS_THROW_IF_NOT_FMT(
-                sk >= 0 && sk < static_cast<int>(SVS_count),
-                "invalid SVS storage_kind=%d (must be in [0, %d))",
-                sk,
-                static_cast<int>(SVS_count));
-        svs->storage_kind = static_cast<SVSStorageKind>(sk);
+        svs->storage_kind = read_svs_storage_kind(f);
 
         if (h == fourcc("ISVL")) {
             auto* leanvec = dynamic_cast<IndexSVSVamanaLeanVec*>(svs.get());
@@ -2636,7 +2647,7 @@ std::unique_ptr<Index> read_index_up(IOReader* f, int io_flags) {
         READ1(svs_ivf->k_reorder);
         READ1(svs_ivf->num_threads);
         READ1(svs_ivf->intra_query_threads);
-        READ1(svs_ivf->storage_kind);
+        svs_ivf->storage_kind = read_svs_storage_kind(f);
         READ1(svs_ivf->is_static);
         if (h == fourcc("ISIL")) {
             auto* leanvec = dynamic_cast<IndexSVSIVFLeanVec*>(svs_ivf.get());

tests/test_read_index_deserialize.cpp

@@ -3908,11 +3908,12 @@ TEST(ReadIndexDeserialize,
 
 #include <faiss/svs/IndexSVSVamana.h>
 
-// An invalid storage_kind value should be rejected at deserialization time
-// with a FaissException, not abort via FAISS_ASSERT in to_svs_storage_kind().
-TEST(ReadIndexDeserialize, SVSVamanaInvalidStorageKind) {
-    std::vector<uint8_t> buf;
-    push_fourcc(buf, "ISVD");
+namespace {
+
+/// Append the IndexSVSVamana on-disk fields up to and including storage_kind.
+/// Caller may then push initialized=true (to trigger the SVS deserialize_impl
+/// path) or any other trailing fields needed to reach the validation site.
+void push_svs_vamana_prefix(std::vector<uint8_t>& buf, int storage_kind) {
     push_index_header(buf, 8, 0);
     push_val<size_t>(buf, 32);  // graph_max_degree
     push_val<float>(buf, 1.2f); // alpha
@@ -3922,10 +3923,85 @@ TEST(ReadIndexDeserialize, SVSVamanaInvalidStorageKind) {
     push_val<size_t>(buf, 750); // max_candidate_pool_size
     push_val<size_t>(buf, 28);  // prune_to
     push_val<bool>(buf, false); // use_full_search_history
-    push_val<int>(
-            buf,
-            static_cast<int>(SVS_count)); // storage_kind — first invalid value
-    push_val<bool>(buf, true);            // initialized
+    push_val<int>(buf, storage_kind);
+}
+
+/// Append the IndexSVSIVF on-disk fields up to and including storage_kind.
+/// Used to exercise the validation guard at the ISIQ/ISIL/ISID read sites.
+void push_svs_ivf_prefix(std::vector<uint8_t>& buf, int storage_kind) {
+    push_index_header(buf, 8, 0);
+    push_val<size_t>(buf, 8);   // num_centroids
+    push_val<size_t>(buf, 64);  // minibatch_size
+    push_val<size_t>(buf, 1);   // num_iterations
+    push_val<bool>(buf, false); // is_hierarchical
+    push_val<float>(buf, 0.1f); // training_fraction
+    push_val<size_t>(buf, 0);   // hierarchical_level1_clusters
+    push_val<size_t>(buf, 0);   // seed
+    push_val<size_t>(buf, 1);   // n_probes
+    push_val<float>(buf, 1.0f); // k_reorder (float, not size_t)
+    push_val<size_t>(buf, 1);   // num_threads
+    push_val<size_t>(buf, 1);   // intra_query_threads
+    push_val<int>(buf, storage_kind);
+}
+
+} // namespace
+
+// An invalid storage_kind value should be rejected at deserialization time
+// with a FaissException, not abort via FAISS_ASSERT in to_svs_storage_kind().
+TEST(ReadIndexDeserialize, SVSVamanaInvalidStorageKind) {
+    std::vector<uint8_t> buf;
+    push_fourcc(buf, "ISVD");
+    push_svs_vamana_prefix(buf, /*storage_kind=*/static_cast<int>(SVS_count));
+    push_val<bool>(buf, true); // initialized
+
+    expect_read_throws_with(buf, "storage_kind");
+}
+
+// The Vamana validator must reject negative storage_kind values too — the
+// shared helper takes a signed int because READ1 reads four bytes that could
+// be either sign.
+TEST(ReadIndexDeserialize, SVSVamanaNegativeStorageKind) {
+    std::vector<uint8_t> buf;
+    push_fourcc(buf, "ISVD");
+    push_svs_vamana_prefix(buf, /*storage_kind=*/-1);
+    push_val<bool>(buf, true);
+
+    expect_read_throws_with(buf, "storage_kind");
+}
+
+// IVF Vamana flavours (ISIQ = IndexSVSIVFLVQ, ISIL = IndexSVSIVFLeanVec,
+// ISID = IndexSVSIVF) all share the same storage_kind read site and must
+// reject out-of-range values at the deserialization boundary, mirroring the
+// Vamana branch above. Without the shared validator the bad value would
+// propagate into IndexSVSIVF::deserialize_impl and only get rejected from
+// to_svs_storage_kind() after several allocations and an SVS-runtime call.
+TEST(ReadIndexDeserialize, SVSIVFInvalidStorageKind) {
+    std::vector<uint8_t> buf;
+    push_fourcc(buf, "ISID");
+    push_svs_ivf_prefix(buf, /*storage_kind=*/static_cast<int>(SVS_count));
+    push_val<bool>(buf, true); // is_static
+    push_val<bool>(buf, true); // initialized
+
+    expect_read_throws_with(buf, "storage_kind");
+}
+
+TEST(ReadIndexDeserialize, SVSIVFLVQInvalidStorageKind) {
+    std::vector<uint8_t> buf;
+    push_fourcc(buf, "ISIQ");
+    push_svs_ivf_prefix(buf, /*storage_kind=*/-1);
+    push_val<bool>(buf, true);
+    push_val<bool>(buf, true);
+
+    expect_read_throws_with(buf, "storage_kind");
+}
+
+TEST(ReadIndexDeserialize, SVSIVFLeanVecInvalidStorageKind) {
+    std::vector<uint8_t> buf;
+    push_fourcc(buf, "ISIL");
+    push_svs_ivf_prefix(buf, /*storage_kind=*/static_cast<int>(SVS_count) + 7);
+    push_val<bool>(buf, true); // is_static
+    push_val<size_t>(buf, 8);  // leanvec_d (only on ISIL path)
+    push_val<bool>(buf, true); // initialized
 
     expect_read_throws_with(buf, "storage_kind");
 }