Challenge 21: Verify safety of StrSearcher with Kani - #621
Conversation
|
@feliperodri Not exactly. The main differences are:
So I would describe this PR as an alternative and strengthening of the Challenge 21 portion of #538, rather than a direct complement. |
feliperodri
left a comment
There was a problem hiding this comment.
Review: PR #621 — Challenge 21 (StrSearcher safety)
Verdict rationale
This PR is substantially and qualitatively different from the rejected #538. It does not repeat #538's fatal pattern, and it does real, careful verification work. However, it fails two required criteria that the author openly acknowledges in-code, so it cannot be approved as-is.
It is NOT the #538 body-swap / assume-the-conclusion failure (credit where due)
I classified all 12 #[cfg(not(kani))] blocks. None compile out TwoWaySearcher::new/next/next_back and replace them with pure nondeterministic stubs. Instead:
- Boundary-repair loop swaps — forward
Searcher::next(pattern.rsdiff L96/L177 → kani at L115-145) and reversenext_back(L334/L362/L443): the realsearcher.next::<RejectAndMatch>(...)/next_back::<...>call is preserved; only thewhile !is_char_boundary(b) { b += 1 }repair loop is summarized. - Candidate-loop swaps —
continue 'search(L807, L1093) replaced by suffix summaries (kani_stub_after_failed_forward_candidate/...reverse...). - Inner byte-scan
for-loop swaps — (L820, L859, L1101, L1152) replaced bykani_scan_forward/kani_scan_reverse, which still indexneedle[index]/haystack[position+index]at a nondeterministic representative index over the complete scan interval (diff L590-598), so memory-safety of the real indexing is genuinely checked. - Empty-needle char decode (L68) — real
.chars().next()retained in non-kani; kani models it as a nondet width 1..=4 bounded by remaining bytes plus a UTF-8 boundary assumption.
The real control flow, arithmetic (self.position += i - self.crit_pos + 1, self.position += self.period, etc.), and state transitions of the Two-Way algorithm are executed for one representative candidate. This is legitimate loop summarization, not a function-body swap.
Assume-the-conclusion check (FATAL in #538): PASS. The UTF-8 boundary facts on the Match path (assume_valid_utf8_two_way_match_boundaries, diff L1821-1855; assume_valid_utf8_two_way_reverse_match_start, L1857-1877) are gated behind assert!s that the relevant needle bytes were byte-equal to the haystack over the full final character, and the needle is valid UTF-8. Assuming the boundary then follows from "byte-equality with a valid-UTF-8 needle transfers the char boundary" — which is exactly a UTF-8 decoding fact Challenge 21 explicitly permits importing (assumptions 2 and 3). This is a defensible proof cut, not a raw assumption of the type-invariant conclusion.
Invariant is non-trivial: PASS. type_invariant_two_way_searcher (diff L2430-2482) constrains cursor bounds, char-boundaries of position/end, 1 <= period <= needle_len, crit_pos/crit_pos_back bounds, long/short-period sentinel consistency (memory/memory_back), and short-period boundary summaries. It is meaningful, not true.
Proof harnesses exist: 15 challenge #[kani::proof] harnesses (1 empty constructor + 6 empty methods + 6 Two-Way methods), plus the 2 retained small_slice_eq proofs. Absence of proof_for_contract is fine here since the challenge is invariant-based, not contract-based.
Blocking issue 1 — Verification is NOT unbounded (mandatory criterion FAILS)
The challenge states: "The verification must be unbounded—it must hold for inputs of arbitrary size." This PR hard-bounds inputs:
pattern.rsdiff L1271:const MAX_UTF8_BYTES: usize = 16;any_valid_utf8_str(L1273-1282) takes&[u8; MAX]and slices a 16-byte array; every harness buildshaystack/needlefrom[u8; MAX_UTF8_BYTES](e.g. L2752-2755).
So needle.len(), period, crit_pos, position, end are all bounded by 16. The author acknowledges this directly at diff L2748-2749 ("the concrete haystack and needle models remain bounded by MAX_UTF8_BYTES") and in the PR description ("the current harness models do not constitute an arbitrary-length input proof"). Summarizing the loops removes unwind bounds but does not make the input size unbounded. This is a clear failure of a mandatory requirement.
Blocking issue 2 — Creation does not establish C for TwoWay (success criterion 1 FAILS)
Criterion 1 requires proving that a searcher created from any valid UTF-8 haystack satisfies C. This is done for the empty needle via harness_str_searcher_empty_into_searcher (diff L2557-2565), which calls the real needle.into_searcher(haystack). But for the core Two-Way case there is no constructor harness: TwoWaySearcher::new is never invoked (grep: 0 references), and the invariant is only kani::assumed (e.g. L2772). The author explicitly disclaims this at diff L2745-2747: "Establishing it for the production preprocessing algorithm is a separate proof obligation and is not claimed by this module."
This is a real gap, not a formality: because the hand-written invariant is never cross-checked against what TwoWaySearcher::new actually produces, the preservation proofs (criterion 3) run over a hand-specified state set that is never tied to reachable states. If that set under-approximates reachable states, criterion 3 coverage is incomplete; the constructor harness is what closes this.
Non-blocking observations
- The loop-state relations (
valid_two_way_*_loop_state) are used as bothassume(loop head) andassert(loop exit) but are not independently shown to be inductive; soundness of the suffix summaries rests on them. Worth a reviewer note even after the blockers are fixed. kani::assume(false)is used to prune the continued-loop path (e.g.next_rejectdiff L271) after the per-iteration safety asserts fire — acceptable idiom, but each such site should be double-checked to confirm every concrete exit (Reject/Match/Done) is reachable via the symbolic prefix.
Direction to author
- Make inputs unbounded: drive the loop summaries/
type_invariantoff symbolic lengths rather than a fixed 16-byte array, or otherwise remove the input-size bound, so the proof holds for arbitrary-size haystack/needle. - Add a Two-Way constructor harness that runs the real
into_searcher/TwoWaySearcher::newon a valid UTF-8 (needle, haystack) and assertstype_invariant_two_way_searcher, closing criterion 1 for the core algorithm.
Once these two are addressed, the approach here (real algorithm + Challenge-21-permitted UTF-8 imports + non-trivial invariant) is a strong basis for approval.
Summary
This PR adds Kani verification for the substring searcher in
core::str::pattern, covering the empty-needle implementation and the forward and reverse Two-Way search methods required by Challenge 21.Verification Coverage Report (12/12 Harnesses Verified)
nextnext_matchnext_rejectnext_backnext_match_backnext_reject_backnextRejectAndMatchcandidate, summarized forward/reverse byte scans, rejection boundary repair, forward progress, valid Match/Reject ranges, and invariant preservation.next_matchMatchOnlysearch from an arbitrary failed-candidate prefix through one real candidate and a conservative suffix summary that covers a later Match or exhaustion.next_rejectnextstep, covering every Reject or Done exit while checking progress and invariant preservation.next_backRejectAndMatchpath, summarized reverse/forward byte scans, rejection boundary repair, reverse progress, valid Match/Reject ranges, and invariant preservation.next_match_backMatchOnlysearch from an arbitrary failed-candidate prefix through one real candidate and a conservative suffix summary.next_reject_backnext_backstep, covering every Reject or Done exit.Verification Approach
The verification defines a stable-state invariant
Cfor bothStrSearcherimplementations.For
EmptyNeedle,Crequires the forward and reverse cursors to remain in bounds and on UTF-8 boundaries. The constructor harness establishesC, and each method harness starts from an arbitrary state satisfyingC, checks the returned transition, and proves thatCis preserved. The Kani-only character step nondeterministically selects a width from 1 through 4, bounds it by the remaining bytes, and imports the UTF-8 boundary fact permitted by Challenge 21. Because empty-needle results strictly alternate between Match and Reject, the four filtering methods need at most two concrete calls tonextornext_back; this removes their production loops without adding a loop invariant or unwind bound.For
TwoWaySearcher,Ccaptures the safety-relevant stable state: bounded UTF-8 cursor positions, nonzero bounded periods, bounded critical positions, consistent long-period sentinels, valid short-period memory states, and UTF-8 boundary facts for the short-period cuts. Each of the six method harnesses starts from a symbolic state satisfying this invariant and proves valid output ranges, cursor progress, preservation of preprocessing fields, valid memory-state transitions, and restoration ofCbefore returning.The Two-Way candidate loops use loop stubbing. A nondeterministic loop head represents any prefix of failed candidates, one representative candidate retains the real production control flow and arithmetic, and a suffix summary over-approximates any number of later failed candidates followed by either a Match, a Reject, or exhaustion. The summaries havoc only loop-carried state and constrain it with the corresponding loop-state relation.
The forward and reverse byte-scan loops are summarized by
KaniTwoWayScan. A nondeterministic representative scan index checks the real indexing operations over the complete scan interval. The summary then conservatively chooses a mismatch position or a completed scan while retaining the first and last UTF-8 character bytes and the period probe needed to justify safe Match boundaries.Two-Way Reject results may initially stop at byte positions that are not character boundaries. The Kani-only boundary-repair summaries first prove that the raw cursor is within the haystack, execute the real increment/decrement operation at a representative non-boundary loop head, and then use the Challenge 21 UTF-8 assumption that a character has at most three continuation bytes to summarize arrival at a nearby character boundary.
Verification Tradeoffs
Directly unwinding the nested Two-Way candidate and byte-scan loops does not finish within a practical verification budget and would make the proof depend on a fixed search length. The loop summaries avoid fixed unwind bounds for those loops and over-approximate their safety-relevant behavior.
The scan and candidate summaries are proof cuts for memory safety and valid UTF-8 result boundaries. They do not prove that every reported Match is the first or semantically correct substring match, nor do they prove the full functional correctness of the Two-Way algorithm.
The concrete harness inputs are symbolic valid UTF-8 subslices of 16-byte storage arrays. The loop summaries cover arbitrary loop prefixes and suffixes without fixed unwinding, but the current harness models do not constitute an arbitrary-length input proof.
Scope Assumptions
type_invariant_two_way_searcher.#[cfg(kani)]and do not affect non-Kani builds.Verification
All added Challenge 21 harnesses pass locally with Kani.
Resolves #278
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.