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

[Merged by Bors] - fuzzer: bubble up NoInstructionsRemain error instead of trying to handle as exception #2566

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - fuzzer: bubble up NoInstructionsRemain error instead of trying to handle as exception #2566

wants to merge 2 commits into from

Conversation

Mrmaxmeier
Copy link
Contributor

Hi,

the vm-implied fuzzer panics when executing this testcase:

try {
    new function() {
        while (this) {}
    }();
} catch {
}

internal error: entered unreachable code: The NoInstructionsRemain native error cannot be converted to an opaque type

Handling the NoInstructionsRemain error upfront instead of going through the VM exception handling logic seems to work.

@Mrmaxmeier
Copy link
Contributor Author

Mrmaxmeier commented Jan 26, 2023

I'm not sure what's going on with the asinh test:

---- builtins::math::tests::asinh stdout ----
undefined
thread 'builtins::math::tests::asinh' panicked at 'assertion failed: `(left == right)`
  left: `0.881373587019543`,
 right: `0.8813735870195429`', boa_engine/src/builtins/math/tests.rs:96:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

might be related to a recent Rust release or just numerical instability of different CI setups? => rust-lang/rust#104553

try {
    new function() {
        while (this) {}
    }();
} catch {
}

was panicking with "internal error: entered unreachable code: The NoInstructionsRemain native error cannot be converted to an opaque type"
@codecov
Copy link

codecov bot commented Jan 28, 2023

Codecov Report

Merging #2566 (d1150a2) into main (e6a1c37) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2566      +/-   ##
==========================================
+ Coverage   50.01%   50.02%   +0.01%     
==========================================
  Files         380      379       -1     
  Lines       37786    37824      +38     
==========================================
+ Hits        18898    18922      +24     
- Misses      18888    18902      +14     
Impacted Files Coverage Δ
boa_engine/src/vm/mod.rs 48.75% <ø> (ø)
boa_parser/src/parser/statement/mod.rs 65.32% <0.00%> (-1.35%) ⬇️
...ser/src/parser/expression/left_hand_side/member.rs 47.54% <0.00%> (-1.18%) ⬇️
.../statement/declaration/hoistable/class_decl/mod.rs 51.51% <0.00%> (-0.86%) ⬇️
...arser/expression/primary/object_initializer/mod.rs 64.39% <0.00%> (-0.71%) ⬇️
boa_engine/src/object/mod.rs 45.84% <0.00%> (-0.11%) ⬇️
boa_examples/src/bin/jstypedarray.rs
boa_parser/src/lexer/identifier.rs 95.91% <0.00%> (+0.17%) ⬆️
boa_parser/src/lexer/string.rs 82.51% <0.00%> (+0.50%) ⬆️
boa_ast/src/visitor.rs 13.37% <0.00%> (+0.58%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@raskad
Copy link
Member

raskad commented Jan 28, 2023

Hi, thanks for the contribution! This indeed seems like undesirable behaviour for the vm-implied fuzzer.

I think with the proposed change we would run into infinite loops while fuzzing. Take for example this code:

while (true) {
  try {
    new function() {
        while (true) {}
    }();
  } catch {
  }
}

The error would be thrown in Context::run but would be catched in the outer try/catch logic and never end the execution.

I'm not 100% sure what the best solution for this problem is. Maybe instead of adding a lot of fuzzing specific logic to the vm, we could keep the panic, catch it in the fuzzer and somehow signal the fuzzer that this was a NoInstructionsRemain panic?

@Mrmaxmeier
Copy link
Contributor Author

The error would be thrown in Context::run but would be catched in the outer try/catch logic and never end the execution.

Yes, it looks like some of the callers of Context::run (async, generators) will silently consume the error without bubbling it up.
That seems bad, but in practice, I don't think we'll get stuck in an infinite loop. Even if the original NoInstructionsRemain error is lost in generator shenannigans, we'll eventually try to execute something else and raise another NoInstructionsRemain error.

(Unfortunately there's no better way to do this, but running the fuzzer with your while-try-while testcase hardcoded works/terminates fine.)

@raskad
Copy link
Member

raskad commented Jan 31, 2023

That seems bad, but in practice, I don't think we'll get stuck in an infinite loop. Even if the original NoInstructionsRemain error is lost in generator shenannigans, we'll eventually try to execute something else and raise another NoInstructionsRemain error.

True, with that in mind this should work. @jedel1043 I think you brought up the panic on NoInstructionsRemain initially. Do you agree that this change should work or do you see any way in which this could fail?

@jedel1043
Copy link
Member

I think it should be ok. Though, if any problems arise, we could just make JsError::to_opaque throw an error on trying to convert a JsNativeError::NoInstructionRemain.

@raskad raskad added this to the v0.17.0 milestone Feb 2, 2023
@raskad raskad added the bug Something isn't working label Feb 2, 2023
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thanks!

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 2, 2023
…dle as exception (#2566)

Hi,

the `vm-implied` fuzzer panics when executing this testcase:

```javascript
try {
    new function() {
        while (this) {}
    }();
} catch {
}
```

`internal error: entered unreachable code: The NoInstructionsRemain native error cannot be converted to an opaque type`

Handling the `NoInstructionsRemain` error upfront instead of going through the VM exception handling logic seems to work.
@bors
Copy link

bors bot commented Feb 2, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title fuzzer: bubble up NoInstructionsRemain error instead of trying to handle as exception [Merged by Bors] - fuzzer: bubble up NoInstructionsRemain error instead of trying to handle as exception Feb 2, 2023
@bors bors bot closed this Feb 2, 2023
@Mrmaxmeier Mrmaxmeier deleted the fuzzer-dont-catch-instruction-limit-exception branch February 3, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants