Challenge 12: safety and correctness of NonZero - #637
Conversation
Part 1 (new / new_unchecked): an #[ensures] contract on NonZero::new
verifying the layout precondition backing its transmute_unchecked
(size_of::<T>() == size_of::<Option<NonZero<T>>>()), that a NonZero is
produced iff the input is nonzero (2a), and that the inner value equals
the input (2b) — with proof_for_contract harnesses across all 12
integer widths. from_mut is verified by plain harnesses with in-body
assertions (its returned Option<&mut Self> mutably aliases the input,
so an #[ensures] reading both would introduce an aliasing hazard).
Part 2 (36 functions): tool-agnostic safety::{requires,ensures}
contracts plus per-width Kani harnesses in nonzero.rs mod verify —
checked/saturating mul, pow and add, the neg and abs families,
count_ones, bit operations (swap_bytes, reverse_bits, rotations,
endian conversions), midpoint, isqrt, and checked_next_power_of_two.
Trait-generic items Kani cannot attach contracts to (max/min/clamp via
Ord — cf. model-checking#202 — and the three const BitOr impls) are covered by
direct harnesses with assertions instead. Partial methods (abs, neg)
use paired value/should_panic harnesses so both the defined and the
panicking (MIN) domains are covered.
The placeholder #[safety::loop_invariant(true)] on checked_pow's loop
(int_macros.rs / uint_macros.rs, added in model-checking#327) is strengthened to
`self == 0 || (acc > 0 && base > 0)` (unsigned) and
`self == 0 || (acc != 0 && base != 0)` (signed). Under
-Z loop-contracts a `true` invariant havocs the accumulator and makes
every nonzero-dependent caller unverifiable; the strengthened
invariant is inductive and discharges NonZero::checked_pow's
new_unchecked obligation with unbounded, full-domain harnesses on
every width. Trade-off, documented in-code: only invariant-derived
facts (nonzero-ness) are provable about the loop's result, so the two
pow contracts state the safety property rather than exact-value
equality (a functional invariant would need ghost state for the
original exponent).
Every assume-bearing harness macro carries a non-vacuity witness;
unchecked_mul's interval harnesses cover the contract precondition
itself (cover(x.checked_mul(y).is_some())) so an interval pairing
that cannot satisfy the assumed #[requires] fails loudly instead of
verifying vacuously. The isqrt wide-width interval strategy is
documented as an explicit known verification gap, with mid-band
harnesses at the root's half-width transition narrowing it.
All 498 harnesses in num::nonzero::verify verified
(VERIFICATION: SUCCESSFUL) via scripts/run-kani.sh.
Towards model-checking#71.
By submitting this pull request, I confirm that my contribution is
made under the terms of the Apache 2.0 and MIT licenses.
f3a666a to
e28529c
Compare
feliperodri
left a comment
There was a problem hiding this comment.
Approve — strongest of the Challenge 12 solutions, no known correctness gap
Reviewed against the challenge criteria (issue #71) and compared with the other open Challenge-12 PRs (#565, #600, #544; plus the complementary perf refactor #624). This is the solution I'd upstream.
Verified against the diff
- No vacuity. 0
cfg(kani)/cfg(not(kani))body-swaps in the added lines — the harnesses exercise the real std bodies, not a hand-written abstraction. - Part 1 done as mandated. Real
#[kani::proof_for_contract(NonZero::new)]with the sanctioned transmute size-equality stand-in plus the 2a (created-iff-nonzero) and 2b (value-preservation) properties.new_uncheckedkeeps its verified contract. This is the only open solution that discharges Part 1 with verified contracts rather than plain proofs. - Broad verified coverage. 32
#[kani::proof_for_contract]with functional#[ensures]per integer width (e.g.result.get() == old(self).get().swap_bytes(),from_be/to_le), beyond the safety-only minimum. checked_powhandled correctly. The placeholderloop_invariant(true)is strengthened toself == 0 || (acc != 0 && base != 0)(> 0unsigned), not reverted — keepingpowunbounded (no exponent cap) and preserving the #327 annotation.abs/negatMINfully verified. Each carries a normal harness (assume != MIN) plus a#[kani::should_panic]MIN harness proving the overflow panic — no over-restrictive#[requires(!=MIN)]mis-statement.- Non-vacuity discipline. 11
kani::coverwitnesses confirm the restricted paths are reachable, and the fix to the pre-existingunchecked_mulharnesses (pairing each extreme interval with a small one) removes 16 previously-vacuous proofs — exactly the kind of empty-proof this review campaign has been screening for. - Trait-generic items Kani can't contract (
max/min/clamp, the threeBitOrimpls,from_mut) are covered with directsafety-crate harnesses.
Note on evidence
This approval is based on static + diff verification and the prior local-Kani run data from the review campaign; I did not re-run the full harness suite locally for this pass. CI (kani.yml) gates the 498 harnesses, and the author reports all pass via scripts/run-kani.sh.
Nice work — this is the first Challenge-12 submission with no known correctness gap.
|
Hi, committee members (@celinval @rahulku @pnkfelix @zhassan-aws @remi-delmas-3000 @qinheping @tautschnig @patricklam @ranjitjhala @carolynzech @robdockins @HuStmpHrrr @Eh2406 @jswrenn @havelund @jorajeev @rajath-mk @thanhnguyen-aws). This PR is ready for a second look. I've reviewed and approved it: it's the strongest of the open Challenge-12 submissions, meets Part 1's verified-contract requirement, covers all 36 Part 2 functions, and has no known correctness gap AFAIK. Since it needs a committee review to merge, does anyone have the bandwidth to take a pass? |
Towards #71. Solves Challenge 12: Safety of NonZero: contracts and Kani harnesses for
NonZero<T>, covering Part 1 (new/new_unchecked) and all 36 Part 2 functions. Since Kani checks each concrete type separately, every function gets one harness perNonZerotype it's defined on (NonZeroI8throughNonZeroUsize). All 498 harnesses innum::nonzero::verifypass viascripts/run-kani.sh.Changes
nonzero.rsnewand the Part 2 methods; harnesses inmod verify; fix for pre-existingunchecked_mulharnesses that were passing without checking anything (see below)int_macros.rs,uint_macros.rschecked_pow's placeholderloop_invariant(true)strengthened to `self == 0Part 1
The contract on
newstates the size equality the challenge accepts in place of full transmute verification (size_of::<T>() == size_of::<Option<NonZero<T>>>()), plus the two required correctness properties: aNonZerois created if and only if the input is nonzero (2a), and the inner value equals the input (2b). Verified with#[kani::proof_for_contract]for all 12 types;new_uncheckedkeeps its existing verified contract.Part 2
Each function gets a
safety::{requires,ensures}contract and per-type harnesses. The common safety property: the value passed to the internalnew_uncheckedis never zero (ruling out the "producing an invalid value" UB). Most contracts also state the exact result value.count_ones, bit ops (swap_bytes,reverse_bits,rotate_*,from_be/le,to_be/le),checked/saturating_add,checked_next_power_of_two,midpoint,checked/overflowing/saturating/wrapping_abs,unsigned_abs,checked/overflowing/wrapping_negproof_for_contractover all possible inputschecked_pow,saturating_powproof_for_contractover all possible inputs, with no exponent bound — the strengthened loop invariant makes this possible. In return, the contracts state only that the result is nonzero, not its exact value (after loop abstraction, the exact value is not provable)checked/saturating/unchecked_mul,isqrtisqrt, the unverified middle range is called out in-code as a known gapmax,min,clamp,bitor(3 impls)abs,negMIN, one proves thatMINpanics (#[kani::should_panic])from_mut,from_mut_uncheckedWherever a harness restricts its inputs with
kani::assume, akani::covercheck confirms that some input actually satisfies the restriction. Without that check, an impossible restriction makes the proof pass while checking nothing — which is exactly what was wrong with the pre-existingunchecked_mulharnesses: both operands came from the same near-extreme range, every product overflowed, so no input satisfied the function's precondition and the proofs were passing empty. Fixed by pairing each extreme range with a small range for the other operand, and adding a cover for the precondition itself.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.