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 All CI checks are green now, and I’ve addressed the non-blocking issues. This should be ready for another look. Thanks! |
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.
|
Strengthen the Challenge 26 Kani safe functions' harnesses for The safe functions' harnesses now:
|
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.
|
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. |
Add post-call kani::cover witnesses to all Rc and Weak proof_for_contract harnesses.
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? |
|
checking now |
| let offset = unsafe { data_offset(ptr) }; | ||
| let strong_ptr = unsafe { ptr.byte_sub(offset) as *const Cell<usize> }; | ||
| unsafe { &raw const *strong_ptr } | ||
| }))] |
There was a problem hiding this comment.
do we not want to assert then increment of the count?
There was a problem hiding this comment.
Fixed. The contract now snapshots the strong count with old(...) and specifies that, on normal return, the resulting strong count is exactly the previous count plus one:
strong_before.checked_add(1) == Some(strong_after).
The modifies clause is also restricted to the strong-count cell.
| let offset = unsafe { data_offset(ptr) }; | ||
| let strong_ptr = unsafe { ptr.byte_sub(offset) as *const Cell<usize> }; | ||
| unsafe { &raw const *strong_ptr } | ||
| }))] |
There was a problem hiding this comment.
Fixed. The contract now snapshots the pre-state and specifies the exact decrement:
strong_before.checked_sub(1) == Some(strong_after).
The strong_before == 1 case is intentionally handled separately in the postcondition. A final decrement may drop T and deallocate the backing allocation, so it is not sound to dereference the old strong-count pointer afterward.
| @@ -1629,6 +1697,26 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> { | |||
| /// } | |||
| /// ``` | |||
| #[unstable(feature = "allocator_api", issue = "32838")] | |||
| #[requires({ | |||
| let offset = unsafe { data_offset(ptr) }; | |||
There was a problem hiding this comment.
this block seems repetitive; do we have a way to modularize it?
There was a problem hiding this comment.
Refactored. The repeated raw-pointer reconstruction, layout, and reference-count checks are now factored into shared Kani-only helpers such as rc_raw_parts, rc_raw_layout_valid, rc_raw_valid, weak_raw_layout_valid, weak_raw_valid, and weak_raw_count_snapshot.
This also lets Weak::from_raw and Weak::from_raw_in share the same validity model instead of duplicating slightly different checks.
| @@ -3214,6 +3350,42 @@ impl<T: ?Sized> Weak<T> { | |||
| /// [`new`]: Weak::new | |||
| #[inline] | |||
| #[stable(feature = "weak_into_raw", since = "1.45.0")] | |||
| #[requires({ | |||
| let is_sentinel = is_dangling(ptr); | |||
There was a problem hiding this comment.
is it semantically sound to permit dangling pointer? what is the motivation?
There was a problem hiding this comment.
The allowed dangling case is not an arbitrary dangling allocation pointer. It is specifically the usize::MAX sentinel representation used by Weak::new / Weak::new_in.
That sentinel has no backing RcInner, and Weak::from_raw{,_in} explicitly supports raw pointers produced from an empty Weak. The contract now makes this distinction explicit: weak_raw_valid accepts the sentinel directly, while every non-sentinel pointer must satisfy the normal allocation/layout/reference-count checks.
|
@feliperodri @HuStmpHrrr Thanks for the detailed review. I went through the comments and updated the contracts and harnesses accordingly. The main changes are:
I also noticed that the Kani CI jobs are currently failing while setting up Kani/CBMC, before reaching the verification itself. The failure comes from Homebrew rejecting the Is this a known CI/infrastructure issue at the moment, or is there anything I should change on my side? I'm happy to make any further changes or refinements if needed. Thanks again for the review! |




Summary
This PR adds Kani-based verification artifacts for
Rc/Weaksafety inlibrary/alloc/src/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.