Bound file-controlled node index against _n_nodes in the query tree walk (OOB read on crafted .ann)

[professor-moody]

Summary

Loading a crafted .ann with AnnoyIndex::load and then querying it (get_nns_by_vector / get_nns_by_item) reads out of bounds. _get(i) computes _nodes + _s*i with no bound on i, and _get_all_nns pushes a split node's children[0]/children[1] (read verbatim from the index file) back onto the queue, then dereferences them on the next hop with no check that they are within [0, _n_nodes).

Node* nd = _get(i);
q.pop();
if (nd->n_descendants == 1 && i < _n_items) { ... }
else if (nd->n_descendants <= _K) { ... }
else {
  q.push(make_pair(..., static_cast<S>(nd->children[1])));   // unbounded
  q.push(make_pair(..., static_cast<S>(nd->children[0])));   // unbounded
}

load() sets _n_nodes = size/_s but validates no per-node content, so children[0/1] (and the leaf-array item ids) can be any value in the file. This matches the // TODO: handle OOB already noted in get_nns_by_item.

Reproduction

A 2-node .ann (f=2) whose root is a split node (n_descendants > _K) with children[0] out of range (e.g. 2, 50, or 0x40000000), loaded and queried, under AddressSanitizer:

AddressSanitizer: heap-buffer-overflow READ of size 4
  #0 AnnoyIndex::_get_all_nns   annoylib.h (nd->n_descendants)
  #1 AnnoyIndex::get_nns_by_vector

An adjacent index reads a few bytes past the node array; a far index segfaults. A legitimate index queries cleanly.

Fix

Bound the node index against _n_nodes before it is dereferenced, right after the queue pop. Validated with AddressSanitizer: with this guard, both an adjacent and a far out-of-range child index are handled cleanly with no OOB, and legitimate indexes still query correctly.

The same [0, _n_nodes) bound would also cover the leaf-array _get(j) and the get_nns_by_item entry _get(item) (the existing TODO); this PR does the minimal fix in the tree walk. Found with a structure-aware model/index parser fuzzer.

[professor-moody]

I kept testing this against a wider set of tampered indexes and found that my first commit did not actually close the bug. Pushed a second commit, and I want to correct two things in the original description as well.

The first commit was incomplete. It only bounds indices that travel through the priority queue. A leaf node's children[] array is copied verbatim into nns:

} else if (nd->n_descendants <= _K) {
  const S* dst = nd->children;
  nns.insert(nns.end(), dst, &dst[nd->n_descendants]);

and those values are then dereferenced in the dedup loop, which the guard never sees. Sweeping single 4 byte edits across every leaf-array node of a stock 200 item, 4 tree index, the patched build still faults:

AddressSanitizer: BUS on unknown address, READ
  #0 _get_all_nns       annoylib.h:1493
  #1 get_nns_by_vector  annoylib.h:1235

The second commit bounds that site, moves the existing guard above _get(i) so the index is checked before it becomes a pointer rather than after, rejects a negative n_descendants before it is used as a copy length, and bounds the item index in get_nns_by_item, which had a // TODO: handle OOB for exactly this.

Correction to the sanitizer output I quoted. I wrote heap-buffer-overflow in the original report. That is not what you will see. load() mmaps the index, so _nodes is a file mapping and ASan does not track it: a far out-of-range index faults as BUS or SEGV, and my quoted signature came from a harness that backed the nodes with a malloc'd buffer. My mistake for not carrying that caveat over.

Worth knowing because it affects how you reproduce it: an out-of-range index that still lands inside the last mapped page does not fault at all. The branch is silently dropped and the query quietly returns incomplete results. Whether a given crafted index crashes also depends on address layout, so it is not reliably reproducible on a single run. That is an argument for the bound rather than against it.

Not a behaviour change for real indexes. Results are byte identical to unpatched annoy across 108 legitimate configurations: dimensions 2 to 128, 1 to 1000 items, 1 to 10 trees, both get_nns_by_vector and get_nns_by_item, including the single item and two item cases where empty leaves show up. Zero is deliberately still accepted as a leaf length.