[PR #98] fix(overlaybd): validate index bounds before allocation #177

New issue

Open

opened 2026-10-29 14:20:40 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:20:40 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/98
Author: @morluto
Created: 7/31/2026
Status: 🔄 Open

Base: main ← Head: fix/overlaybd-index-bounds

📝 Commits (9)

11cb227 fix(overlaybd): validate index bounds before loading
12c6398 test(overlaybd): cover combined index bounds
9f09aaf fix(overlaybd): separate trailer and index validation
bf11ab0 fix(overlaybd): bound index allocations
97843b9 fix(overlaybd): budget decoded index memory
d02f2f8 fix(overlaybd): bound compact index construction
0359b0e fix(overlaybd): bound index allocation growth
0d1b50f fix(overlaybd): bound compaction working memory
ebffd4b fix(overlaybd): reject zero compaction buffers

📊 Changes

6 files changed (+452 additions, -87 deletions)

View changed files

📝 storage/overlaybd/src/lsmt/file/helper.rs (+260 -52)
📝 storage/overlaybd/src/lsmt/file/readonly.rs (+9 -6)
📝 storage/overlaybd/src/lsmt/file/readwrite.rs (+14 -15)
📝 storage/overlaybd/src/lsmt/file/stack.rs (+20 -13)
📝 storage/overlaybd/src/lsmt/file/tests.rs (+147 -1)
📝 storage/overlaybd/src/lsmt/file/types.rs (+2 -0)

📄 Description

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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/98 **Author:** [@morluto](https://github.com/morluto) **Created:** 7/31/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/overlaybd-index-bounds` --- ### 📝 Commits (9) - [`11cb227`](https://github.com/kvcache-ai/AgentENV/commit/11cb227703e72dba1cd94b8c88fe091c1725c351) fix(overlaybd): validate index bounds before loading - [`12c6398`](https://github.com/kvcache-ai/AgentENV/commit/12c639850a5c2c07fe1775e8761bb0f972104e1f) test(overlaybd): cover combined index bounds - [`9f09aaf`](https://github.com/kvcache-ai/AgentENV/commit/9f09aaf1c0bba3f33b29a303da41f9941cea1579) fix(overlaybd): separate trailer and index validation - [`bf11ab0`](https://github.com/kvcache-ai/AgentENV/commit/bf11ab00d1c3d7b2420362bc64dbfae506d555a9) fix(overlaybd): bound index allocations - [`97843b9`](https://github.com/kvcache-ai/AgentENV/commit/97843b9a31d83a8fa9f8001cba79e88b84fce4ae) fix(overlaybd): budget decoded index memory - [`d02f2f8`](https://github.com/kvcache-ai/AgentENV/commit/d02f2f8e27e754f17870b9bab19b1293957f8d9c) fix(overlaybd): bound compact index construction - [`0359b0e`](https://github.com/kvcache-ai/AgentENV/commit/0359b0ef83f495777ef01c81cc674db8c9f95965) fix(overlaybd): bound index allocation growth - [`0d1b50f`](https://github.com/kvcache-ai/AgentENV/commit/0d1b50fc2687df30125812f6d93cbcd803a63b59) fix(overlaybd): bound compaction working memory - [`ebffd4b`](https://github.com/kvcache-ai/AgentENV/commit/ebffd4b481cd7078ce31bb52217ad98a96844cfd) fix(overlaybd): reject zero compaction buffers ### 📊 Changes **6 files changed** (+452 additions, -87 deletions) <details> <summary>View changed files</summary> 📝 `storage/overlaybd/src/lsmt/file/helper.rs` (+260 -52) 📝 `storage/overlaybd/src/lsmt/file/readonly.rs` (+9 -6) 📝 `storage/overlaybd/src/lsmt/file/readwrite.rs` (+14 -15) 📝 `storage/overlaybd/src/lsmt/file/stack.rs` (+20 -13) 📝 `storage/overlaybd/src/lsmt/file/tests.rs` (+147 -1) 📝 `storage/overlaybd/src/lsmt/file/types.rs` (+2 -0) </details> ### 📄 Description ## 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 ```text 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: ```text 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 - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-29 14:20:40 +00:00

bsakaguchi referenced this issue

2026-10-29 14:21:15 +00:00

[PR #177] [MERGED] fix(snapshot): parallelize OSS layer uploads and skip publish-time re-hash #229

bsakaguchi referenced this issue

2026-10-29 14:21:19 +00:00

[PR #185] [MERGED] fix(snapshot): revert digest tracking on compaction output paths #234