fix(ast_fuzzer): bound literal vector indices, and diagnose leftover ACIR memory operations - #13478
Draft
AztecBot wants to merge 8 commits into
Draft
fix(ast_fuzzer): bound literal vector indices, and diagnose leftover ACIR memory operations#13478AztecBot wants to merge 8 commits into
AztecBot wants to merge 8 commits into
Conversation
TomAFrench
approved these changes
Aug 12, 2026
TomAFrench
approved these changes
Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
The nightly AST fuzzer run found seed
0xbc347a7e00100000failingvalid_after_passwithLoad or Store instruction found.A dead
MakeArrayholding a reference makes the allocation inside it look first-class tomem2reg, so the allocation is never promoted and thestorereaches ACIR generation. Original RCA: https://gist.github.com/AztecBot/fc70ad4799ca0e246832bf77d00b5c13Reducing the seed's program showed the shape is not reachable from Noir: keeping the reference-holding aggregate alive that far requires a dynamic index into it, which
verify_no_dynamic_indices_to_referencesrejects with a clean error. So rather than add handling inmem2regfor a shape that cannot occur, this PR fixes the generator that produced it and makes the situation diagnosable if it ever recurs.mem2regis unchanged.1. The generator hole
gen_vector_accesspicks the index one of two ways. Thegen_exprbranch setsin_no_dynamic(banning dynamic indices into reference-holding items, per #8888) and appliesavoid_index_out_of_bounds. The other branch — a plain literal, taken whenevermax_depth == 0or a coin flip — did neither: it emitted an arbitraryu32with no bound and no regard for the item type.That is exactly what the seed did:
c[1168058454_u32]on a constrained[([Field; 2], …, &[&str<1>; 2])]. A constant out-of-bounds index into a vector is not rejected at compile time (unlike an array, the length isn't statically known), so it survives to ACIR generation as an index that could not be simplified — precisely the caseavoid_index_out_of_boundsalready documents as leaving reference allocations behind.The fix hoists the
in_no_dynamiccomputation above both branches and routes the literal branch throughavoid_index_out_of_boundstoo, matching whatgen_indexalready does for arrays. Two consequences:in_no_dynamicis set, soavoid_index_out_of_boundsreturnstrueunconditionally and the index is always% len. The shape can no longer be generated.Config::avoid_index_out_of_boundsat all, which it previously ignored — so setting that config actually takes effect there. Out-of-bounds literal indices are still generated at the usual 10% rate when it is off.2. A useful message on the assertion
assert_not_load_or_storesaid onlyLoad or Store instruction found. It now reports which function, which address, and what the address is a reference to:Its doc comment records why an address can survive that far and points at
verify_no_dynamic_indices_to_referencesas the user-facing check anything reaching the assertion has already passed.load_or_store_assertion_names_the_function_and_addresspins the message.3. Coverage for the validation that keeps this out of SSA
The validator already had unit tests (
dynamic_array_of_2_mut_bools,error_on_index_overflow, the vector-intrinsic cases) andcompile_failureprograms forvector.remove/vector.insert. Two gaps:test_programs/compile_failure/dynamic_array_index_references:*r[i]on a[&mut Field; 2]. The harness snapshots stderr and asserts it contains no panic, pinning that this stays a clean rejection.ArraySetarm of the validator, onlyArrayGet. Addeddynamic_array_set_of_references.Evidence
2702a1e82e6, the revision the seed was found on — the generated AST there isc[1168058454_u32], a bare literal from that branch.0xbc347a7e00100000pass. (Taken alone this is weak: the extraavoid_index_out_of_boundscall shifts entropy consumption, so the seed generates a different program. The structural argument above is what carries it.)mem2regis deliberately unchanged. What changes is that the fuzzer no longer builds it, and the panic now names the function and address.Tests
cargo fmt --all -- --checkcargo clippy -p noir_ast_fuzzer -p noirc_evaluator --all-targets -- -D warningscargo test -p noir_ast_fuzzer— 25 + 4 passedcargo test -p noirc_evaluator— 1948 passed, 15 ignoredcargo test -p nargo_cli --test execute— 9280 passedvalid_after_pass420s,acir_vs_brillig360s,pass_vs_prev360s — all cleanCreated by claudebox · group:
slackbot· requested by Tom (@TomAFrench) · Slack thread