[jac0626]

Change Type

• Bug fix (cherry-pick to release 0.18)

Linked Issue

• Fixes: [bug](ivf): KnnSearch with topk<10 overruns non-fixed MemmoveHeap buffer #2298

Note

Cherry-pick of #2299 (target main) onto the 0.18 release branch. The
buggy code is identical on 0.18; only a trivial include-block context
differed in the test file and was resolved. Verified on the 0.18 tree
(all topk 1-9 correct, no overflow under AddressSanitizer).

What Changed

The non-fixed MemmoveHeap (MemmoveHeap<max_heap, false>) ran
std::upper_bound over the entire ordered_buffer_ (begin()..end()).
Because Pop() only decrements cur_size_ without shrinking ordered_buffer_,
a stale tail accumulates, so the insertion index pos could exceed cur_size_
and make memmove(..., (cur_size_ - pos) * sizeof(DistanceRecord)) underflow to
a huge size_t, overrunning the heap buffer.

DistanceHeap::MakeInstanceBySize selects MemmoveHeap only when
max_size < 10, and IVF::search requests the non-fixed variant
(MakeInstanceBySize<true, false>(topk)) then interleaves Push with a bounded
Pop. So any KnnSearch with topk < 10 reaches the buggy path; whether it
overruns is data-dependent. The fixed-size branch was already correct (it bounds
upper_bound at begin() + cur_size_).

• Bound the non-fixed branch's upper_bound at begin() + cur_size_, matching
  the already-correct fixed-size branch.
• Reuse the stale tail slot instead of emplace_back when
  ordered_buffer_.size() > cur_size_, which also bounds the previously
  unbounded buffer growth.
• Tests: a regression test (memmove_heap non-fixed Pop-then-Push (topk<10) in
  src/impl/heap/distance_heap_test.cpp) replaying IVF's Push/Pop
  interleaving across topk 1–9 with a triggering seed, validated against a
  brute-force top-k.

Test Evidence

• make fmt
• make lint
• make test
• make cov, run tests, and collect coverage
• Other (describe below)

Test details:

# Red -> Green (normal Debug build, ENABLE_ASAN=OFF):
#   before fix: ./build/tests/unittests "memmove_heap non-fixed Pop-then-Push (topk<10)"
#     => wrong results (0.0f leaked into top-5) + free(): invalid pointer -> SIGABRT (exit 134)
#   after fix:
./build/tests/unittests "memmove_heap non-fixed Pop-then-Push (topk<10)"
  => All tests passed (18 assertions in 1 test case)

# Regression (no behavior change on existing heap paths):
./build/tests/unittests "[distance_heap]"   => All tests passed (8106 assertions in 3 test cases)
./build/tests/unittests "[IVFNearestPartition][ut],[IVFParameter][ut]"
  => All tests passed (3035 assertions in 4 test cases)

# End-to-end: 200 real IVF KnnSearch queries at topk=5 against a rebuilt libvsag.so
#   => "OK: 200 topk=5 queries returned 5 results each, no crash"

# Cross-checked against a brute-force top-k over 4500 cases (500 seeds x topk 1-9):
#   no overflow, results match, worst-case peak buffer memory ~1.5 MB -> 96 bytes.

# clang-format-15 / clang-tidy-15: clean on memmove_heap.cpp. The test file is
# excluded from `make lint` by the repo's `(?<!_test)` source filter.
# Note: the full `make test` suite was not run end-to-end here; the targeted
# heap + IVF unit runs and the end-to-end IVF check above cover the changed paths.

Compatibility Impact

• API/ABI compatibility: none (no public header changes; the change is internal
  to src/impl/heap/).
• Behavior changes: KnnSearch with topk < 10 now returns correct top-k
  results without memory corruption, matching topk >= 10 (StandardHeap).

Performance and Concurrency Impact

• Performance impact: negligible — the non-fixed Push now searches a tighter
  range ([begin, begin + cur_size_)) and reuses slots, reducing reallocations.
• Concurrency/thread-safety impact: none — each IVF search thread owns its heap.

Documentation Impact

• No docs update needed

Risk and Rollback

• Risk level: low — change is localized to the non-fixed MemmoveHeap::Push
  branch; the fixed-size branch and StandardHeap are untouched.
• Rollback plan: revert the commit; no data format or API changes are involved.

Checklist

• I have linked the relevant issue
• I have added/updated tests for new behavior or bug fixes
• I have considered API compatibility impact
• I have updated docs if behavior/workflow changed (n/a)
• My commit messages follow project conventions

[mergify]

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Require linked issue for feature/bug PRs

