Challenge 26: verify Rc/Weak safety in alloc::rc with Kani - #582
Challenge 26: verify Rc/Weak safety in alloc::rc with Kani#582v3risec wants to merge 24 commits into
Conversation
|
You should use macros to reduce code duplication. It'll also make review easier. |
… harnesses with rust macro
- fix verify_4533 slice harness generation - rename duplicate UniqueRcUninit drop macro - add unstable(kani) annotations to verify modules - keep production from_iter_exact loop under non-Kani builds - make nondet Vec helper initialize elements soundly
058866b to
436a1b4
Compare
|
Thanks for the thoughtful review. We have addressed all 5 comments and pushed 3 follow-up commits with the requested changes. The PR should now be ready for another round of CI and review. Could you please re-run the CI checks when possible? |
|
Update on the CI resource issue: The recent changes add a macOS-only bound to the nondeterministic slice/vector length used by the shared The bound is guarded by The intent is to keep the macOS CI jobs within their time/memory budget, not to change normal std behavior or the Linux verification setup. |
feliperodri
left a comment
There was a problem hiding this comment.
Challenge 26 (Rc/Weak) — verification-soundness review
This is genuine, substantial verification work: 112 #[kani::proof] + 20 #[kani::proof_for_contract] (the macros expand these across ~13 primitive/slice instantiations each, yielding the full 132/24 harness counts). Both success criteria are met and the work is sound. I'm landing on COMMENT for a few non-blocking issues a maintainer should weigh before merge.
Soundness checklist — all clear
- cfg-swap vacuity — BENIGN. The only
#[cfg(not(kani))](library/alloc/src/rc.rs, diff line 434) rewrites thefrom_slicefor (i,item) in iter.enumerate()loop into an equivalentwhile let Some(item) = iter.next()under#[cfg(kani)], purely to attach#[kani::loop_invariant(i == guard.n_elems)]andloop_modifies. Runtime path keeps the upstream body; the Kani body performs identicalptr::write+n_elemsbookkeeping. This is the accepted for→while rewrite pattern, not a fatal body swap. The othercfg(not(...))areno_global_oom_handling(upstream) andtarget_os="macos"harness disables (resource limits) — benign. - No assume-the-conclusion. proof_for_contract harnesses construct a valid
Rc/Weakfromkani::any()and derive the raw pointer viainto_raw/into_raw_with_allocator, so the precondition holds by construction rather than by assuming the conclusion. - No trivial invariants. Only
loop_invariant(i == guard.n_elems)— meaningful. Noinvariant(true),requires(true), orassume(false). - Contract-liveness (T7) — real & faithful. All 10
proof_for_contract-verified unsafe fns target contracts newly added in this diff. Contracts encode provenance/refcount preconditions with the correct predicates:from_raw/from_raw_in(diff ~1333, ~1630) requireptr::addr_eqto the rebuilt inner pointer,kani::mem::checked_size_of_raw/checked_align_of_rawmatch, andstrong >= 1;get_mut_unchecked(~1955) requirescan_writeand ensures result aliases the value;downcast_unchecked(~2204) requires(*self).is::<T>()(exactly the documented precondition);Weak::from_raw{,_in}(~3317/~3534) correctly special-case theis_danglingsentinel plusweak > 0andsame_allocation. These are faithful, not decorative, not over-constrained. - Symbolic, not concrete. Values are
kani::any(); pointers are symbolic in Kani's model. Slice lengths are symbolic within a bound. - Bounded vs unbounded. Slice harnesses bound length (
kani::assume(len <= 1024)/sz <= 1024inverifier_nondet_vec). Acceptable for tractability; challenge permits primitive types only. - Success criteria — MET. (a) All 12 required unsafe fns have contracts: 10 verified via
proof_for_contract; the twoassume_initvariants carry contracts (requires can_dereference,ensures strong_count >= 1) verified via#[kani::proof]with an explicit postcondition assertion — justified by both the Kani 0.65MaybeUninit-impl path-resolution limitation and the challenge's own note that "showing something is initialized … may be impossible to express." (b) Safe-fn coverage spans essentially the entire ~53-entry list (new/new_uninit/new_zeroed/try_* families, pin/pin_in, into_array, get_mut, make_mut, downcast, from_box_in, From impls, Drop/Clone/Default, UniqueRc/UniqueRcUninit, Weak::{as_ptr,upgrade,inner,into_raw_with_allocator}, RcInnerPtr::inc_*), well above 75%. Branch-split harnesses (unique/shared/weak-present; success/failure; live/strong_zero/dangling) show real thought.
This is clearly superior to the competing #574 (zero contracts, concrete-only inputs).
Non-blocking issues to address
- Convention: uses raw
kani::attributes, not the tool-agnosticsafetycrate. All contracts are#[cfg_attr(kani, kani::requires(...))]/kani::ensures/kani::modifieswithuse core::kani. PerCLAUDE.md/library/contracts/safety, contracts are supposed to useuse safety::{requires, ensures}so they are tool-agnostic and plausibly upstreamable. As written these contracts are Kani-only and diverge from every other merged challenge. Recommend porting to thesafetyattributes. (Note theCargo.lockchurn addingsafety/proc-macro-errorto core/alloc deps — reconcile with the branch's existing safety integration.) assume_initcontracts are present but notproof_for_contract-enforced. The harnesses (verify_1198/verify_1239) run the real body on constructed-valid inputs and manually assert the postcondition, which is sound. But the code comment's claim that "the requires clause is still checked as an assertion at the call site" is imprecise — a contract on a normally-called (non-stubbed, non-target) function is inert in Kani; the requires is satisfied here only because the input is constructed valid, not independently checked. Reword the comment to avoid over-claiming.- Roundtrip inside a
requiresclause. Theincrement_strong_count/increment_strong_count_inpreconditions (diff ~1408, ~1760) buildRc::from_raw(ptr)and callinto_rawinside therequiresto compare addresses. It nets out refcount-neutral and evidently passes CI, but embedding construct/consume logic in a precondition is fragile; consider simplifying to the pure provenance/addr_eqchecks used by the other contracts.
Net: sound, faithful, meets both criteria. The above are quality/convention items, not soundness defects.
- Migrate Rc and Weak contracts to tool-agnostic safety attributes. - Clarify that regular assume_init proofs do not activate callee contracts. - Explicitly mirror the expressible assume_init preconditions and postconditions. - Remove raw Rc roundtrips from pointer contract preconditions.
|
@feliperodri Thanks for the detailed review. I’ve addressed the three non-blocking points:
Please let me know if there are any other changes you would like me to make. |
- Bound selected unsized harness inputs for stable CI verification. Not a verification limitation. - Preserve all ownership-state coverage.




