fix(overlaybd): validate index bounds before allocation by morluto · Pull Request #98 · kvcache-ai/AgentENV

morluto · 2026-07-31T11:12:42Z

What

Validate OverlayBD index offsets and mapping counts against the backing file before allocating or reading an index.

The loader now:

keeps trailer-provided mapping counts as u64 until checked conversion;
rejects multiplication and allocation-size overflow;
verifies that the index starts after the header and ends before the sealed trailer;
applies the same checks to sealed layers, stacked lower layers, and index-only writable-layer files.

Why

At the affected base, load_index calculated count * size_of::<DiskSegmentMapping>() and allocated that buffer before checking whether the trailer-described range existed in the file. A corrupted trailer could therefore drive excessive allocation or arithmetic failure before returning a malformed-layer error.

Closes #97.

Scope and non-goals

This PR changes parser validation only. It does not change the OverlayBD wire format, valid layer layout, index ordering, compression, or remote backend behavior.

Design and behavior changes

HeaderTrailer(index_offset, index_count)
                  │
                  ▼
checked count × mapping_size
and checked file-range validation
          ├── valid   ──> allocate and read
          └── invalid ──> return a structured error

For sealed data files, the valid range ends at the start of the 4 KiB trailer. For index-only writable files, it ends at the current file length.

Compatibility and operations

Public API or generated protocol: N/A; no API change.
Configuration or defaults: N/A.
Snapshot manifest, artifact layout, or storage format: unchanged; valid existing layers retain their current behavior.
Upgrade and rollback: no migration is required.
Host requirements, permissions, ports, or dependencies: unchanged.

Validation

make fmt
make clippy
make test-unit
Relevant Rust integration tests
make -C services test (required when services/ changes)
Generated clients/server regenerated with the documented make target
Documentation updated
Benchmarks or performance comparison completed

Commands and results:

cargo fmt --all -- --check
  passed

cargo clippy -p overlaybd --lib --tests -- -D warnings
  passed

cargo test -p overlaybd --lib rejects_sealed_index_offset_out_of_bounds
cargo test -p overlaybd --lib rejects_sealed_index_size_out_of_bounds
cargo test -p overlaybd --lib rejects_index_count_overflow_before_allocation
  each passed: 1 passed, 0 failed

cargo test -p overlaybd --lib --no-fail-fast -- --test-threads=1
  passed: 306 passed, 0 failed, 1 ignored

The library suite is listed with one test thread because several tests share the process-global transient I/O worker; the parallel invocation can close that shared channel and cascade unrelated channel closed failures.

Risks and reviewer notes

The main review point is validate_index_bounds in storage/overlaybd/src/lsmt/file/helper.rs. In particular, sealed files reserve HEADER_SIZE at the end for the trailer, while direct index-file loading intentionally validates against the full file length.

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring.
New behavior is covered by tests, or I explained why testing is impractical.
Logs and examples contain no credentials, tokens, or private registry information.
I did not manually edit generated code without updating its source and regenerating it.

github-actions · 2026-07-31T12:03:20Z

🔍 OpenCodeReview found 2 issue(s) in this PR.

✅ Successfully posted inline: 2 comment(s)

⚠️ 1 warning(s) occurred during review.

⚠️ Warnings:

storage/overlaybd/src/lsmt/file/helper.rs (subtask_error): LLM completion error: context deadline exceeded

github-actions · 2026-07-31T19:23:21Z

+    let total_index_memory = metadata.iter().try_fold(0usize, |total, layer| {
+        total
+            .checked_add(stack_index_memory_bytes(layer.index_size)?)
+            .context("stack index memory size overflow")
+    })?;
+    ensure!(
+        total_index_memory <= MAX_STACK_INDEX_MEMORY_BYTES,
+        "stack index memory {total_index_memory} exceeds limit {MAX_STACK_INDEX_MEMORY_BYTES}"
+    );


[bug · medium]
This limit is enforced only when merge_readonly_indexes runs. In open_files_ro_with_premerged_cache, both cache-hit branches return a decoded merged index before calling this function, so the same layer stack can bypass MAX_STACK_INDEX_MEMORY_BYTES whenever a cache artifact exists. Move the metadata-based validation to a helper invoked immediately after metadata loading (before cache lookup), or also apply it on each cache-hit path, so behavior and the memory policy do not depend on cache state.

github-actions · 2026-07-31T19:23:21Z

+#[tokio::test]
+async fn rejects_sealed_index_size_out_of_bounds() {
+    let temp_dir = TempDir::new().unwrap();
+    let data = create_sealed_layer(&temp_dir, "bad-index-size", 8192, &[(0, 0x22)]).await;
+    let file_size = data.size().await.unwrap();
+    let trailer = verify_ht(&(data.clone() as Arc<dyn VirtualFile>), true, file_size)
+        .await
+        .unwrap();
+    let stride = size_of::<DiskSegmentMapping>() as u64;
+    let remaining = file_size - HEADER_SIZE - trailer.index_offset.get();
+    let bad_index_size = remaining / stride + 1;


[test · low]
The new bounds tests only exercise clearly invalid values. Add direct acceptance tests for the inclusive boundaries used by validate_index_bounds, especially an index ending exactly at file_size - HEADER_SIZE and a zero-count index at a valid boundary. Otherwise an accidental <=/< regression could reject valid sealed layers while all these rejection tests continue to pass.