2 Commits
Author SHA1 Message Date
Alibek Zhakubayev 796977a1ab Fix IndexBinaryMultiHash::reset() not clearing hash maps and avoid unnecessary copies (#5100)
Summary:
Pull Request resolved: https://github.com/facebookresearch/faiss/pull/5100

Fix two issues in `IndexBinaryMultiHash` related to incorrect use of range-based for loops over the `maps` member (`std::vector<std::unordered_map<idx_t, std::vector<idx_t>>>`).

## Bug 1: `reset()` does not actually clear the hash maps (correctness)

In `IndexBinaryMultiHash::reset()`, the loop `for (auto map : maps)` iterates by **value**, copying each `unordered_map` into a local variable. `map.clear()` clears the copy; the original maps in the vector are never modified.

This is a direct consequence of C++ range-based for semantics ([stmt.ranged]): `auto map` deduces to `Map` (value type), and copy-initialization creates an independent object. Mutations to `map` do not affect the container element.

**Proof:**
```cpp
std::vector<std::unordered_map<int, int>> v(1);
v[0][42] = 1;
for (auto m : v) { m.clear(); }
assert(v[0].size() == 1);  // original untouched

for (auto& m : v) { m.clear(); }
assert(v[0].size() == 0);  // original cleared
```

**Impact:** After `reset()` + `add()`, stale hash map entries still point to old vector IDs. Search returns phantom results from pre-reset data — silent data corruption.

**Fix:** `for (auto map : maps)` → `for (auto& map : maps)`.

## Bug 2: `hashtable_size()` copies all hash maps unnecessarily (performance)

Same `for (auto map : maps)` pattern copies each entire `unordered_map` (with all buckets and stored `vector<idx_t>` values) just to call `.size()`. O(total entries) of needless allocation per call.

**Fix:** `for (auto map : maps)` → `for (const auto& map : maps)`. `const` is correct since `hashtable_size()` is a `const` method.

## Test

Added `MultiHashResetClearsMaps` unit test with three phases:

1. **Pre-reset**: Add a vector, assert `hashtable_size() > 0` (precondition — maps are populated).
2. **Reset verification**: Call `reset()`, assert `ntotal == 0` and `hashtable_size() == 0`. Then search for the old vector, assert `labels[0] == -1` (not found).
3. **Post-reset smoke test**: Add a different vector, search for it, assert exact match at ID 0 — verifies the index is fully functional after reset.

Reviewed By: limqiying

Differential Revision: D100797480

fbshipit-source-id: 7313d103371a8442c00bbff5d0c60b6bead8e7c6
2026-04-14 23:35:31 -07:00
Justin Gibbs ec30fb387c Use BitstringReader in IndexBinaryHash to avoid OOB reads (#4961)
Summary:
Pull Request resolved: https://github.com/facebookresearch/faiss/pull/4961

Replace unsafe *(uint64_t*) casts with BitstringReader in all 4 hash
computation sites in IndexBinaryHash and IndexBinaryMultiHash. The old
casts read 8 bytes regardless of code_size, causing out-of-bounds reads
for small vectors (e.g., code_size=2 for d=16).

BitstringReader reads only the needed bits byte-by-byte.

Reviewed By: mnorris11

Differential Revision: D97317369

fbshipit-source-id: 07436583d15bef8546afe4e4c2d19586d93e6d37
2026-03-19 19:31:13 -07:00