• body~=(?im)(?:^|[\s\-\*])(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s+(?:#\d+|[\w.\-]+/[\w.\-]+#\d+|https?://github\.com/[\w.\-]+/[\w.\-]+/issues/\d+)

[gemini-code-assist]

Code Review

This pull request addresses a buffer overflow issue in MemmoveHeap when topk < 10 by ensuring that the non-fixed heap's Push operation only searches up to cur_size_ and reuses stale tail slots instead of always appending. A regression test has been added to verify this behavior. The reviewer suggested a more robust approach to handle the buffer size check using resize instead of conditional emplace_back to prevent potential out-of-bounds writes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             0.18    #2300      +/-   ##
==========================================
+ Coverage   91.40%   91.77%   +0.36%     
==========================================
  Files         335      335              
  Lines       20112    20114       +2     
==========================================
+ Hits        18384    18460      +76     
+ Misses       1728     1654      -74     

Flag	Coverage Δ
cpp	91.77% <100.00%> (+0.36%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
common	86.03% <ø> (ø)
datacell	93.26% <ø> (+0.21%)	⬆️
index	91.80% <ø> (+0.29%)	⬆️
simd	100.00% <ø> (ø)

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e64ea0...3d4578e. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @inabao
/request-review @ShawnShawnYou

[LHT129]

LGTM

[LHT129]

Thank you for this well-crafted fix. The root cause analysis is thorough and the fix is surgically precise:

• memmove_heap.cpp: Bounding std::upper_bound at begin() + cur_size_ (matching the already-correct fixed-size branch) and replacing emplace_back with conditional resize(cur_size_ + 1) are exactly the right changes. The stale-tail reuse via resize is a nice bonus that prevents unbounded buffer growth.

• distance_heap_test.cpp: The regression test faithfully replays the IVF Push/Pop interleaving pattern across topk 1–9 with a known triggering seed, and cross-validates against brute-force top-k. This is exactly the kind of targeted regression test a bug like this needs.

No blocking issues found. The existing review comments about the emplace_back → resize transition have been addressed in the current diff.

[LHT129]

LGTM. The fix correctly addresses the buffer overflow in the non-fixed MemmoveHeap::Push path:

1. upper_bound range: bounded from end() to begin() + cur_size_, matching the fixed-size branch and preventing stale-tail insertion index from exceeding cur_size_.
2. Buffer growth: resize(cur_size_ + 1) replaces emplace_back, reusing stale tail slots and bounding buffer growth.
3. memmove size: changed from max_size_ - pos to cur_size_ - pos, eliminating the underflow risk when pos > cur_size_.

The regression test exercises the exact IVF Push/Pop interleaving pattern across topk 1-9 and validates against brute-force top-k — good coverage of the buggy path.

[LHT129]

I have reviewed this PR thoroughly. The fix is correct and well-targeted:

Core fix (src/impl/heap/memmove_heap.cpp):

• Bounding std::upper_bound at begin() + cur_size_ instead of end() prevents the stale tail from producing a pos > cur_size_, which would cause (cur_size_ - pos) to underflow to a huge size_t in the memmove call.
• Replacing unconditional emplace_back with resize(cur_size_ + 1) when the buffer is too small bounds growth and reuses stale tail slots. This matches the suggestion from the earlier bot review and is the correct approach.

Test (src/impl/heap/distance_heap_test.cpp):

• The regression test faithfully replays the IVF Push/Pop interleaving pattern across topk 1–9, which is exactly the scenario that triggers the bug. Validating against a brute-force top-k is the right way to verify correctness.
• The test uses seed=0 which the PR description confirms triggers the overflow within a few dozen iterations — good choice for deterministic reproduction.

No concerns found. The change is minimal, localized, and has no API/ABI impact. The existing bot review comment has been addressed in the current diff.

[LHT129]

I have reviewed this cherry-pick of #2299 to the 0.18 release branch. The fix is identical in logic to the already-merged mainline PR.

Core fix (src/impl/heap/memmove_heap.cpp):

• Bounding std::upper_bound at begin() + cur_size_ instead of end() prevents the stale tail from producing a pos > cur_size_, which would cause (cur_size_ - pos) to underflow to a huge size_t and overrun the heap buffer.
• Replacing unconditional emplace_back with conditional resize(cur_size_ + 1) bounds the previously unbounded buffer growth and reuses stale tail slots.

Test (src/impl/heap/distance_heap_test.cpp):

• The regression test correctly replays the IVF Push/Pop interleaving pattern for topk 1-9 with a triggering seed (0), and validates the retained records against a brute-force top-k.
• The test only covers MemmoveHeap<true, false> (max-heap, non-fixed), which is the variant used by IVF. The MemmoveHeap<false, false> path has the same bug but is not exercised by any production code path; this is acceptable for a release-branch cherry-pick.

No issues found. The fix is correct and well-tested.

[LHT129]

This is a clean, well-focused cherry-pick of #2299 to the 0.18 release branch. The fix is minimal and correct, and the regression test thoroughly covers the affected code path.

Fix (src/impl/heap/memmove_heap.cpp):

• std::upper_bound is now correctly bounded to [begin, begin + cur_size_) instead of searching the stale tail, which was the root cause of the overflow.
• The resize(cur_size_ + 1) guard replaces emplace_back, ensuring the buffer always has enough storage for the subsequent memmove and element assignment — this is more robust and addresses the earlier review suggestion.
• The memmove size (cur_size_ - pos) is now guaranteed non-negative since pos is bounded by cur_size_.

Test (src/impl/heap/distance_heap_test.cpp):

• The regression test correctly mirrors IVF's actual Push/Pop interleaving pattern across all topk 1–9.
• Validates results against a brute-force sorted reference, which is the right approach.

No issues found. The changes are correct, well-tested, and appropriate for a release-branch cherry-pick.

[LHT129]

The fix is clean, minimal, and well-tested. The two changes in memmove_heap.cpp are correct:

1. upper_bound range bounded to begin() + cur_size_ — prevents searching the stale tail that accumulates after Pop(), which was the root cause of the buffer overflow.

2. resize(cur_size_ + 1) guard replacing emplace_back — ensures the buffer has enough capacity before the memmove without unbounded growth from repeated emplace_back.

The memmove size (cur_size_ - pos) is now safe because pos is guaranteed to be in [0, cur_size_] by the bounded upper_bound.

The regression test in distance_heap_test.cpp correctly mirrors the IVF Push/Pop interleaving pattern across topk 1–9 and validates against a brute-force top-k, which is exactly the scenario that triggers the bug.

No issues found. This is a solid cherry-pick to the 0.18 release branch.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[mergify]

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Require linked issue for feature/bug PRs

• body~=(?im)(?:^|[\s\-\*])(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s+(?:#\d+|[\w.\-]+/[\w.\-]+#\d+|https?://github\.com/[\w.\-]+/[\w.\-]+/issues/\d+)