mirror of
https://github.com/facebookresearch/faiss.git
synced 2026-10-12 02:13:36 +00:00
Fix null dereference, dead condition, and Python binding crash in AutoTune and Clustering (#5321)
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<double>::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
This commit is contained in:
committed by
meta-codesync[bot]
parent
6653b98402
commit
376930beab
@@ -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);
|
||||
}
|
||||
|
||||
@@ -367,7 +367,7 @@ void Clustering::train_encoded(
|
||||
? std::numeric_limits<double>::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<size_t>(std::numeric_limits<int>::max()),
|
||||
"kmeans_clustering: d=%zu exceeds INT_MAX",
|
||||
d);
|
||||
FAISS_THROW_IF_NOT_FMT(
|
||||
k <= static_cast<size_t>(std::numeric_limits<int>::max()),
|
||||
"kmeans_clustering: k=%zu exceeds INT_MAX",
|
||||
k);
|
||||
Clustering clus(static_cast<int>(d), static_cast<int>(k));
|
||||
clus.verbose = d * n * k > (size_t(1) << 30);
|
||||
// display logs if > 1Gflop per iteration
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user