Skip to content

fix(acir): Do not read empty array under inactive predicates for various slice ops#10882

Merged
vezenovm merged 1 commit intomasterfrom
mv/acir-empty-slice-preds
Dec 10, 2025
Merged

fix(acir): Do not read empty array under inactive predicates for various slice ops#10882
vezenovm merged 1 commit intomasterfrom
mv/acir-empty-slice-preds

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Dec 10, 2025

Description

Problem

Resolves https://github.com/noir-lang/noir/security/advisories/GHSA-mvh4-wv69-m9xh

Summary

I decided to implement a zero length check in ACIR gen for SlicePopFront, SlicePopBack, SliceRemove, and SliceInsert.

As per the comments under ssa_gen/mod.rs:

In ACIR we do have protection against reading empty slices (it returns "Index Out of Bounds"), so we don't get invalid reads. 
The memory operations in ACIR ignore the side effect variables, so even if we added a constraint here, it could still fail when it inevitably tries to read from an empty slice anyway. 
We have to handle that by removing operations which are known to fail and replace them with conditional constraints that do take the side effect into account.

In order to have a defense in depth we should defend against empty slices in ACIR gen. ACIR gen does not want to rely on SSA gen checks to prevent ACIR memory read failures. Even so it has already been noted that ACIR memory ops ignore side effects variables by design so we need to prevent those operations which we know will fail in ACIR gen.

Changes:

  • Use existing has_zero_length to check if the slice block is empty after initializing the dynamic slice contents array
  • This checks the actual ACIR value representation
  • Works even when dynamic length is unknown
  • If zero length we return dummy values and assert that the predicate is zero to ensure safety
  • Removed the is_constant_zero_length check from fix(acir-gen): Use the side effect variable in slice_pop_back #10455. The assertion was incorrect and needed to use the predicate as well (we just use the check included in this PR now). It is unlikely, but if we had a slice with a length known to be a constant zero under an invalid predicate this would have triggered unnecessary failures
  • Tests under execution_success and execution_failure as well as a couple ACIR gen unit test updates

Additional Context

User Documentation

Check one:

  • No user documentation needed.
  • Changes in docs/ included in this PR.
  • [For Experimental Features] Changes in docs/ to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@vezenovm vezenovm requested a review from TomAFrench December 10, 2025 15:40
@TomAFrench TomAFrench enabled auto-merge December 10, 2025 15:41
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: b822cd0 Previous: 1eeaba1 Ratio
rollup-block-root 1.97 s 1.46 s 1.35

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@TomAFrench TomAFrench added this pull request to the merge queue Dec 10, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 10, 2025
@TomAFrench TomAFrench added this pull request to the merge queue Dec 10, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 10, 2025
@vezenovm vezenovm added this pull request to the merge queue Dec 10, 2025
Merged via the queue into master with commit 5a65dae Dec 10, 2025
182 of 184 checks passed
@vezenovm vezenovm deleted the mv/acir-empty-slice-preds branch December 10, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants