Commit 8d3cc92
Fix clone_index null return for IndexRowwiseMinMax (#5220)
Summary:
Pull Request resolved: #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: ed29db91b51716133ef8ceb426bc92760a222fef1 parent ff1d543 commit 8d3cc92
2 files changed
Lines changed: 20 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
377 | 377 | | |
378 | 378 | | |
379 | 379 | | |
| 380 | + | |
380 | 381 | | |
381 | 382 | | |
382 | 383 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
86 | 86 | | |
87 | 87 | | |
88 | 88 | | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
0 commit comments