Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for overflows in relocatable operations #859

Merged
merged 10 commits into from
Mar 1, 2023

Conversation

fmoletta
Copy link
Contributor

@fmoletta fmoletta commented Feb 24, 2023

This PR does the following:

  • Add checks for possible overflows when adding to a relocatable's offset (which can panic if the addition exceeds usize::MAX)
  • Move Relocatable operations to trait implementations

Depends on #855

@fmoletta fmoletta changed the base branch from main to math-error February 24, 2023 17:59
@fmoletta fmoletta marked this pull request as ready for review February 24, 2023 18:01
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #859 (d0e12f3) into math-error (4beb4c8) will increase coverage by 0.00%.
The diff coverage is 97.76%.

@@             Coverage Diff             @@
##           math-error     #859   +/-   ##
===========================================
  Coverage       96.90%   96.91%           
===========================================
  Files              69       69           
  Lines           29006    28965   -41     
===========================================
- Hits            28108    28070   -38     
+ Misses            898      895    -3     
Impacted Files Coverage Δ
src/hint_processor/hint_processor_utils.rs 88.88% <80.00%> (ø)
src/vm/vm_core.rs 97.48% <90.00%> (-0.04%) ⬇️
src/types/relocatable.rs 99.66% <98.73%> (+0.63%) ⬆️
..._processor/builtin_hint_processor/blake2s_utils.rs 99.74% <100.00%> (-0.01%) ⬇️
...uiltin_hint_processor/cairo_keccak/keccak_hints.rs 93.96% <100.00%> (ø)
...rocessor/builtin_hint_processor/dict_hint_utils.rs 99.22% <100.00%> (-0.01%) ⬇️
...cessor/builtin_hint_processor/find_element_hint.rs 96.46% <100.00%> (ø)
...t_processor/builtin_hint_processor/keccak_utils.rs 100.00% <100.00%> (ø)
...int_processor/builtin_hint_processor/math_utils.rs 97.94% <100.00%> (ø)
...hint_processor/builtin_hint_processor/pow_utils.rs 100.00% <100.00%> (ø)
... and 25 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +134 to +135
type Output = Result<Relocatable, MathError>;
fn add(self, other: &MaybeRelocatable) -> Result<Relocatable, MathError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typical (equivalent) signature:

Suggested change
type Output = Result<Relocatable, MathError>;
fn add(self, other: &MaybeRelocatable) -> Result<Relocatable, MathError> {
type Output = Result<Relocatable, MathError>;
fn add(self, other: &MaybeRelocatable) -> Self::Output {

I'm not 100% sure returning a Result<_> from an operator is what anybody would expect, as typically they'd use checked_* to handle errors and let it panic! otherwise. Still, this is infinitely better than what we had, so I'm OK with it.

Copy link
Contributor

@Jrigada Jrigada left a comment

Choose a reason for hiding this comment

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

Much nicer

@Jrigada Jrigada merged commit 1b8f29e into math-error Mar 1, 2023
@Jrigada Jrigada deleted the check-overflow-relocatable-operations branch March 1, 2023 19:18
Oppen pushed a commit that referenced this pull request Mar 6, 2023
* Use only option for Memory.get

* Fix some tests + refactor range_check validation

* use proper error for get_memory_holes

* Move MaybeRelocatable methods get_int_ref & get_reloctable to Option

* Fix tests

* Clippy

* Fix `CairoRunner::write_output` so that it prints missing and relocatable values (#853)

* Print relocatables & missing members in write_output

* Add test

* Move errors outputed by math_utils to MathError

* Start moving relocatable operations to MathError

* Fix tests

* Remove math-related errors from vm error

* Move conversion errors to MathError

* Move type conversions to MathError

* Remove unused errors

* Clippy

* Clippy

* Simplify addition

* Simplify addition

* Clippy

* Add math_errors.rs

* Check for overflows in relocatable operations (#859)

* Catch possible overflows in Relocatable::add

* Move sub implementations to trait impl

* Swap sub_usize for - operator

* Vheck possible overflows in Add<i32>

* Fix should_panic test

* remove referenced add

* Replace Relocatable methods for trait implementations

* Catch overflows in mayberelocatable operations

* Fix keccak

* Clippy
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