[Merged by Bors] - VM Fuzzer

[addisoncrump]

This Pull Request offers a basic VM fuzzer which relies on implied oracles (namely, "does it crash or timeout?").

It changes the following:

• Adds an insns_remaining field to Context, denoting the number of instructions remaining to execute (only available when fuzzing)
• Adds a JsNativeError variant, denoting when the number of instructions has been exceeded (only available when fuzzing)
• Adds a VM fuzzer which looks for cases where Boa may crash on an input

This offers no guarantees about correctness, only assertion violations. Depends on #2400.

Any issues I raise in association with this fuzzer will link back to this fuzzer.

You may run the fuzzer using the following commands:

$ cd boa_engine
$ cargo +nightly fuzz run -s none vm-implied

[jasonwilliams]

It would be useful to have a README.md in the boa_engine/fuzz directory explaining what it is and why we have it there if you can.

[addisoncrump]

It would be useful to have a README.md in the boa_engine/fuzz directory explaining what it is and why we have it there if you can.

Done -- provided some explanation for both fuzzers.

[codecov]

Codecov Report

Merging #2401 (3c718d0) into main (98e6dd3) will increase coverage by 0.10%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##             main    #2401      +/-   ##
==========================================
+ Coverage   52.41%   52.51%   +0.10%     
==========================================
  Files         329      327       -2     
  Lines       34945    34874      -71     
==========================================
- Hits        18315    18314       -1     
+ Misses      16630    16560      -70     

Impacted Files	Coverage Δ
boa_engine/src/vm/mod.rs	48.98% <ø> (ø)
fuzz/fuzz_targets/common.rs	0.00% <0.00%> (ø)
boa_engine/src/context/mod.rs	64.61% <100.00%> (+0.18%)	⬆️
...arser/src/parser/expression/left_hand_side/call.rs	54.09% <0.00%> (-1.64%)	⬇️
boa_engine/src/environments/runtime.rs	47.71% <0.00%> (-0.71%)	⬇️
boa_parser/src/lexer/cursor.rs	87.90% <0.00%> (-0.47%)	⬇️
boa_examples/src/bin/commuter_visitor.rs	0.00% <0.00%> (ø)
boa_examples/src/bin/classes.rs
boa_examples/src/bin/jsarraybuffer.rs
boa_engine/src/object/mod.rs	45.77% <0.00%> (+0.10%)	⬆️
... and 1 more

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

[Razican]

I guess this depends on the parser fuzzer, so let's merge that first :)

[jedel1043]

There was an issue asking for a new feature for limits on loops: #2350. NoInstructionsRemain could be reused to implement this.

[addisoncrump]

There was an issue asking for a new feature for limits on loops: #2350. NoInstructionsRemain could be reused to implement this.

We are on one mind here lol

[jedel1043]

Now that I think about it, you don't need to juggle with conversions of the NoInstructionsRemain error! The VM bails before you can execute anything that could call to_opaque. We just need to panic inside to_opaque if a user tries to convert a NoInstructionsRemain error into an object.

[addisoncrump]

Can the error even be manipulated? Not sure how boa makes native errors available to users.

[addisoncrump]

Now that I think about it, you don't need to juggle with conversions of the NoInstructionsRemain error! The VM bails before you can execute anything that could call to_opaque. We just need to panic inside to_opaque if a user tries to convert a NoInstructionsRemain error into an object.

This isn't quite true. Consider:

while (true) try {} catch {}

The catch attempts to convert it into an object between instructions, so we actually do care. I previously panicked on conversion and had a bunch of false positives (including the one above).

[jedel1043]

Ah, right, the check is inside execute_instruction. Maybe the check should be moved to the while loop inside run?

[addisoncrump]

That's up to y'all; I don't really know what the preference on execution order is.

[jedel1043]

I mean, a NoInstructionsRemain error shouldn't be circumventable from JS, for obvious reasons. I think bailing directly makes more sense for that type of error.

[addisoncrump]

It already isn't -- it immediately throws another in any catch block.

[jedel1043]

Yes, but I think it should directly quit execution instead of trying to execute a catch that won't be executed anyways.

[addisoncrump]

That is not what I thought those buttons did, whoops

[Razican]

Thanks!! LGTM!

[Razican]

@jedel1043 could you check if your comments were addressed?

[jedel1043]

Nice work!

[jedel1043]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• Build and Test (macos-latest)
• Build and Test (windows-latest)
• Coverage
• Misc