Skip to content
New issue

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

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

Already on GitHub? # to your account

Correctly check for overflow in add, mul and pow #6452

Merged
merged 65 commits into from
Sep 9, 2024

Conversation

SwayStar123
Copy link
Collaborator

@SwayStar123 SwayStar123 commented Aug 22, 2024

Description

Adds flag checks for overflow in core lib, properly cap values if overflow is enabled

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@Dentosal
Copy link
Member

Dentosal commented Aug 23, 2024

This is not a vm issue, the instruction that's supposed to revert is getting optimized away. Likely the compiler doesn't consider panic-on-invalid-input side effect. Not sure if the compiler behavior is expected or not.

Investigation

if the function looks like this:

#[test(should_revert)]
fn math_0th_root_fail() {
    let _res = asm(r1: 100, r2: 0, r3) {
        log flag r1 r2 zero;
        mroo r3 r1 r2;
        log flag r1 r2 r3;
        log one one one one;
        r3: u8
    };
}

Then the test passes as expected.

However, if we remove the middle log, i.e.:

#[test(should_revert)]
fn math_0th_root_fail() {
    let _res = asm(r1: 100, r2: 0, r3) {
        log flag r1 r2 zero;
        mroo r3 r1 r2;
        log one one one one;
        r3: u8
    };
}

Then the test fails.

Let's look at the bytecode. Compile with forc build --output-bin prog.bin --tests (in test/src/in_language_tests/test_programs/math_inline_tests). Then we can find the relevant log instructions with forc parse-bytecode prog.bin | rg -C 1 '\bLOG \{'

For the first version, i.e. without logging r3 , the bytecode looks like this:

      18230   72920   MOVE { dst: 0x11, src: 0x0 }                                               1a 44 00 00
      18231   72924   LOG { a: 0xf, b: 0x10, c: 0x11, d: 0x0 }                                   33 3d 04 40
      18232   72928   LOG { a: 0x1, b: 0x1, c: 0x1, d: 0x1 }                                     33 04 10 41
      18233   72932   RET { value: 0x0 }                                                         24 00 00 00

However, when we introduce the log in the code in the second version, we get:

      18230   72920   MOVE { dst: 0x11, src: 0x0 }                                               1a 44 00 00
      18231   72924   LOG { a: 0xf, b: 0x10, c: 0x11, d: 0x0 }                                   33 3d 04 40
      18232   72928   MROO { dst: 0x12, lhs: 0x10, rhs: 0x11 }                                   18 49 04 40
      18233   72932   LOG { a: 0xf, b: 0x10, c: 0x11, d: 0x12 }                                  33 3d 04 52
      18234   72936   LOG { a: 0x1, b: 0x1, c: 0x1, d: 0x1 }                                     33 04 10 41
      18235   72940   RET { value: 0x0 }                                                         24 00 00 00

So it seems like the mroo is completely missing in the first version.

@SwayStar123 SwayStar123 changed the title Correctly check for overflow and unsafe math Correctly check for overflow in add and mul Aug 26, 2024
@SwayStar123 SwayStar123 requested a review from IGI-111 September 5, 2024 05:57
@SwayStar123 SwayStar123 requested a review from bitzoic September 5, 2024 05:57
@IGI-111 IGI-111 merged commit 4830d00 into master Sep 9, 2024
37 of 38 checks passed
@IGI-111 IGI-111 deleted the swaystar123/proper_overflow_unsafe_math_checking branch September 9, 2024 15:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
audit-report Related to the audit report bug Something isn't working lib: std Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants