From 8d3cc92536e340e210f05c8265b8523e1efd1168 Mon Sep 17 00:00:00 2001
From: Alibek Zhakubayev <alibek@meta.com>
Date: Mon, 18 May 2026 14:35:48 -0700
Subject: [PATCH] Fix clone_index null return for IndexRowwiseMinMax (#5220)
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/5220

## TL;DR

`clone_index` returns `nullptr` for any `IndexRowwiseMinMax` or `IndexRowwiseMinMaxFP16` instance — a silent failure that leaks the allocated clone and its deep-copied inner index. We add the missing `return res;` and a regression test covering both subtypes.

## Bug

`clone_Index` in `clone_index.cpp` is a dispatch table of `else if` branches, one per index type. Every branch ends with `return res;` except one: the `IndexRowwiseMinMaxBase` branch allocates `res`, sets `own_fields = true`, deep-copies the inner index into `res->index`, and then falls through to the bottom of the function. The fallthrough hits `return nullptr;`.

The result: callers receive `nullptr` with no exception or warning, and the `res` pointer plus its inner clone are leaked.

## Impact

`clone_index` has 15+ callsites across the faiss codebase and is part of the public C++ and Python API. None of the callers we surveyed guard against `nullptr` — the contract is "clone returns a usable index or throws." With this bug:

- Python users see `clone_index(codec)` return `None`. The next attribute access on the result raises `AttributeError`, often deep in unrelated code where the connection to the clone call is not obvious.
- C++ users dereference the null pointer on the next method call, producing a segfault with no stack frame pointing at `clone_index`.
- Memory leaks: every failed clone leaks `res` (the `IndexRowwiseMinMax` shell) and `res->index` (the inner `IndexScalarQuantizer` deep copy).

The bug affects any pipeline that wraps a codec with `MinMax,*` or `MinMaxFP16,*` factory strings and then clones it — common when cloning a trained codec for parallel encoding, snapshot persistence, or sharded deployment.

## Fix

One line added:

```
} else if (
        const IndexRowwiseMinMaxBase* irmmb =
                dynamic_cast<const IndexRowwiseMinMaxBase*>(index)) {
    IndexRowwiseMinMaxBase* res = clone_IndexRowwiseMinMax(irmmb);
    res->own_fields = true;
    res->index = clone_Index(irmmb->index);
    return res;  // added
}
```

The fix restores symmetry with every other branch in the function. No new abstractions, no related cleanup — the missing return is purely an oversight from when the branch was added.

The `clone_IndexRowwiseMinMax` helper dispatches via `TRYCLONE` to both `IndexRowwiseMinMaxFP16` and `IndexRowwiseMinMax`, so both concrete subtypes are restored by this one line.

Reviewed By: mnorris11

Differential Revision: D105157714

fbshipit-source-id: ed29db91b51716133ef8ceb426bc92760a222fef
---
 faiss/clone_index.cpp |  1 +
 tests/test_clone.py   | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/faiss/clone_index.cpp b/faiss/clone_index.cpp
index 8eb035f6b..4b3387cc0 100644
--- a/faiss/clone_index.cpp
+++ b/faiss/clone_index.cpp
@@ -377,6 +377,7 @@ Index* Cloner::clone_Index(const Index* index) {
         IndexRowwiseMinMaxBase* res = clone_IndexRowwiseMinMax(irmmb);
         res->own_fields = true;
         res->index = clone_Index(irmmb->index);
+        return res;
     } else if (
             dynamic_cast<const IndexAdditiveQuantizerFastScan*>(index) ||
             dynamic_cast<const IndexAdditiveQuantizer*>(index) ||
diff --git a/tests/test_clone.py b/tests/test_clone.py
index 7ed047d54..06f71f87e 100644
--- a/tests/test_clone.py
+++ b/tests/test_clone.py
@@ -86,3 +86,22 @@ class TestClone(unittest.TestCase):
         self.do_test_clone("RQ3x4fs_32_Nlsq2x4")
         self.do_test_clone("PLSQ2x3x4fs_Nlsq2x4")
         self.do_test_clone("PRQ2x3x4fs_Nrq2x4")
+
+    def test_RowwiseMinMax(self):
+        # clone_Index for IndexRowwiseMinMaxBase was missing return res, so
+        # clone_index returned nullptr and leaked the clone allocation plus
+        # the deep-copied inner index.
+        d = 32
+        n = 200
+        rng = np.random.default_rng(42)
+        x = rng.standard_normal((n, d)).astype(np.float32)
+
+        for factory in ("MinMax,SQ8", "MinMaxFP16,SQ8"):
+            codec = faiss.index_factory(d, factory)
+            codec.train(x)
+            codes = codec.sa_encode(x)
+
+            codec2 = faiss.clone_index(codec)
+            self.assertEqual(type(codec), type(codec2))
+            codes2 = codec2.sa_encode(x)
+            np.testing.assert_array_equal(codes, codes2)