Skip to content

Conversation

@vezenovm
Copy link
Contributor

Description

Problem

Resolves https://github.com/noir-lang/noir/security/advisories/GHSA-pgh4-36hc-pmrx

Summary

Changes:

  • Adds a defensive check that a store address is unused. This is run even when DIE is run after flattening. These cases should be prevented by our pre/post debug assertions, however, this can act as another defensive check.

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.

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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 1bb5312 Previous: 13ce5c8 Ratio
test_report_zkpassport_noir_rsa_ 2 s 1 s 2

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

CC: @TomAFrench

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: 1bb5312 Previous: 13ce5c8 Ratio
private-kernel-inner 2.982 s 2.386 s 1.25

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

CC: @TomAFrench

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 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 1bb5312 Previous: 13ce5c8 Ratio
sha512-100-bytes 0.083 s 0.055 s 1.51

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

CC: @TomAFrench


Store { .. } => should_remove_store(function, flattened),
Store { address, .. } => {
should_remove_store(function, flattened) && !used_values.contains(address)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like !used_values.contains(address) should be added to should_remove_store since it is now a condition on us removing the store or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should_remove_store is a free function also used in the DIE post check so that is why I did not do this change. I could expand its definition to accept used_values though. Do you prefer that?

@vezenovm vezenovm requested a review from jfecher January 29, 2026 17:07
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.

3 participants