Summary
This PR adds Kani-based verification artifacts for
Rc/Weaksafety inlibrary/alloc/src/rc.rsfor Challenge 26.The change introduces:
#[cfg(kani)]for 12 required unsafe functions and a broad safe-function subsetRc<[T]>/Weak<[T]>paths can be exercised in a reusable wayNo non-verification runtime behavior is changed in normal builds.
Verification Coverage Report
Unsafe functions (required by Challenge 26)
Coverage: 12 / 12 (100%)
Verified set includes:
Rc<mem::MaybeUninit<T>,A>::assume_initRc<[mem::MaybeUninit<T>],A>::assume_initRc<T:?Sized>::from_rawRc<T:?Sized>::increment_strong_countRc<T:?Sized>::decrement_strong_countRc<T:?Sized,A:Allocator>::from_raw_inRc<T:?Sized,A:Allocator>::increment_strong_count_inRc<T:?Sized,A:Allocator>::decrement_strong_count_inRc<T:?Sized,A:Allocator>::get_mut_uncheckedRc<dyn Any,A:Allocator>::downcast_uncheckedWeak<T:?Sized>::from_rawWeak<T:?Sized,A:Allocator>::from_raw_inSafe functions (Challenge 26 list)
Coverage: 51 / 54 (94.4%)
This exceeds the challenge threshold (>= 75%).
Covered safe functions (51/54), grouped by API category:
Allocation
Rc<T>::newRc<T>::new_uninitRc<T>::new_zeroedRc<T>::try_newRc<T>::try_new_uninitRc<T>::try_new_zeroedRc<T>::pinRc<T,A:Allocator>::new_uninit_inRc<T,A:Allocator>::new_zeroed_inRc<T,A:Allocator>::new_cyclic_inRc<T,A:Allocator>::try_new_inRc<T,A:Allocator>::try_new_uninit_inRc<T,A:Allocator>::try_new_zeroed_inRc<T,A:Allocator>::pin_inSlice
Rc<[T]>::new_uninit_sliceRc<[T]>::new_zeroed_sliceRc<[T]>::into_arrayRc<[T],A:Allocator>::new_uninit_slice_inRc<[T],A:Allocator>::new_zeroed_slice_inRcFromSlice<T: Copy>::from_sliceConversion and pointer
Rc<T:?Sized, A:Allocator>::innerRc<T:?Sized, A:Allocator>::into_inner_with_allocatorRc<T,A:Allocator>::try_unwrapRc<T:?Sized,A:Allocator>::into_raw_with_allocatorRc<T:?Sized,A:Allocator>::as_ptrRc<T:?Sized,A:Allocator>::get_mutRc<T:?Sized+CloneToUninit, A:Allocator+Clone>::make_mutRc<T:?Sized,A:Allocator>::from_box_inRc<dyn Any,A:Allocator>::downcastTrait implementations (Rc)
Clone<T: ?Sized, A:Allocator>::clone for RcDrop<T: ?Sized, A:Allocator>::drop for RcDefault<T:Default>::defaultDefault<str>::defaultFrom<&str>::fromFrom<Vec<T,A:Allocator>>::fromFrom<Rc<str>>::fromWeak and traits
Weak<T:?Sized,A:Allocator>::as_ptrWeak<T:?Sized,A:Allocator>::into_raw_with_allocatorWeak<T:?Sized,A:Allocator>::upgradeWeak<T:?Sized,A:Allocator>::innerDrop<T:?Sized, A:Allocator>::drop for WeakUniqueRc and traits
UniqueRc<T:?Sized,A:Allocator>::into_rcUniqueRc<T:?Sized,A:Allocator+Clone>::downgradeDeref<T:?Sized,A:Allocator>::derefDerefMut<T:?Sized,A:Allocator>::deref_mutDrop<T:?Sized, A:Allocator>::drop for UniqueRcUniqueRcUninit<T:?Sized, A:Allocator>::newUniqueRcUninit<T:?Sized, A:Allocator>::data_ptrDrop<T:?Sized, A:Allocator>::drop for UniqueRcUninitRefcount internals
RcInnerPtr::inc_strongRcInnerPtr::inc_weakNot yet listed as standalone harness targets (3/54):
RcFromSlice<T: Clone>::from_sliceToRcSlice<T, I>::to_rc_sliceTryFrom<Rc<[T],A:Allocator>>::try_fromThree Criteria Met (Challenge 26)
Tis instantiated with allowed representative concrete types, and allocator-focused proofs are limited to standard-library allocator scope (Global).Approach
The verification strategy combines contracts for unsafe entry points with executable proof harnesses:
requirespreconditions for pointer validity, alignment soundness, same-allocation checks, and refcount well-formedness.ensures) and mutation footprints (kani::modifies) for refcount-changing operations.#[kani::proof_for_contract(...)]harnesses for all required unsafe functions, and regular#[kani::proof]harnesses for the covered safe functions.?Sizedslice-based functions.Rc<[T]>/Weak<[T]>constructions without duplicating per-harness setup logic.cfg(kani)so normal std behavior is unchanged.Scope assumptions (per challenge allowance)
i8..i128,u8..u128),bool,(), arrays, vectors, slices,str, and trait objects (dyn Any).Global(both explicitRc<_, Global>/Weak<_, Global>and defaultRc/Weakaliases).Verification
All harnesses in this PR pass locally with Kani 0.65.
Platform-specific CI tractability note
The shared nondeterministic vector helper now bounds the symbolic length
to <= 100for CI resource stability. This is only a verification-time tractability bound for shared CI runners; it is not a safety condition or a function-behavior assumption. The bound can be removed for local verification to restore the intended unbounded input space.Resolves #382
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.