Skip to content

Prevent panic by Number::modulo - #773

Merged
anakrish merged 4 commits into
microsoft:mainfrom
jaylorch:modulo-panic
Jul 27, 2026
Merged

Prevent panic by Number::modulo#773
anakrish merged 4 commits into
microsoft:mainfrom
jaylorch:modulo-panic

Conversation

@jaylorch

@jaylorch jaylorch commented Jul 25, 2026

Copy link
Copy Markdown
Member

In Number::modulo, it calls Number::ints_to_bigint, which could panic. The reason is that calling .to_integer() isn't enough to guarantee that .to_bigint_owned() will return Some, but Number::ints_to_bigint assumes it will and calls unwrap(). In particular, it might be that it's a float corresponding to an integer that's larger than F64_SAFE_INTEGER. The fix is to not call Number::ints_to_bigint (and indeed to delete that entire function, which is only used in this one place), and instead only call unwrap when Some is returned.

A concrete example of interpreter input that would cause a panic is 100000000000000000e-1 % 2. This PR prevents that panic.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens Number::modulo to avoid a panic when operands are floats that are mathematically integral but not exactly representable as integers (e.g., magnitude beyond 2^53), ensuring modulo returns an error instead of panicking.

Changes:

  • Removes the ints_to_bigint helper that unconditionally unwrap()ed bigint conversions and could panic.
  • Updates Number::modulo to only proceed when both operands can be safely converted to BigInt, otherwise returning modulo on floating-point number.
  • Adds regression coverage via an interpreter YAML case and a Rust unit test for large integral floats.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/number.rs Reworks Number::modulo to avoid panic on failed float→bigint conversion; removes the panic-prone helper; adds unit tests.
tests/interpreter/cases/builtins/numbers/mod.yaml Adds a regression test for modulo on an integral-but-too-large float that previously could panic.

Comment thread src/number.rs
@jaylorch
jaylorch requested a review from anakrish July 25, 2026 02:21
@jaylorch
jaylorch marked this pull request as ready for review July 25, 2026 15:06
anakrish
anakrish previously approved these changes Jul 27, 2026

@anakrish anakrish left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for identifying and fixing this @jaylorch. Once the verus formalization of number is done, I'll want to revisit number.

I don't think the official OPA semantics of numbers is that well defined (e.g large integers) and might not be worth being compatible with if we can devise a simpler semantics that policies could be written to.

@jaylorch
jaylorch marked this pull request as draft July 27, 2026 17:47
@jaylorch
jaylorch marked this pull request as ready for review July 27, 2026 19:03
@jaylorch

Copy link
Copy Markdown
Member Author

I had to rebase, which apparently requires another approval.

@jaylorch
jaylorch requested a review from anakrish July 27, 2026 19:04
@anakrish
anakrish merged commit 839510d into microsoft:main Jul 27, 2026
60 checks passed
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