From 376930beabd2c0e24eb86f4e1209f6594f750b9d Mon Sep 17 00:00:00 2001 From: Alibek Zhakubayev Date: Thu, 18 Jun 2026 16:46:43 -0700 Subject: [PATCH] Fix null dereference, dead condition, and Python binding crash in AutoTune and Clustering (#5321) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Pull Request resolved: https://github.com/facebookresearch/faiss/pull/5321 ### THIS DIFF Four correctness fixes found during Core Utilities code review. **AutoTune.cpp — null dereference in `set_groundtruth`** `set_groundtruth` guarded `gt_D_in` with an explicit null check ("allow null for this, as it is often not used") but left `gt_I_in` unguarded. Passing null `gt_I_in` crashed in `memcpy(gt_I.data(), nullptr, ...)`. Added `FAISS_THROW_IF_NOT_MSG` before the copy to convert the crash to a clean `FaissException`. **class_wrappers.py — ValueError crash when passing non-None gt_D** `replacement_set_groundtruth` used `swig_ptr(D) if D else None`. For any non-None numpy array, `bool(D)` raises `ValueError: The truth value of an array with more than one element is ambiguous`. This made it impossible to call `criterion.set_groundtruth(D, I)` with a non-None distance matrix. Fixed to `if D is not None`. **Clustering.cpp — dead `change >= 0` sub-expression in early-stop check** `change` is either `std::numeric_limits::max()` or `std::abs(prev_obj - obj) / std::abs(prev_obj)` — both always ≥ 0. The `change >= 0` sub-expression in `if (change >= 0 && change <= early_stop_threshold)` never filtered anything. Removed it. **Clustering.cpp — unchecked `size_t → int` cast in `kmeans_clustering`** `kmeans_clustering` accepts `size_t d` and `size_t k` then casts both to `int` without bounds checking. Values exceeding `INT_MAX` silently wrap to negative and produce garbage in all downstream `memcpy` and index operations. Added `FAISS_THROW_IF_NOT_FMT` guards before the cast. ### CONTEXT Thursday Core Utilities rotation. Issues found by reading AutoTune.cpp and Clustering.cpp during Phase 1 discovery. The `class_wrappers.py` bug was discovered while writing tests for the AutoTune fix. Reviewed By: limqiying Differential Revision: D108407514 fbshipit-source-id: 76d8c143071f98aee864251e592ad62b5dc8aab2 --- faiss/AutoTune.cpp | 2 ++ faiss/Clustering.cpp | 10 +++++++++- faiss/python/class_wrappers.py | 2 +- tests/test_autotune.py | 24 ++++++++++++++++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/faiss/AutoTune.cpp b/faiss/AutoTune.cpp index 216d88c05..c1017c56b 100644 --- a/faiss/AutoTune.cpp +++ b/faiss/AutoTune.cpp @@ -45,6 +45,8 @@ void AutoTuneCriterion::set_groundtruth( gt_D.resize(nq * gt_nnn_in); memcpy(gt_D.data(), gt_D_in, sizeof(gt_D[0]) * nq * gt_nnn_in); } + FAISS_THROW_IF_NOT_MSG( + gt_I_in != nullptr, "set_groundtruth: gt_I must be non-null"); gt_I.resize(nq * gt_nnn_in); memcpy(gt_I.data(), gt_I_in, sizeof(gt_I[0]) * nq * gt_nnn_in); } diff --git a/faiss/Clustering.cpp b/faiss/Clustering.cpp index d0137ee30..b82b1be40 100644 --- a/faiss/Clustering.cpp +++ b/faiss/Clustering.cpp @@ -367,7 +367,7 @@ void Clustering::train_encoded( ? std::numeric_limits::max() : std::abs(prev_obj - obj) / std::abs(prev_obj); - if (change >= 0 && change <= early_stop_threshold) { + if (change <= early_stop_threshold) { if (verbose) { printf("\n Converged at iteration %d: " "objective did not change\n", @@ -439,6 +439,14 @@ float kmeans_clustering( size_t k, const float* x, float* centroids) { + FAISS_THROW_IF_NOT_FMT( + d <= static_cast(std::numeric_limits::max()), + "kmeans_clustering: d=%zu exceeds INT_MAX", + d); + FAISS_THROW_IF_NOT_FMT( + k <= static_cast(std::numeric_limits::max()), + "kmeans_clustering: k=%zu exceeds INT_MAX", + k); Clustering clus(static_cast(d), static_cast(k)); clus.verbose = d * n * k > (size_t(1) << 30); // display logs if > 1Gflop per iteration diff --git a/faiss/python/class_wrappers.py b/faiss/python/class_wrappers.py index f35b719d4..2acbbd8ba 100644 --- a/faiss/python/class_wrappers.py +++ b/faiss/python/class_wrappers.py @@ -1171,7 +1171,7 @@ def handle_AutoTuneCriterion(the_class): assert I.shape == D.shape self.nq, self.gt_nnn = I.shape self.set_groundtruth_c( - self.gt_nnn, swig_ptr(D) if D else None, swig_ptr(I)) + self.gt_nnn, swig_ptr(D) if D is not None else None, swig_ptr(I)) def replacement_evaluate(self, D, I): assert I.shape == D.shape diff --git a/tests/test_autotune.py b/tests/test_autotune.py index 9cb830aa7..a3b6eee6c 100644 --- a/tests/test_autotune.py +++ b/tests/test_autotune.py @@ -5,6 +5,7 @@ import unittest +import numpy as np import faiss from common_faiss_tests import for_all_simd_levels @@ -48,3 +49,26 @@ class TestParameterSpace(unittest.TestCase): ps.set_index_parameter(index, "quantizer_efSearch", 5) index2 = faiss.downcast_index(index.quantizer) self.assertEqual(index2.hnsw.efSearch, 5) + + +class TestAutoTuneCriterion(unittest.TestCase): + + def test_set_groundtruth_with_distances(self): + nq, gt_nnn = 10, 5 + crit = faiss.OneRecallAtRCriterion(nq, gt_nnn) + rng = np.random.default_rng(42) + gt_I = rng.integers(0, 100, size=(nq, gt_nnn)).astype("int64") + gt_D = rng.random(size=(nq, gt_nnn)).astype("float32") + crit.set_groundtruth(gt_D, gt_I) + self.assertEqual(crit.gt_nnn, gt_nnn) + self.assertEqual(crit.gt_I.size(), nq * gt_nnn) + + def test_set_groundtruth_null_distances(self): + # gt_D is documented as optional (None skips the distances copy). + nq, gt_nnn = 10, 5 + crit = faiss.OneRecallAtRCriterion(nq, gt_nnn) + rng = np.random.default_rng(42) + gt_I = rng.integers(0, 100, size=(nq, gt_nnn)).astype("int64") + crit.set_groundtruth(None, gt_I) + self.assertEqual(crit.gt_nnn, gt_nnn) + self.assertEqual(crit.gt_I.size(), nq * gt_nnn)