Repository navigation
Conversation
|
You should use macros to reduce code duplication. It'll also make review easier. |
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.
|
@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. |
|
@feliperodri All CI checks are green now, and I’ve addressed the non-blocking issues. This should be ready for another look. Thanks! |
|
Strengthen the Challenge 26 Kani safe functions' harnesses for The safe functions' harnesses now:
|
|
Added standalone verification coverage for the two previously uncovered safe functions:
The The These two harnesses use nondeterministic inputs bounded to at most four elements with With these additions, the PR now provides standalone coverage for all 54/54 safe functions listed in Challenge 26. |
feliperodri
left a comment
There was a problem hiding this comment.
Approving — leading solution for Challenge 26
Thanks @v3risec. After reviewing both open Challenge 26 (Rc/Weak) solutions with our vacuity tooling and local Kani (pinned 0.67.0 / CBMC 6.8.0), this is complete and sound. Prioritizing it.
Coverage against Ch26 criteria:
- A: 12/12 unsafe pub fns with real
#[requires]/#[ensures]/kani::modifiescontracts backed by matching#[kani::proof_for_contract]harnesses, monomorphized across primitive T (i8..i128, u8..u128, bool, unit, [u8;4]) and unsized[T]as challenge-allowed. - B: ~54/54 safe abstractions with unsafe (~100%, ≥ 75%). Complete coverage across new(_uninit/_zeroed/in), try*, pin/pin_in, into_array, get_mut/make_mut (with 3-state coverage), downcast (Ok+Err), from_box_in, RcFromSlice / ToRcSlice, Drop/Clone/Default, all From/TryFrom variants, Weak::{as_ptr,into_raw_with_allocator,upgrade,inner} (multi-path), inc_strong/inc_weak (+ should_panic overflow), UniqueRc + UniqueRcUninit.
Soundness (all clean):
- T1: no cfg body swaps — grep for
cfg(not(kani))in the diff returns 0 hits; harnesses run the real bodies. - T2: no trivial invariants, no
loop_invariant(true). - T7: all 12 unsafe fns have explicit
proof_for_contract;.github/workflows/kani.ymlautoharness allowlist unchanged — no silent autoharness dependency. - Not assume-the-conclusion — the only
kani::assume(can_dereference(...))isVec::set_len's own precondition inside a helper, not the fn under proof. - Inputs symbolic (
kani::any::<T>()); slice length bounded ≤100 (documented CI-tractability budget); iterator loopsunwind(6)for 4-element input (documented Kani loop-contract limitation). - No runtime std logic changed.
Local Kani sample (CBMC 6.8.0): 4/4 VERIFICATION SUCCESSFUL — harness_rc_assume_init_i8 (proof_for_contract), harness_rc_downcast_unchecked_i8 (proof_for_contract), harness_inc_strong_overflow_should_panic, harness_rc_default_str.
Minor weaknesses to note for the record (non-blocking):
verifier_nondet_vecusesptr::write_bytes(..., kani::any::<u8>(), size_of::<T>() * sz)— one symbolic byte pattern replicated across all elements, so a run overRc<[u32]>only covers[0x00000000; n],[0x01010101; n], … Symbolic but coverage-limited for multi-byte T.Rc::from_raw,Rc::increment_strong_count, andWeak::from_raw(non-_invariants) read(*ptr).get()insideunsafewithout an explicitkani::mem::can_dereferencein the requires, unlike the_invariants which do. Harmless for the roundtrip harnesses (pointers are always valid), but the exported contract is weaker than the_invariant for arbitrary callers — consider addingcan_dereferenceto match.Rc::get_mut_uncheckedcontract only requirescan_write(value); the documented safety property (no otherRc/Weakmay reference the inner value) is not encoded — the harness proves UB-freedom of the body but under-specifies the caller obligation.- Roundtrip-narrowed inputs (from_raw/increment/decrement use ptr obtained via into_raw) narrow the input universe vs a fully symbolic pointer meeting the precondition.
These are contract-strength refinements, not soundness bugs; they can be addressed in a followup. Challenge 26 explicitly permits bounded + primitive-mono.
|
@lucasccordeiro @rajath-mk @patricklam @HuStmpHrrr could you review this propose solution for challenge 26? |
- 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.
- Bound selected unsized harness inputs for stable CI verification. Not a verification limitation. - Preserve all ownership-state coverage.
Add semantic assertions and matching kani::cover properties. Check reference-count invariants, pointer identity, slice metadata, and weak-pointer ownership states across Rc-related harnesses. No production Rc implementation logic was changed.
Add bounded harnesses for RcFromSlice<T: Clone>::from_slice and ToRcSlice::to_rc_slice. Remove the Kani-specific loop contract and rewritten loop so the harnesses directly verify the real from_iter_exact implementation.
Add post-call kani::cover witnesses to all Rc and Weak proof_for_contract harnesses.
- Remove unreachable allocation-failure covers from the try_new* harnesses and document that Kani does not currently model allocation failure. - Restore the trailing newline in library/Cargo.lock and remove the unrelated lockfile diff. - Strengthen rc_raw_valid with same-allocation and implicit weak-count validation. - Mark Challenge 26 as resolved in the README and challenge book, linking PR model-checking#582, the Kani proof, and contributor information.
0dd2ef5 to
8f799d2
Compare
|
@feliperodri Thanks! I've rebased onto the latest A few updates:
No worries about the delay, and thanks for the continued reviews! |
|
@v3risec, an update after re-reviewing #587 (Ch27, Arc), so the two are judged the same way. This PR has the same UB-list gaps as #587 (which also leaves out data races):
#587 is now approved as sound proofs toward Ch27 that don't solve the challenge, and this PR should be treated the same way, so I'm no longer holding it on these items. You can address them in follow-up PRs. Whether solutions must cover the first one, which Kani can't check, is being discussed in #733. |
Update on
|
feliperodri
left a comment
There was a problem hiding this comment.
Re-approving on the current commit (fa0abd4). Criteria 1 and 2 are met and the proofs are sound. Challenge 26 doesn't list data races. The two UB items Kani can't establish here ("mutating immutable bytes" and "producing an invalid value") are with the committee in #733, so whether this resolves the challenge depends on that ruling.




Summary
This PR adds Kani-based verification artifacts for
Rc/Weaksafety inlibrary/alloc/src/rcs/rc.rsfor Challenge 26.The change introduces:
#[cfg(kani)]for all 12 required unsafe functions and all 54 listed safe functions;kani::coverproperties to demonstrate that the checked paths are reachable;Rc<[T]>/Weak<[T]>paths can be exercised in a reusable way.No 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: 54 / 54 (100%)
This exceeds the challenge threshold (>= 75%).
Covered safe functions (54/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_sliceRcFromSlice<T: Clone>::from_sliceToRcSlice<T, I>::to_rc_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>>::fromTryFrom<Rc<[T],A:Allocator>>::try_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_weakNote
RcFromSlice<T: Clone>::from_sliceimplementation with a manually implemented non-trivialClonetype, ensuring that the harness does not dispatch to theTrivialClonespecialization.ToRcSlice<T, I>::to_rc_slicethrough theFromIteratorand exact-sizeTrustedLenpath.Rc::from_iter_exactimplementation and its original element-writing loop. The target implementation is not replaced with acfg(kani)-specific loop.Three 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.kani::coverproperties after the corresponding assertions to show that the verified paths are not vacuous.?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.
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.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.