Conversation
There was a problem hiding this comment.
Pull request overview
Adds Kani verification artifacts for Challenge 23’s Vec safety targets.
Changes:
- Adds contracts and proof harnesses for
VecAPIs. - Introduces Kani-specific loop invariants and helper accessors.
- Updates verification dependencies and compiler features.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
library/Cargo.lock |
Records safety-contract dependencies. |
library/alloc/src/lib.rs |
Enables proc-macro hygiene. |
library/alloc/src/vec/mod.rs |
Adds contracts, loop models, and harnesses. |
library/alloc/src/vec/set_len_on_drop.rs |
Adds Kani-only pointer accessors. |
Suppressed comments (1)
library/alloc/src/vec/set_len_on_drop.rs:34
- As with
local_len_ptr, thispub(super)verification helper should not add an inline hint under the repository's inline policy. Remove#[inline].
#[inline]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| use super::*; | ||
|
|
||
| const MAX_VEC_LEN: usize = 4; |
| // Harnesses for `Vec::from_raw_parts` | ||
| macro_rules! gen_from_raw_parts_harness { | ||
| ($name:ident, $ty:ty) => { | ||
| #[kani::proof_for_contract(Vec::<$ty>::from_raw_parts)] |
| .checked_mul(capacity) | ||
| .is_some_and(|size| size <= isize::MAX as usize) | ||
| ))] | ||
| #[cfg_attr(kani, kani::requires({ |
| @@ -1975,6 +2052,8 @@ impl<T, A: Allocator> Vec<T, A> { | |||
| /// [`spare_capacity_mut()`]: Vec::spare_capacity_mut | |||
| #[inline] | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| #[cfg_attr(kani, kani::requires(new_len <= self.capacity()))] | |||
| let spare = self.capacity().saturating_sub(self.len()); | ||
| count <= spare | ||
| && ( |
| let spare_write_len = if mem::size_of::<T>() == 0 { 0 } else { 8 }; | ||
| #[cfg(kani)] | ||
| let spare_write_set = | ||
| core::ptr::slice_from_raw_parts_mut(self.as_mut_ptr(), spare_write_len); |
| let new_len: usize = if core::mem::size_of::<$ty>() == 0 { | ||
| kani::any() | ||
| } else { | ||
| kani::any_where(|len: &usize| *len <= initialized_len) |
| // Create a non-deterministic Vec for the panic case | ||
| let mut vec = verifier_nondet_vec::<i32>(); | ||
| // Choose a non-deterministic split point outside the initialized length | ||
| let at = kani::any_where(|at: &usize| *at >= vec.len()); |
| let start: usize = kani::any_where(|start: &usize| *start <= end); | ||
| // Compute how many elements will be copied from the initialized prefix | ||
| let count = end - start; | ||
| // Call the unsafe internal function for the selected range |
| #[inline] | ||
| pub(super) fn local_len_ptr(&mut self) -> *mut usize { |
feliperodri
left a comment
There was a problem hiding this comment.
Review: PR #598 — Challenge 23 (Vec pt1)
1. The 11 #[cfg(not(kani))] blocks — NOT fatal (credit given)
I classified each against the fatal pattern from #537/#538/#557/#558 (replacing a verified body with a #[cfg(kani)] nondeterministic stub that assumes its own safety). None of them do this. Every one is a loop-contract rewrite that faithfully reproduces the real logic, only rebinding locals into pre-declared mut variables so kani::loop_modifies can name them, or splitting the original body from a Kani variant that adds loop_invariant/loop_modifies. The real writes (ptr::write, same_bucket, drop_in_place, increment_len, clone()) are preserved.
| # | Diff line | Function | Classification |
|---|---|---|---|
| 1 | 367 | dedup_by loop 1 |
Benign — kani block (356-366) recomputes found_duplicate into tracked vars; logic identical |
| 2-4 | 420,422,426 | dedup_by loop 2 |
Benign — kani block (410-419) rebinds read/prev ptr + found_duplicate; real drop/copy branch unchanged |
| 5 | 439 | dedup_by write_ptr |
Benign — rebinds write_ptr only |
| 6 | 516 | extend_with |
Benign swap — kani block (546-620) reproduces clone loop + last-write via SetLenOnDrop with invariants |
| 7 | 676 | spec_extend_from_within |
Benign swap — kani block (634-674) is manual MaybeUninit::write loop, semantically equal to iter::zip |
| 8 | 758,770 | extend_desugared |
Concern (not fatal): kani replaces if len==cap { reserve } with if len==cur_cap { kani::assume(false) } — see §3 |
| 9 | 777 | extend_desugared dst |
Benign — rebinds dst |
| 10 | 805 | extend_trusted |
Benign swap — kani block (814-885) reproduces for_each as manual loop with invariants |
Conclusion: no cfg-swap vacuity. This is materially better than the rejected PRs and I credit the author for structuring the rewrites correctly.
2. Contract-liveness (T7) — PASS (correcting the triage note)
The triage note "6 proof_for_contract, 0 added contracts" is incorrect. All 6 proof_for_contract targets have contracts added in this diff:
from_raw_parts(diff 187-217),from_parts(225-246),from_parts_in(254-275):kani::requiresset_len(283-284):requires+modifiesappend_elements(447-472):requires+modifiessplit_at_spare_mut_with_len(480-495):ensures
No orphaned contract harnesses. T7 is satisfied.
3. BLOCKING — both mandatory Challenge-23 criteria are violated
The spec states verbatim: "The verification must be unbounded---it must hold for slices of arbitrary length" and "The verification must hold for generic type T (no monomorphization)."
(a) Monomorphization — violated for 100% of harnesses. Every harness is macro-generated over a fixed concrete type list (u8..i128, usize/isize, (), [u8;4]). There is not a single generic-T harness. verifier_nondet_bounded_vec even requires T: kani::Arbitrary. Concrete primitives cannot discharge the obligation for T with validity/drop invariants. (Copilot: mod.rs:5000.) This alone fails the mandatory criterion.
(b) Bounded length — violated for several required functions. verifier_nondet_bounded_vec (diff 937-946) caps at MAX_VEC_LEN = 4, and is used by retain_mut, dedup_by, append_elements, extend_desugared, extend_trusted (lengths 0–4 only). Additionally extend_with is bounded len<=8, n<=8 (diff 2006-2007), extend_desugared's max_write is clamped <=8 with kani::assume(false) otherwise (diff 699-704), and the *_clone_only harnesses assume <=4/<=8. These directly contradict "arbitrary length." (Copilot: mod.rs:4845, 4169.)
Note: many other harnesses are properly unbounded via verifier_nondet_vec (symbolic cap/len with len<=cap) — good. But the mixed picture still fails the criterion for the bounded subset, and monomorphization fails it globally.
4. Additional soundness gaps (would block independently)
set_lenonly verifies shrinking. The harness constrains non-ZSTnew_len <= initialized_len(diff 1255), and the contract is onlynew_len <= self.capacity()(diff 283). The safety-sensitive half — growinglenover uninitialized spare — is never exercised, and the contract omits the "elements inold_len..new_leninitialized" precondition. (Copilot: mod.rs:2055, 5232.)append_elementsassumes away reallocation. Contract requirescount <= spare(diff 449-450), making the function's ownself.reserve(count)a no-op; the grow path is never verified. Spare capacity is not a precondition of this function. (Copilot: mod.rs:2999.)extend_desugaredassumes the Vec is never full (if len==cur_cap { kani::assume(false) }, diff 755-757), excluding every reserve/realloc path. (Copilot: mod.rs:4169, 4183.)split_offshould_panic harness includes a non-panicking case.harness_vec_split_off_out_of_boundsusesat >= vec.len()(diff 1732), butat == lenis valid forsplit_offand does not panic; the panic claim is not valid on every path. Should beat > len. (Copilot: mod.rs:5709.)from_parts/from_parts_inaccept a misalignedNonNullwhen ZST orcapacity==0(alignment requirement gated away, diff 260-271). (Copilot: mod.rs:797, 1242.)spec_extend_from_within_clone_only(diff 2090-2107) omits thecount <= cap - lenassumption that the macro variant has, so the body's internalkani::assume(can_write(...))silently discards invalid states instead of surfacing them. (Copilot: mod.rs:6080.)
Minor (non-blocking): #[inline] on pub(super) Kani-only helpers in set_len_on_drop.rs:29,34 violates the repo inline policy (Copilot).
Required direction to reach APPROVE
- Replace all monomorphized macro harnesses with generic-
Tharnesses (no concrete-type expansion), per the mandatory no-monomorphization criterion. - Remove the
MAX_VEC_LEN=4/<=8bounds; drive length symbolically and unbounded via loop contracts forretain_mut,dedup_by,append_elements,extend_desugared,extend_trusted,extend_with. - Make
set_lenverify the growth path (symbolic initialized prefix beyond old len) and strengthen its contract with the initialization precondition. - Cover the reallocation paths in
append_elementsandextend_desugaredrather than assuming spare capacity. - Fix the
split_offpanic-harness boundary (at > len) and add the unconditional alignment requirement tofrom_parts/from_parts_in.
The loop-contract engineering here is solid and free of the body-swap vacuity that sank prior PRs, but it does not yet meet the two mandatory criteria, so changes are required.
Notes on the
|
feliperodri
left a comment
There was a problem hiding this comment.
Thanks @v3risec. Reviewed Challenge 23 with our vacuity tooling. Broad coverage (35/35), but soundness issues block acceptance:
- 5 cfg(kani) body swaps (T1) — critical. Kani verifies alternate bodies for
spec_extend_from_within,extend_desugared,extend_trusted,extend_with, anddedup_by. The verified artifact is not the shipped code. - Pervasive
kani::assume(false)cutoffs. Inextend_desugaredthe reserve/grow path is short-circuited withif len == cur_cap { kani::assume(false); }and the iteration bound is capped (if raw_max_write > 8 { assume(false); });extend_trustedusesassume(false)on TrustedLen-yields-None early. Plus overflow branches forchecked_addand layout representability. These skip the interesting failure branches rather than verifying them. - Assume-the-conclusion inside verified bodies:
kani::assume(kani::mem::can_write(len_ptr))andcan_write(spare_write_set)are placed inside the fn being verified — assumes the memory-access safety property. set_lencontract is under-specified:#[requires(new_len <= self.capacity())]omits the initialized-prefix requirement the safety docstring calls out.- Fails unbounded+generic-T universally (monomorphized ~14 concrete types;
retain_mut/dedup_by/extend_desugaredetc. bounded MAX_VEC_LEN=4). verifier_nondet_vecfills the buffer with a single symbolic byte pattern (ptr::write_bytes(..., kani::any::<u8>(), size_of::<T>() * sz)), so all elements share the same byte value — swap_remove/dedup_by/retain_mut can't exercise element-to-element differentiation.
Between the two open Challenge 23 solutions we're prioritizing #569 as the sounder shell (no body swaps), though it too is incomplete (hardcoded ARRAY_LEN=3, monomorphized i32). Please replace the body swaps with loop contracts on the real bodies and drop the assume(false)s.
|
@feliperodri Thanks for the detailed review. I have substantially reworked this PR. Changes since the previous reviewThe main soundness issues have been addressed:
The raw-parts contracts also now encode the allocation base, writable capacity, exact allocation size/end, and initialized prefix requirements. Unbounded verificationA major improvement in the current revision is that all Challenge 23 target families except Lengths, capacities, indices, ranges, and operation counts remain symbolic, and loops that require induction are discharged using loop contracts. Non-ZST allocations are restricted by a CBMC object-model limit ( The two exceptions are currently: I am intentionally documenting these as limitations rather than claiming they satisfy the unbounded requirement. Remaining
|
|
A small follow-up to my previous comment: I have also strengthened the representative-type coverage. I added a non- I also added a These additions do not change the unbounded status described in my previous comment; they are supplementary coverage for |
Summary
This PR adds Kani verification for the
VecAPIs listed in Challenge 23.Coverage: 36 / 36 Challenge 23 entries targeted.
The verification combines:
contracts for unsafe raw-parts, length, append, and spare-capacity APIs;
executable harnesses for safe and internal
Vecoperations;symbolic
Vecstates with symbolic length, capacity, ranges, indices, and operation counts;loop contracts for operations that require induction;
explicit panic-path and allocation-growth coverage.
No non-verification runtime behavior is changed in normal builds.
Unbounded verification
Except for
extend_desugaredandextend_trusted, the Challenge 23 target harnesses do not impose a fixed element-count bound and do not use#[kani::unwind].The main proofs therefore operate over symbolic logical lengths rather than small fixed-size Vecs. Loops are either verified directly or discharged with loop contracts.
Non-ZST allocations are restricted by
MAX_ALLOCATION_BYTESso that one symbolic allocation remains representable in CBMC's object model. This is a verifier representation bound rather than a small semantic Vec-length bound. ZST logical lengths remain symbolic overusize.The two exceptions are documented limitations:
extend_desugaredis verified by bounded unwinding of the exact shipped implementation. An unbounded loop-contract proof is currently blocked by reallocation/free/provenance state across induction and by Kani issue #4796 for method calls in loop invariants.extend_trustedis also verified by bounded unwinding of the exact shipped implementation. Its loop is hidden insideIterator::for_each/fold; an explicit repeated-nextproof requires aTrustedLenstate relation that currently runs into the same loop-invariant limitations.These two proofs intentionally prefer bounded verification of the shipped implementation over claiming an unbounded proof of a verification-only replacement.
Contracts
Contract-backed APIs include:
Vec::from_raw_partsVec::from_partsVec::from_parts_inVec::set_lenVec::append_elementsVec::split_at_spare_mut_with_lenThe raw-parts contracts cover length/capacity consistency, alignment, allocation size, writable capacity, allocation-base/provenance requirements, and the initialized prefix.
For
from_raw_parts,from_parts, andfrom_parts_in, the exact one-past-allocation check requireskani::mem::is_inbounds. The tool-independent safety-contract API currently has no equivalent predicate, so only that clause useskani::requires; the remaining preconditions usesafety::requires.set_lenincludes both documented safety requirements:new_len <= capacityand initialization/validity of the newly exposed prefix when growing.Harness coverage
Harnesses cover:
raw-parts reconstruction and decomposition;
boxed-slice and array conversions;
truncate,swap_remove,insert,remove,push,push_within_capacity,pop, andclear;append,drain,split_off, andextract_if;extend_from_within,extend_with,spec_extend_from_within,extend_desugared, andextend_trusted;spare_capacity_mut,split_at_spare_mut, andsplit_at_spare_mut_with_len;Deref,DerefMut,IntoIterator, andDrop;normal and representative panic/overflow paths.
The current revision also covers both existing-spare and allocation-growth paths where applicable instead of assuming reallocation away.
Kani-only loop reshaping
Most targets execute the shipped implementation directly. Two functions still contain narrowly scoped Kani-only reshaping needed for unbounded loop contracts:
dedup_bypredeclares scalar/raw-pointer loop temporaries so they can be named byloop_modifies. The same values are recomputed before use, and predicate calls, drops, copies, branches, and length updates remain unchanged.extend_withexpresses the shippedfor _ in 1..nclone loop as an explicit counter loop. Both forms execute the samen - 1clone/write/length-update operations forn > 0, zero forn == 0, followed by the same final write.These are intended as semantics-preserving proof reshaping, not abstract replacements of the target behavior. They are documented in the source together with the pinned-Kani limitations that require them.
If required, supplementary shipped-text evidence can be added for these transcriptions.
append_elementsappend_elementsis verified with a direct#[kani::proof]rather than#[kani::proof_for_contract].Its call to
reservemay replace the backing allocation and free the old buffer. The pinned Kani function-contract interface can express writablemodifiesregions, but cannot express the correspondingfreesframe needed for reallocation. The direct harness therefore executes the real function while retaining both:no-growth executions; and
growth/reallocation executions.
The explicit non-overlap assumption is the documented unsafe-call precondition.
Symbolic Vec model
verifier_nondet_vecgenerates valid symbolic Vec states with symbolic capacity and logical length.Pre-existing element contents are intentionally treated as opaque and initialized with a shape-valid repeated byte pattern, avoiding another unbounded initialization loop before every target proof.
The verified Vec internals generally do not branch on those stored values. Predicate-driven APIs such as
retain_mut,dedup_by, andextract_ifuse nondeterministic predicate outcomes, over-approximating possible keep/remove/equality decision sequences. Values supplied through API arguments and iterators remain independently symbolic.Representative element shapes currently cover ZSTs, ordinary scalar layouts, validity-constrained
bool, and fixed-size aggregates.Generic
THarness bodies are written generically but Kani proof entry points are instantiated over representative concrete shapes.
Therefore this PR does not claim to literally satisfy the Challenge wording of "generic
T(no monomorphization)". This is the same tool-level/project-policy question discussed for Challenge 25 / #681 and other generic collection challenges.Challenge 23 naming
Some Challenge 23 names differ from the current repository API. This PR follows the checked-in source:
from_nonnull→Vec::from_partsfrom_nonnull_in→Vec::from_parts_inPrivate targets such as
append_elements,split_at_spare_mut_with_len,extend_with,spec_extend_from_within,extend_desugared, andextend_trustedare verified from insidevec::mod.Verification
All added Challenge 23 harnesses pass locally with the pinned Kani configuration.
The remaining documented limitations are primarily:
literal generic-
T/ no-monomorphization support;bounded shipped-code proofs for
extend_desugaredandextend_trusted;the two narrowly scoped Kani-only loop reshaping cases described above.
Resolves #284
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.