You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix native dependency advisories and preserve saved index compatibility - #172
Engine 1.0.0 dependency scans report vulnerable PyO3, Thrift, Rustls, maligned, and optional lru versions. This change updates those dependencies while retaining the existing saved-index format.
Update PyO3, Arrow/Parquet, Rustls, and the optional terminal tools. Remove Thrift and maligned.
Backport aligned byte storage into epserde 0.8.0. Declare the Apache-only additions in the package license and provenance.
Reject truncated input, release loader storage on errors, and correct ControlFlow reader tags to match the existing writer.
Run the copied library tests explicitly in CI. Isolate documentation-test temporary files and retain eight frozen index fixtures.
The updated library passes 89 checks, including four new regression tests and 11 documentation tests. All eight frozen-index tests pass through the three loaders with unchanged saved bytes. Generated artifact and notice checks pass for 404 components. Regression tests reproduced the enum-tag error, truncated-input panic, and acceptance of an incomplete file before the fixes.
At e7428e17, the full Rust workspace run completed with 2,101 passed, one failed, and 18 ignored. The unchanged disk_manager::tests::test_eviction_under_real_disk_pressure fails with InsufficientSpace on this machine. Standalone library Clippy passes with its existing manual_is_multiple_of lint allowed; without that allowance, that unchanged expression fails warnings-as-errors.
A wheel from earlier commit 0eced1c passed the Azure T4 training, prediction, and old/new model reload checks. That run used one candidate, one epoch capped at two batches, and 50 predictions. The new reader changes have not been GPU-tested.
Remaining limits
Production manylinux builds, full training, other GPU classes, and multiple GPUs remain unverified. The earlier Trivy dependency scan reported zero findings; independent OSV records for 11 other unchanged dependency versions still need exposure review. This PR does not publish a package or change the release version.
The change vendors epserde 0.8.0 and configures the workspace to use the local copy. The vendored crate adds serialization, deserialization, memory-loading APIs, standard-type implementations, and tests. The workspace updates Arrow and Parquet dependencies, optional terminal UI dependencies, and PyO3. It also adjusts code for updated library APIs, adds legacy index compatibility fixtures and tests, and refreshes third-party notices, artifact manifests, and source provenance.
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant Serialize
participant WriterWithPos
participant Deserialize
participant check_header
participant DeserializeInner
Caller->>Serialize: Serialize a value
Serialize->>WriterWithPos: Write header and root value
WriterWithPos->>WriterWithPos: Forward bytes and track position
Caller->>Deserialize: Deserialize serialized bytes
Deserialize->>check_header: Validate serialized header
Deserialize->>DeserializeInner: Deserialize full value or borrowed view
Deserialize-->>Caller: Return value or error
Loading
Important
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
The pull request changes 92 files. This exceeds the limit of 40 files.
Split the pull request into smaller, reviewable units.
Change Map Design Review
The required PR conversation comment beginning <!-- kumo-pr-change-map --> is unavailable. The checkout contains only the template, checker, and review guidance, not the author-authored change map. …
Provide the exact author-authored change-map comment, including its table and rationale. Then reassess each proposed change against the diff, existing consumers, and verification. The map should explicitly address the borrowed-slice seriali…
Full details: Public Resource Access
Explanation
The previous failure remains unresolved. The pull request adds inaccessible docs.rs references in packages/diskgraph/third_party/epserde/README.md: lines 694, 696�700, and 708. Anonymous requests returned HTTP 404 for MaxSizeOf, ReprHash, DeserializeInner, SerializeInner, Epserde, and the DeserType anchor. The current diff still adds the latest URLs. Versioned AlignHash and Epserde replacements returned HTTP 200.
Resolution
Update README lines 694, 697, 698, 700, and 708 to public versioned 0.8.0 docs.rs URLs. Replace the nonexistent ReprHash reference at line 696 with the local AlignHash API: https://docs.rs/epserde/0.8.0/epserde/traits/type_info/trait.AlignHash.html. Use https://docs.rs/epserde-derive/0.8.0/epserde_derive/derive.Epserde.html for Epserde, and use the versioned DeserializeInner URL with its DeserType anchor.
Full details: Change Map Design Review
Explanation
The required PR conversation comment beginning <!-- kumo-pr-change-map --> is unavailable. The checkout contains only the template, checker, and review guidance, not the author-authored change map. The PR description links to a comment but does not include its table or rationale, so necessity, abstraction-layer placement, focused verification, and stopping condition cannot be assessed. The diff also adds an unsafe borrowed-slice serializer in packages/diskgraph/third_party/epserde/src/impls/slice.rs:42-56; its error path uses ? before mem::forget, which requires explicit treatment in the missing design map.
Resolution
Provide the exact author-authored change-map comment, including its table and rationale. Then reassess each proposed change against the diff, existing consumers, and verification. The map should explicitly address the borrowed-slice serializer error path and explain the focused test and stopping condition.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details.
Observed requirement: remove the reported vulnerable native dependencies while keeping existing saved indexes readable.
The previous build used PyO3 0.28.3, Thrift 0.17, Rustls 0.23.45, maligned 0.2.1, and optional lru 0.12.5.
The change updates their dependency paths, backports byte-alignment types into epserde 0.8, and keeps format 1.1.
Verification covers the dependency graph, frozen-index fixtures, native tests, notices, and GPU training at commit 0eced1c. The later trim keeps runtime code and resolved dependencies unchanged.
Stop after these fixes and compatibility checks. Other OSV records and full release qualification remain separate work.
File
What changed
Why
3rdparty/notices/inventory.json
Regenerate dependency attribution and record the vendored epserde legal files.
Ship notices for the resolved artifacts and remove departed versions.
THIRD-PARTY.txt
Regenerate dependency attribution and record the vendored epserde legal files.
Ship notices for the resolved artifacts and remove departed versions.
packages/diskgraph/Cargo.lock
Resolve the patched dependency graph and remove Thrift/maligned.
Build fixed PyO3/Rustls and optional lru versions consistently.
packages/diskgraph/Cargo.toml
Update the Arrow/Parquet or PyO3/TUI requirements and register the epserde backport.
Keep compatible dependency versions across native crates.
packages/diskgraph/deny.toml
Remove the old maligned advisory exception.
The affected dependency is removed, so no exception remains necessary.
Guard the zero-size and failed allocations in load_mem.
If the file is empty, file_len is 0 and capacity is 0. std::alloc::alloc with a zero-size Layout is undefined behavior. A failed index write when the disk is full can leave a zero-byte file, so this input is realistic.
The code also does not check the returned pointer for null. On allocation failure, Vec::from_raw_parts receives a null pointer, which is undefined behavior.
There is also a leak. MemBackend is written into the MaybeUninit before deserialize_eps runs. If deserialize_eps returns Err, the early ? exits without dropping the backend, so the buffer leaks.
🛡️ Proposed fix
- #[allow(invalid_value)]- let mut aligned_vec = unsafe {- <Vec<MemoryAlignment>>::from_raw_parts(- std::alloc::alloc(std::alloc::Layout::from_size_align(capacity, align_to)?)- as *mut MemoryAlignment,- capacity / align_to,- capacity / align_to,- )- };+ if capacity == 0 {+ return Err(Error::ReadError.into());+ }+ let layout = std::alloc::Layout::from_size_align(capacity, align_to)?;+ let raw = unsafe { std::alloc::alloc(layout) };+ if raw.is_null() {+ std::alloc::handle_alloc_error(layout);+ }+ let mut aligned_vec = unsafe {+ <Vec<MemoryAlignment>>::from_raw_parts(+ raw as *mut MemoryAlignment,+ capacity / align_to,+ capacity / align_to,+ )+ };
To fix the leak, drop the backend on the error path, for example with addr_of_mut!((*ptr).1).drop_in_place(), before you return the error from deserialize_eps.
Based on learnings: "verify ... security implications such as unbounded allocation, malformed input handling ... during deserialization."
Make the ControlFlow tags match between serialization and deserialization.
383-390: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Make the ControlFlow tags match between serialization and deserialization.
_serialize_inner writes tag 0 for Break and tag 1 for Continue (Lines 368 and 372). Both deserialization paths treat 1 as Break and 2 as Continue.
As a result, a serialized Break fails with InvalidTag(0). A serialized Continue decodes as Break and reads the payload as type B. If B and C have compatible layouts, the value is corrupted silently. No test covers the ControlFlow round trip.
Change the reader to use 0/1. Files written by this code always use 0/1, so the change is compatible with existing data.
Return ReadError for truncated input instead of panicking.
SliceWithPos::skip slices self.data[bytes..] without a bounds check. align calls skip(padding) without a bounds check. The ε-copy helpers also index past the end without a check: deserialize_eps_zero, deserialize_eps_slice_zero, the array implementation, and the primitive _deserialize_eps_inner. Each of these indexes backend.data[..bytes] first and calls skip afterwards.
The trigger is a truncated index file. A partial write when the disk is full, or an interrupted store, can produce one. When load_mem, load_mmap, or mmap loads that file, the process panics instead of receiving a deser::Error. The ReaderWithPos path already returns ReadError for the same input, so the two loader families behave differently.
This repository is now the sole source of truth for this code. Add the check in skip and align, and validate bytes <= backend.data.len() before slicing in the helpers. Use checked_mul for len * size_of::<T>() in deserialize_eps_slice_zero. These checks do not change the saved format.
121-123: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use unique temporary files in the doctests.
The excluded epserde package is not tested by the inspected CI workflow, so this is not a demonstrated CI collision. However, when the package is tested directly, each doctest uses a fixed name in the shared temporary directory. std::fs::File::create(&file) can truncate a pre-existing file, and concurrent direct test runs can share these paths. Create a unique temporary directory or file for each example and remove it after the example completes.
Please shorten the description: remove the trim history and detailed line counts. Keep the problem, final changes, current test results, and remaining limits.
Check truncated reads, own loader storage until successful deserialization, and match ControlFlow writer tags. Run copied-library tests in CI, isolate doctest files, and record the Apache requirement for local additions.
704-704: Minor · Functional Correctness · Quick win
Pin the epserde API documentation links.
This link points to epserde 0.13.1, whose load_mem signature differs from the vendored 0.8.0 API. Developers following it will see a different return type and safety contract. Pin the epserde links to 0.8.0 or link to the local documentation. (docs.rs)
69-83: Minor · Stability & Availability · Quick win
Handle invalid UTF-8 without panicking or creating an invalid &str.
The two deserialization paths fail differently on invalid UTF-8:
Full path:String::from_utf8(slice).unwrap() panics. check_header decodes TYPE_NAME through this function on every load. A corrupt header therefore panics in load_full, load_mem, load_mmap, and mmap, so callers never receive a deser::Error.
ε-copy path: the code transmutes unvalidated bytes to &str. Invalid UTF-8 in a &str breaks a language invariant, and later string operations can misbehave.
Map the full-path failure to an error. For the ε-copy path, validate with core::str::from_utf8. If profiling shows the O(n) check is too expensive, keep the unchecked path and document the trusted-input requirement.
42-50: Minor · Stability & Availability · Quick win
Bound the allocation before trusting a length read from the file.
deserialize_full_vec_zero reads len from the stream and then calls Vec::with_capacity(len) before it reads any payload. The same pattern appears in deserialize_full_vec_deep (Line 61) and deserialize_eps_vec_deep (Line 109). A corrupt length field fails in one of two ways:
Capacity overflow: a very large len makes len * size_of::<T>() overflow, and the code panics.
Allocation failure: a large but representable len triggers handle_alloc_error, which aborts the process.
This path runs on every load, not only for user data. check_header reads TYPE_NAME through String::_deserialize_full_inner, which calls this helper. A corrupt header therefore aborts load_full, load_mem, load_mmap, and mmap instead of returning deser::Error. The truncated-read checks added in this PR do not cover this case.
🛡️ Proposed fix
let len = usize::_deserialize_full_inner(backend)?;
+ len.checked_mul(core::mem::size_of::<T>())+ .ok_or(deser::Error::ReadError)?;
backend.align::<T>()?;
- let mut res = Vec::with_capacity(len);+ let mut res = Vec::new();+ res.try_reserve_exact(len)+ .map_err(|_| deser::Error::ReadError)?;
The ε-copy deep path can bound the initial capacity by the remaining input:
let len = usize::_deserialize_full_inner(backend)?;
- let mut res = Vec::with_capacity(len);+ let mut res = Vec::with_capacity(len.min(backend.data.len()));
In deserialize_full_vec_deep, cap the initial capacity at a fixed value, for example len.min(4096), and let push grow the vector.
Based on learnings: "verify ... security implications such as unbounded allocation, malformed input handling ... during deserialization."
Source: Learnings
74-79: Minor · Stability & Availability · Quick win
Do not return an uninitialized reference for zero-sized types.
The function returns &'a T, so the compiler infers MaybeUninit::uninit() as MaybeUninit<&'a T>. Calling assume_init() on it creates an uninitialized reference, which is undefined behavior immediately. A reference must be non-null and aligned even when T is zero-sized. The #[allow(invalid_value)] attribute suppresses the compiler warning about exactly this problem.
Zero-copy zero-sized structs reach this branch through ε-copy deserialization. The tests exercise it: test_generics.rsData3, test_phantom.rsZeroCopyType and OnlyPhantom. A dangling non-null pointer is valid for zero-sized types.
🐛 Proposed fix
if bytes == 0 {
- // SAFETY: T is zero-sized and `assume_init` is safe.- #[allow(invalid_value)]- #[allow(clippy::uninit_assumed_init)]- return Ok(unsafe { MaybeUninit::uninit().assume_init() });+ // SAFETY: a dangling, aligned, non-null pointer is a valid+ // reference to a zero-sized type.+ return Ok(unsafe { &*core::ptr::NonNull::<T>::dangling().as_ptr() });
}
131-138: Minor · Stability & Availability · Quick win
Fix the panic when a write follows a seek past the end.
set_position and Seek accept any usize position. The docs at Line 95 state this explicitly. Here is the failure path when self.pos is greater than the current capacity cap:
In a debug build, cap - self.pos overflows and panics.
In a release build, the subtraction wraps to a huge rem, so rem < len is false and the vector does not grow. bytes[pos..pos + len] then panics with an out-of-bounds index.
std::io::Cursor handles this case by zero-filling the gap. resize already zero-fills here, so only the comparison needs to change. Line 123 bounds len, so self.pos + len cannot overflow.
🐛 Proposed fix
- let cap = self.vec.len().saturating_mul(std::mem::size_of::<T>());- let rem = cap - self.pos;- if rem < len {- self.vec.resize(- (self.pos + len).div_ceil(std::mem::size_of::<T>()),- T::default(),- );- }+ let end = self.pos + len;+ let cap = self.vec.len().saturating_mul(std::mem::size_of::<T>());+ if cap < end {+ self.vec+ .resize(end.div_ceil(std::mem::size_of::<T>()), T::default());+ }
214-221: Minor · Stability & Availability · Quick win
Return an error for invalid char and NonZero* values.
Three deserializers call unwrap() on values decoded from file bytes, so malformed input panics:
char::from_u32(...).unwrap() at Line 214 and Line 221.
NonZerotry_into().unwrap() at Line 117 and Line 127.
A corrupt or zeroed region in a saved file aborts the load with a panic instead of returning deser::Error. No upstream validation excludes these values: the type hash checks only the type, not the payload. Add an error variant, for example InvalidValue, to deser::Error and map these conversions to it.
🛡️ Proposed fix (char shown; apply the same change to the NonZero macro)
Based on learnings: "flag unwrap(), expect(), or indexing on data derived from untrusted input only after checking whether malformed input can actually make it panic."
This build script does no useful work. It appends "IT WORKS" to the README. It prints the README to stderr with dbg!. It then exports the README through cargo:rustc-env=README=.... The crate never reads that README variable: src/lib.rs Line 8 uses include_str! instead. Cargo parses the multi-line value one line at a time, so only the first README line reaches the variable. The other lines go to the build-script stdout.
Every consumer of epserde still has to compile and run this build script. Delete build.rs. Then record the removal in LOCAL-PATCH.md next to the maligned removal.
Files not fully reviewed because their changes were unavailable: 1
3rdparty/notices/inventory.json
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
167-167: Minor · Stability & Availability · Quick win
Return an error instead of panicking on an exhausted RangeInclusive.
_serialize_inner writes an exhausted flag. An exhausted range, for example one that was iterated, is a valid value to serialize. Deserialization then hits assert! and panics. Return a deser::Error in both paths.
🐛 Proposed fix
- assert!(!exhausted, "cannot deserialize an exhausted range");+ if exhausted {+ return Err(deser::Error::InvalidTag(1));+ }
Apply the same change at Line 178.
Also applies to: 178-178
Source: Learnings
.github/workflows/testing.yml: 1
385-386: Minor · Maintainability & Code Quality · Quick win
Pin the dependency resolution of the standalone epserde test step.
The Test copied epserde library step runs from packages/diskgraph, but it does not pass --locked. The vendored crate at third_party/epserde is excluded from the workspace and has no Cargo.lock. Each fresh checkout resolves its dependencies to the latest semver-compatible releases. A new upstream release can then break this step or change which code it tests.
Commit a Cargo.lock for the vendored crate. Then pass --locked to the step.
Suggested fix
- name: Test copied epserde library
- run: cargo test --manifest-path third_party/epserde/Cargo.toml+ run: cargo test --locked --manifest-path third_party/epserde/Cargo.toml
Call deserialize_eps in the ε-copy blocks of test_struct_deep and test_struct_zero.
Both blocks bind the result to eps but call deserialize_full. The tests duplicate the full-copy check and never exercise the ε-copy path for derived structs. This PR changed that path to use bounds-checked reads.
✅ Proposed fix
- let eps = <Struct>::deserialize_full(&mut cursor).unwrap();- assert_eq!(a, eps);+ let eps = <Struct>::deserialize_eps(cursor.as_bytes()).unwrap();+ assert_eq!(a, eps);
In test_struct_zero, the ε-copy result is a reference:
- let eps = <Struct>::deserialize_full(&mut cursor).unwrap();- assert_eq!(a, eps);+ let eps = <Struct>::deserialize_eps(cursor.as_bytes()).unwrap();+ assert_eq!(a, *eps);
Files not fully reviewed because their changes were unavailable: 1
3rdparty/notices/inventory.json
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
34-34: Minor · Data Integrity & Integration · Quick win
Protect the frozen fixtures from the patched writer.
Running this example in the current workspace overwrites the frozen binaries and expected values with patched-library output. The compatibility tests can then pass without testing the legacy format. Require an explicit output directory or refuse to overwrite existing fixtures by default. (raw.githubusercontent.com)
Cover the production long-key lookup in the frozen-index tests.
The frozen reader tests call only index(). Production uses index_no_remap for ingest lookups. Production also sets prefer_fallback_hash for keys longer than 16 bytes. The tests do not check either path against a saved index. The saved bytes do not depend on this choice, so this is a coverage gap rather than a format incompatibility.
Add a regression that loads or builds an index with keys longer than 16 bytes. Check its index_no_remap results against expected slot values.
Files not fully reviewed because their changes were unavailable: 1
3rdparty/notices/inventory.json
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Engine 1.0.0 dependency scans report vulnerable PyO3, Thrift, Rustls, maligned, and optional lru versions. This change updates those dependencies while retaining the existing saved-index format.
Prior discussion: PR171.
Change map
Maintained file-by-file map.
Testing Done
The updated library passes 89 checks, including four new regression tests and 11 documentation tests. All eight frozen-index tests pass through the three loaders with unchanged saved bytes. Generated artifact and notice checks pass for 404 components. Regression tests reproduced the enum-tag error, truncated-input panic, and acceptance of an incomplete file before the fixes.
At
e7428e17, the full Rust workspace run completed with 2,101 passed, one failed, and 18 ignored. The unchangeddisk_manager::tests::test_eviction_under_real_disk_pressurefails withInsufficientSpaceon this machine. Standalone library Clippy passes with its existingmanual_is_multiple_oflint allowed; without that allowance, that unchanged expression fails warnings-as-errors.A wheel from earlier commit
0eced1cpassed the Azure T4 training, prediction, and old/new model reload checks. That run used one candidate, one epoch capped at two batches, and 50 predictions. The new reader changes have not been GPU-tested.Remaining limits
Production manylinux builds, full training, other GPU classes, and multiple GPUs remain unverified. The earlier Trivy dependency scan reported zero findings; independent OSV records for 11 other unchanged dependency versions still need exposure review. This PR does not publish a package or change the release version.