Challenge 29: Verify Boxed safety in alloc::boxed, alloc::boxed::convert and alloc::boxed::thin with Kani - #589
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Kani-based verification artifacts for Challenge 29 across alloc::boxed (including ThinBox) by introducing Kani harness modules and attaching Kani-only contracts (cfg_attr(kani, ...)) to selected unsafe APIs, without altering normal runtime behavior.
Changes:
- Adds Kani preconditions/postconditions to boxed raw-pointer reconstruction and unchecked downcast APIs.
- Introduces extensive
#[cfg(kani)]proof harness modules forBox,ThinBox, and header utilities across representative concrete instantiations. - Updates
library/Cargo.lockto include thesafetyproc-macro crate (and its transitive deps) in the lockfile dependency graph.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| library/alloc/src/boxed.rs | Adds Kani-only contracts for unsafe Box APIs and a large set of Kani proof harnesses/helpers for Challenge 29 coverage. |
| library/alloc/src/boxed/convert.rs | Adds Kani-only preconditions to downcast_unchecked variants and adds harnesses for boxed conversions/downcasts. |
| library/alloc/src/boxed/thin.rs | Adds Kani proof harnesses for ThinBox deref/drop/meta/header-related behavior. |
| library/Cargo.lock | Lockfile updates to reflect safety crate usage and its proc-macro dependencies. |
|
Thanks for the thoughtful review. We have addressed the review comments about grouping Kani harnesses. Changes made:
One note on the import-path review comment in |
feliperodri
left a comment
There was a problem hiding this comment.
Challenge 29 (Boxed) — Kani verification review of PR #589
Verdict: sound, no fatal issues; a few non-blocking concerns
This is a large, carefully-structured submission (macro-generated harnesses over primitive instantiations, per the challenge's "primitive types only" allowance). I could not find any soundness-breaking pattern.
Soundness checklist
- cfg-swap vacuity (FATAL): none. All contracts are added via
#[cfg_attr(kani, kani::requires(...))]/kani::ensures(...); no#[cfg(not(kani))]body substitution exists. No trivially-passing harnesses. - Assume documented precondition vs. assume-the-conclusion: legitimate. For
downcast_uncheckedthe harnesses dokani::assume(erased.is::<$ty>())(convert.rs~2159/2201/2247) — this restates the documented precondition, and sinceerasedgenuinely holds$tythe assume is always satisfiable (not over-constrained, not conclusion-assuming). - Genuinely symbolic inputs.
verifier_nondet_vec(boxed.rs299-315) builds symbolic-length, symbolic-byte vectors; slice harnesses usekani::any_where(|l| box_slice_layout_ok::<T>(*l))keeping length unbounded/symbolic.into_array/try_fromfixN=100but keepvec.len()symbolic, so both thelen==Nandlen!=Nbranches are explored (boxed.rs1042-1058,convert.rs2351-2367). Not concrete-only unit tests. - Faithful, truly-verified contracts on the raw-pointer reconstruction fns.
from_raw,from_non_null,from_raw_in,from_non_null_ingetrequires(!null && checked_align_of_raw.is_some() && size <= isize::MAX && can_dereference)plusensures((&**result) as *const T == raw)(boxed.rs204-283), and are verified with#[kani::proof_for_contract(...)]across sized + unsized (slice) instantiations. These preconditions are the actual documented safety requirements — non-trivial and meaningful.
Success-criteria coverage
- The 9 required unsafe fns are all addressed.
assume_init(sized + slice),from_raw,from_non_null,from_raw_in,from_non_null_in, and the threedowncast_unchecked. - Targeting is correct — the challenge doc table is stale. The table lists
<dyn Error>::downcast_unchecked, but I verified (grep -rn "fn downcast_unchecked" library/) that no such function exists: inalloc::boxed::convertthe onlydowncast_uncheckedmethods are the three onBox<dyn Any, A>(convert.rs394/453/512), andimpl dyn Error(convert.rs736-779) exposes only the safedowncast. PR #589 added contracts + harnesses to exactly those three realBox<dyn Any>::downcast_uncheckedfunctions — this is the right target, contrary to the task note's premise. It additionally verifies the safe<dyn Error>::downcastfamily (ok/err paths,convert.rs2617-2757). - ThinBox/WithHeader family fully covered in
thin.rs:Deref,DerefMut,Drop,meta,with_header,WithHeader::{new, try_new, new_unsize_zst, header}across sized/dyn Any/slice instantiations. (This is exactly what the competing #573 lacked.) - Safe-fn coverage is well above the 75% bar — essentially every entry in the safe table has harness(es), including
Drop,Default,Clone,into_pin(with a!Unpinsentinel),leak,into_unique, all the*_slice_in/try_new_*constructors, and the convertFrom/TryFrompaths.
Non-blocking issues (worth addressing before merge)
- Contract style deviates from the repo convention (upstreamability). The repo uses the tool-agnostic
safetycrate everywhere:grep -rln "use safety::" library/core/src library/alloc/src= 46 files, whilecfg_attr(kani, kani::requires= 0. This PR hardcodes#[cfg_attr(kani, kani::requires/ensures(...))](e.g.boxed.rs176-215,convert.rs2044/2052/2060). It works under Kani (run-kani passes-Z function-contracts), so verification validity is unaffected, but maintainers will likely wantuse safety::{requires, ensures};for consistency and upstreamability (a stated review criterion). - 5 of the 9 unsafe fns' contracts are not verified via
proof_for_contract.assume_init(×2) anddowncast_unchecked(×3) carry contracts but are exercised with plain#[kani::proof]that restate the precondition viakani::assume/by construction (documented as Kani limitations: MaybeUninit generic impl, and trait-object generic method target resolution —boxed.rs358-365,convert.rs2140-2147). The concrete safety (UB-freedom of the real body) is verified, but the contract text is not machine-linked to the proof, so it could drift without a proof failing. The 4 pointer-reconstruction fns are the ones with trueproof_for_contractlinkage. Consider noting this gap explicitly, or filing/referencing the Kani limitation issues. - Minor coverage gap: I did not find a dedicated harness for
<Box<[T]> as BoxFromSlice<T>>::from_slicefrom the safe table. Coverage remains far above 75%, so non-blocking.
Copilot review notes — mostly reject/stale
- Copilot's two comments that
super::super::kani_box_harness_helpers"won't compile / resolves toalloc::…" are incorrect.convert/thinare declaredmod convert;/mod thin;insideboxed.rs(boxed.rs212/216), so fromalloc::boxed::convert::verify,super::super=alloc::boxed, and the path correctly reachesalloc::boxed::kani_box_harness_helpers. Its suggested fix (super::kani_box_harness_helpers) would point at a non-existentalloc::boxed::convert::kani_box_harness_helpersand break the build. Do not apply. - Copilot's "consolidate the many numbered
verify_170/verify_944modules" comments are stale — nomod verify_NNNexists in the current diff (grep -rn "mod verify_[0-9]" library/= empty); the PR already uses a single#[cfg(kani)] mod verifyper file, matching the repo convention it cites.
Bottom line
Sound, vacuity-free, correctly-targeted, and exceeds the coverage bar, with the ThinBox/WithHeader family that the competing PR lacked. Recommend COMMENT: address the safety-crate contract style and clarify the non-proof_for_contract subset before approval; no changes are required for verification soundness.
Use tool-agnostic safety contract attributes and document the limitations of the plain assume_init and downcast_unchecked proofs. Restore library/Cargo.lock to remove the unrelated lockfile update.
|
@feliperodri Thank you for the thorough review. I have addressed the two follow-up concerns in a new commit:
I updated the PR summary to explicitly distinguish which unsafe functions are verified with Please let me know if there are any other changes you would like me to make. |
Summary
This PR adds Kani-based verification artifacts for
Box,ThinBox, and related boxed conversion APIs inlibrary/alloc/src/boxed.rs,library/alloc/src/boxed/convert.rs, andlibrary/alloc/src/boxed/thin.rsfor Challenge 29.The change introduces:
#[cfg(kani)]for the required unsafe functions and a broad safe-function subsetNo non-verification runtime behavior is changed in normal builds.
Notes on Challenge 29 Function Signatures
Several Challenge 29 entries do not exactly match the current repository source. This PR follows the actual checked-in API signatures rather than the likely stale or imprecise challenge text.
Notable mismatches include:
Box<T, A>::new_uninit_slice_in,Box<T, A>::new_zeroed_slice_in,Box<T, A>::try_new_uninit_slice_in, andBox<T, A>::try_new_zeroed_slice_in, but the current source implements them onBox<[T], A>and returns boxed[MaybeUninit<T>]slices.<Box<[T; N]> as TryFrom<Box<T>>>::try_fromdoes not match the source. The current implementation is<Box<[T; N]> as TryFrom<Vec<T>>>::try_from.<dyn Error>::downcast_uncheckedvariants in the challenge text, but the current source provides unchecked downcast APIs forBox<dyn Any, A>,Box<dyn Any + Send, A>, andBox<dyn Any + Send + Sync, A>.T: ?Sized,T: Clone, allocator bounds, or specialization-related constraints. The harnesses use the bounds from the actual source definitions.Verification Coverage Report
Unsafe functions
Coverage: 9 / 9 (100%)
Verified set includes:
Box<mem::MaybeUninit<T>, A>::assume_initBox<[mem::MaybeUninit<T>], A>::assume_initBox<T>::from_rawBox<T>::from_non_nullBox<T, A>::from_raw_inBox<T, A>::from_non_null_inBox<dyn Any, A>::downcast_uncheckedBox<dyn Any + Send, A>::downcast_uncheckedBox<dyn Any + Send + Sync, A>::downcast_uncheckedFor the 9 required unsafe functions:
from_raw,from_non_null,from_raw_in, andfrom_non_null_inare verified with#[kani::proof_for_contract(...)], directly linking the proof harnesses to their function contracts.assume_initvariants and the threedowncast_uncheckedvariants are verified with plain#[kani::proof]harnesses under their documented preconditions. These cannot currently useproof_for_contractdue to Kani limitations aroundMaybeUninitgeneric impls and generic methods on trait-object receivers.Safe functions
Coverage: 45 / 46 (97.8%)
This exceeds the Challenge 29 threshold of at least 75%.
Covered safe functions include APIs from the following groups:
Allocation and initialization
Box<[T], A>::new_uninit_slice_inBox<[T], A>::new_zeroed_slice_inBox<[T], A>::try_new_uninit_slice_inBox<[T], A>::try_new_zeroed_slice_inBox<mem::MaybeUninit<T>, A>::writeRaw pointer and ownership conversion
Box<T>::into_non_nullBox<T, A>::into_raw_with_allocatorBox<T, A>::into_non_null_with_allocatorBox<T, A>::into_uniqueBox<T, A>::leakBox<T, A>::into_pinTrait implementations for
Box<Box<T, A> as Drop>::drop<Box<T> as Default>::default<Box<str> as Default>::default<Box<T, A> as Clone>::clone<Box<str> as Clone>::clone<Box<str> as From<&str>>::from<Box<[u8], A> as From<Box<str, A>>>::from<Box<[T; N]> as TryFrom<Box<[T]>>>::try_from<Box<[T; N]> as TryFrom<Vec<T>>>::try_fromBox<dyn Any>andBox<dyn Error>conversionsBox<dyn Any, A>::downcastBox<dyn Any + Send, A>::downcastBox<dyn Any + Send + Sync, A>::downcast<dyn Error>::downcast<dyn Error + Send>::downcast<dyn Error + Send + Sync>::downcastThinBoxandWithHeader<ThinBox<T> as Deref>::deref<ThinBox<T> as DerefMut>::deref_mut<ThinBox<T> as Drop>::dropThinBox<T>::metaThinBox<T>::with_headerWithHeader<H>::newWithHeader<H>::try_newWithHeader<H>::new_unsize_zstWithHeader<H>::headerNot listed as a standalone harness target:
<Box<[T]> as BoxFromSlice<T>>::from_sliceApproach
The verification strategy combines contracts for unsafe entry points with executable proof harnesses:
kani::requirespreconditions for pointer non-nullness, layout compatibility, dereferenceability, and initialization where expressible.#[kani::proof_for_contract]for the required unsafe APIs.#[kani::proof]harnesses for safe APIs across allocation, conversion, trait implementation, downcast,ThinBox, andWithHeaderbehavior.bool, unit, arrays, slices, strings,dyn Any,dyn Error, and allocator-awareGlobalcases.cfg(kani)so normal std behavior is unchanged.Scope assumptions
Tis instantiated with representative concrete types allowed by the challenge.Global.Verification
All added Challenge 29 harnesses pass locally with Kani.
Resolves #526
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.