Add boundary checks in gen_associated_data_from_range() - #847
Conversation
Co-authored-by: harsha-simhadri <5590673+harsha-simhadri@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens diskann-tools’s gen_associated_data_from_range() against invalid user-supplied CLI ranges by validating end >= start and using checked arithmetic to prevent u32 underflow/overflow when computing the count written into the DiskANN binary header.
Changes:
- Added early input validation returning
CMDToolErrorwhenend < start. - Replaced
end - start + 1with checked arithmetic to detect thestart=0, end=u32::MAXoverflow case. - Added unit tests covering
end < startand maximum-range overflow.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Aditya Krishnan (arkrishn94)
left a comment
There was a problem hiding this comment.
Looks fine Alex, happy to stamp after we resolve the two small comments.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #847 +/- ##
==========================================
+ Coverage 89.10% 89.51% +0.41%
==========================================
Files 443 460 +17
Lines 83361 85454 +2093
==========================================
+ Hits 74277 76497 +2220
+ Misses 9084 8957 -127
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
# DiskANN v0.52.0 Release Notes ## Breaking Changes An AI generated, human reviewed list of changes is summarized below. ### `get_degree_stats` signature changed ([#998](#998)) `DiskANNIndex::get_degree_stats` now takes an explicit iterator of IDs instead of requiring the data provider to implement `IntoIterator`. ```rust // Before — provider had to impl IntoIterator index.get_degree_stats(&mut accessor)?; // After — caller supplies the ID iterator index.get_degree_stats(&mut accessor, id_iter)?; ``` ### PQ dimension contract tightened; entries now `&[f32]` only ([#1044](#1044)) With `AlignedBoxWithSlice` removed from the PQ path, the dimension handling has been refactored into a three-layer contract: | Layer | Where | Contract | |---|---|---| | **Boundary (inmem)** | `QueryComputer::new`, `MultiQueryComputer::new`, `DistanceComputer::evaluate_similarity` | `len == dim` (returns `Err` on mismatch) | | **Boundary (disk)** | `PQScratch::set` | `len >= dim`, slices to `[..dim]` | | **Internal** | `TableL2/IP/Cosine::{new, populate}` | Trusted — no re-validation | **Other changes:** - PQ table populate/distance methods now accept `&[f32]` instead of `<U: Into<f32>>`. Callers must pre-decode quantized vectors via `VectorRepr::as_f32`. - Generic trampoline impls (`&Vec<u8>`, `&&[u8]`) on `QueryComputer` / `DistanceComputer` have been removed. ### `calculate_chunk_offsets` relocated to `ChunkOffsets` constructors ([#976](#976)) The free functions `calculate_chunk_offsets` and `calculate_chunk_offsets_auto` have been moved into constructors on `ChunkOffsets` / `ChunkOffsetsView` in `diskann-quantization::views`. ```rust // Before let offsets = calculate_chunk_offsets(dim, num_chunks); // After (allocating) let offsets = ChunkOffsets::partition(dim, num_chunks)?; // After (zero-alloc, borrows caller-owned scratch) let view = ChunkOffsetsView::partition_into(dim, &mut scratch)?; ``` Additionally, `get_chunk_from_training_data` has been moved from public API. ### `CachingProvider` removed ([#1052](#1052)) The entire `diskann_providers::model::graph::provider::async_::caching` module has been deleted. **Why:** The `CachingProvider` was an experiment in transparent caching over `DataProvider`. In practice it required double monomorphization of the indexing code, didn't save integration work for bulk methods like `on_elements_unordered`/`distances_unordered`, and was complex to maintain. An internal user who …migrated off it removed ~1,000 lines of code, improved compile times by ~20%, and substantially reduced complexity. **Upgrade:** Manage caching directly in your `DataProvider` implementation. ## New Features ### AVX-512 4-bit distance kernels ([#1045](#1045)) Native V4 (AVX-512) specializations for 4-bit packed vector distance computations: - **`SquaredL2`** — 16 × `u32` lanes per iteration via `_mm512_madd_epi16`. - **`InnerProduct`** — AVX-512 VNNI (`_mm512_dpbusd_epi32`) over `u8x64` / `i8x64` operands. Previously, V4 hardware fell back to two AVX2 (V3) kernel invocations per 512-bit chunk. The native kernels double per-instruction throughput. No API changes — existing code benefits automatically on AVX-512 capable hardware. ## Merged PRs * Deprecate 32-bit targets by @suhasjs in #1022 * Add a fast path to `Map::prepare`. by @hildebrandmw in #1023 * Add boundary checks in gen_associated_data_from_range() by @Copilot in #847 * [deps] Don't pull `rayon` as a dependency of `diskann`. by @hildebrandmw in #1024 * Bump openssl from 0.10.78 to 0.10.79 by @dependabot[bot] in #1026 * Cleaning up test work and changing the get_degree_stats signature. by @JordanMaples in #998 * Reduce scalar-quantization benchmark monomorphization by @suri-kumkaran in #1041 * [diskann-vector] Support truly unaligned distances. by @hildebrandmw in #981 * rename spherical.json to graph index with spherical quantization by @harsha-simhadri in #1042 * [PQ Cleanup] Part 2: Consolidate `calculate_chunk_offsets*` by @arkrishn94 in #976 * PQ: tighten dim contract; right-size scratch buffer by @wuw92 in #1044 * Add v4 distance kernels (4-bit SquaredL2 / InnerProduct) by @m3hm3t in #1045 * Remove the Caching Provider by @hildebrandmw in #1052 ## New Contributors * @suhasjs made their first contribution in #1022 * @m3hm3t made their first contribution in #1045 **Full Changelog**: v0.51.0...v0.52.0 Co-authored-by: Mark Hildebrand <mhildebrand@microsoft.com>
`gen_associated_data_from_range()` performed unchecked `u32` arithmetic
on user-supplied CLI values: `end - start` underflows when `end <
start`, and `end - start + 1` overflows when `end == u32::MAX && start
== 0`.
## Changes
- **Input validation**: Return `CMDToolError` early when `end < start`
- **Checked arithmetic**: Replace `end - start + 1` with `(end -
start).checked_add(1)`, returning an error on overflow
- **Tests**: Add
`test_gen_associated_data_from_range_end_less_than_start` and
`test_gen_associated_data_from_range_max_overflow`
```rust
// Before (panics/wraps on bad input)
let num_ints = end - start + 1;
// After
if end < start {
return Err(CMDToolError {
details: format!("invalid range: end ({end}) must be greater than or equal to start ({start})"),
});
}
let num_ints = (end - start).checked_add(1).ok_or_else(|| CMDToolError {
details: format!("range [{start}, {end}] is too large: count overflows u32"),
})?;
```
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
>
> ----
>
> *This section details on the original issue you should resolve*
>
> <issue_title>Add boundary checks in
gen_associated_data_from_range()</issue_title>
> <issue_description>> Since `start`/`end` are user-provided (via the
CLI wrapper), this function should defensively validate `end >= start`
and avoid unchecked `u32` arithmetic (currently `end - start + 1` will
overflow/panic when `end < start`, and can also overflow when `end ==
u32::MAX && start == 0`). Consider returning an error on invalid ranges
and computing the count with checked arithmetic.
>
> _Originally posted by @Copilot in
[microsoft#763](https://github.com/microsoft/DiskANN/pull/763/changes/BASE..79acee9153745a996d163ab0db3026683f4d6f8a#r2834667390)_</issue_description>
>
> ## Comments on the Issue (you are @copilot in this section)
>
> <comments>
> </comments>
>
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
- Fixes microsoft#788
<!-- START COPILOT CODING AGENT TIPS -->
---
📱 Kick off Copilot coding agent tasks wherever you are with [GitHub
Mobile](https://gh.io/cca-mobile-docs), available on iOS and Android.
An AI generated, human reviewed list of changes is summarized below. ([microsoft#998](microsoft#998)) `DiskANNIndex::get_degree_stats` now takes an explicit iterator of IDs instead of requiring the data provider to implement `IntoIterator`. ```rust // Before — provider had to impl IntoIterator index.get_degree_stats(&mut accessor)?; // After — caller supplies the ID iterator index.get_degree_stats(&mut accessor, id_iter)?; ``` ([microsoft#1044](microsoft#1044)) With `AlignedBoxWithSlice` removed from the PQ path, the dimension handling has been refactored into a three-layer contract: | Layer | Where | Contract | |---|---|---| | **Boundary (inmem)** | `QueryComputer::new`, `MultiQueryComputer::new`, `DistanceComputer::evaluate_similarity` | `len == dim` (returns `Err` on mismatch) | | **Boundary (disk)** | `PQScratch::set` | `len >= dim`, slices to `[..dim]` | | **Internal** | `TableL2/IP/Cosine::{new, populate}` | Trusted — no re-validation | **Other changes:** - PQ table populate/distance methods now accept `&[f32]` instead of `<U: Into<f32>>`. Callers must pre-decode quantized vectors via `VectorRepr::as_f32`. - Generic trampoline impls (`&Vec<u8>`, `&&[u8]`) on `QueryComputer` / `DistanceComputer` have been removed. ([microsoft#976](microsoft#976)) The free functions `calculate_chunk_offsets` and `calculate_chunk_offsets_auto` have been moved into constructors on `ChunkOffsets` / `ChunkOffsetsView` in `diskann-quantization::views`. ```rust // Before let offsets = calculate_chunk_offsets(dim, num_chunks); // After (allocating) let offsets = ChunkOffsets::partition(dim, num_chunks)?; // After (zero-alloc, borrows caller-owned scratch) let view = ChunkOffsetsView::partition_into(dim, &mut scratch)?; ``` Additionally, `get_chunk_from_training_data` has been moved from public API. ([microsoft#1052](microsoft#1052)) The entire `diskann_providers::model::graph::provider::async_::caching` module has been deleted. **Why:** The `CachingProvider` was an experiment in transparent caching over `DataProvider`. In practice it required double monomorphization of the indexing code, didn't save integration work for bulk methods like `on_elements_unordered`/`distances_unordered`, and was complex to maintain. An internal user who …migrated off it removed ~1,000 lines of code, improved compile times by ~20%, and substantially reduced complexity. **Upgrade:** Manage caching directly in your `DataProvider` implementation. ([microsoft#1045](microsoft#1045)) Native V4 (AVX-512) specializations for 4-bit packed vector distance computations: - **`SquaredL2`** — 16 × `u32` lanes per iteration via `_mm512_madd_epi16`. - **`InnerProduct`** — AVX-512 VNNI (`_mm512_dpbusd_epi32`) over `u8x64` / `i8x64` operands. Previously, V4 hardware fell back to two AVX2 (V3) kernel invocations per 512-bit chunk. The native kernels double per-instruction throughput. No API changes — existing code benefits automatically on AVX-512 capable hardware. * Deprecate 32-bit targets by @suhasjs in microsoft#1022 * Add a fast path to `Map::prepare`. by @hildebrandmw in microsoft#1023 * Add boundary checks in gen_associated_data_from_range() by @Copilot in microsoft#847 * [deps] Don't pull `rayon` as a dependency of `diskann`. by @hildebrandmw in microsoft#1024 * Bump openssl from 0.10.78 to 0.10.79 by @dependabot[bot] in microsoft#1026 * Cleaning up test work and changing the get_degree_stats signature. by @JordanMaples in microsoft#998 * Reduce scalar-quantization benchmark monomorphization by @suri-kumkaran in microsoft#1041 * [diskann-vector] Support truly unaligned distances. by @hildebrandmw in microsoft#981 * rename spherical.json to graph index with spherical quantization by @harsha-simhadri in microsoft#1042 * [PQ Cleanup] Part 2: Consolidate `calculate_chunk_offsets*` by @arkrishn94 in microsoft#976 * PQ: tighten dim contract; right-size scratch buffer by @wuw92 in microsoft#1044 * Add v4 distance kernels (4-bit SquaredL2 / InnerProduct) by @m3hm3t in microsoft#1045 * Remove the Caching Provider by @hildebrandmw in microsoft#1052 * @suhasjs made their first contribution in microsoft#1022 * @m3hm3t made their first contribution in microsoft#1045 **Full Changelog**: microsoft/DiskANN@v0.51.0...v0.52.0 Co-authored-by: Mark Hildebrand <mhildebrand@microsoft.com>
gen_associated_data_from_range()performed uncheckedu32arithmetic on user-supplied CLI values:end - startunderflows whenend < start, andend - start + 1overflows whenend == u32::MAX && start == 0.Changes
CMDToolErrorearly whenend < startend - start + 1with(end - start).checked_add(1), returning an error on overflowtest_gen_associated_data_from_range_end_less_than_startandtest_gen_associated_data_from_range_max_overflowOriginal prompt
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.