[Merged by Bors] - Parser Idempotency Fuzzer #2400

addisoncrump · 2022-11-02T17:54:02Z

This Pull Request offers a fuzzer which is capable of detecting faults in the parser and interner. It does so by ensuring that the parsed AST remains the same between a parsed source and the result of parsing the to_interned_string result of the first parsed source.

It changes the following:

Adds a fuzzer for the parser and interner.

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 parser-idempotency

codecov · 2022-11-02T18:15:54Z

Codecov Report

Merging #2400 (0f4223c) into main (b88736a) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2400      +/-   ##
==========================================
- Coverage   38.74%   38.70%   -0.05%     
==========================================
  Files         313      314       +1     
  Lines       23856    23883      +27     
==========================================
  Hits         9244     9244              
- Misses      14612    14639      +27

Impacted Files	Coverage Δ
boa_ast/src/declaration/mod.rs	`44.00% <ø> (ø)`
boa_ast/src/declaration/variable.rs	`46.47% <ø> (ø)`
boa_ast/src/expression/access.rs	`26.92% <ø> (ø)`
boa_ast/src/expression/await.rs	`36.36% <ø> (ø)`
boa_ast/src/expression/call.rs	`33.33% <ø> (ø)`
boa_ast/src/expression/identifier.rs	`11.76% <ø> (ø)`
boa_ast/src/expression/literal/array.rs	`19.44% <ø> (ø)`
boa_ast/src/expression/literal/mod.rs	`22.58% <ø> (ø)`
boa_ast/src/expression/literal/object.rs	`17.47% <ø> (ø)`
boa_ast/src/expression/literal/template.rs	`9.67% <ø> (ø)`
... and 42 more

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

boa_engine/fuzz/Cargo.toml

Razican · 2022-11-04T07:24:54Z

boa_engine/fuzz/fuzz_targets/common.rs

+pub struct FuzzData {
+    pub context: Context,
+    pub ast: StatementList,
+}


Even if the usage of the structure could be considered straightforward, adding some documentation could be useful.

Razican · 2022-11-04T07:28:14Z

boa_engine/fuzz/fuzz_targets/common.rs

+        let mut syms_available = Vec::with_capacity(8);
+        for c in 'a'..='h' {
+            syms_available.push(context.interner_mut().get_or_intern(&*String::from(c)));
+        }


Any reason to only have these symbols? Also, I see a TODO below requesting arbitrary string literals.

Yes -- this creates a fixed pool of symbols to use in the AST. The AST, when generated, just throws random symbol indices in; we need to fix them to well-known (non-keyword) items in the symbol interner. Additionally, we don't want to generate them dynamically from the fuzz data because this can introduce "noise" into our sample (basically, bytes used previously to make symbols now form part of the AST or vice versa, unaligning the AST-generating bytes). This is also the reason for the TODO -- string literals from the arbitrary data could introduce quite a bit of undesirable noise, but not doing so means we don't test string parsing/interning. I decided not to for this PR because it could introduce so much noise that it would render this unusable.

Razican · 2022-11-04T07:30:34Z

boa_engine/fuzz/fuzz_targets/parser-idempotency.rs

+use std::error::Error;
+use std::io::Cursor;
+
+fn do_fuzz(mut data: FuzzData) -> Result<(), Box<dyn Error>> {


Even if the usage of the function could be considered straightforward, adding some documentation could be useful. A link to the libfuzzer documentation or to the cargo-fuzz documentation where this usage is explained would be nice.

Razican · 2022-11-04T07:42:03Z

The merge conflict is just because the ast module was moved to its own crate, but the files themselves should be pretty similar.

jedel1043

I think it should be better to lift the fuzz directory into the project root, then have a subdirectory for every type of fuzzer.

addisoncrump · 2022-11-06T15:56:10Z

I think it should be better to lift the fuzz directory into the project root, then have a subdirectory for every type of fuzzer.

I'm not sure if this is possible for cargo-fuzz, but I'll see. I seem to remember it having some trouble executing from a virtual workspace root.

jedel1043 · 2022-11-06T16:02:50Z

You should be able to easily. For example, on gfx-extras they do precisely that :)

addisoncrump · 2022-11-06T16:03:40Z

Yup -- the issue is that you cannot init the fuzzer directory in the virtual workspace root, but you can move it later. Interesting.

jedel1043

Very nice work!

boa_ast/Cargo.toml

boa_interner/src/sym.rs

fuzz/Cargo.toml

addisoncrump · 2022-11-06T16:23:47Z

Perhaps in the future we could put this into CI? It catches problems very fast.

jedel1043 · 2022-11-06T16:45:34Z

Perhaps in the future we could put this into CI? It catches problems very fast.

Yeah! Though, we'll need a proper CI platform for it. We can't run it on every PR because it could throw random errors at any time, A CI action that runs it every hour or so would be nice.

jedel1043

You should rebase this. I recently pushed a changed that extracted the parser from the engine into a crate, and the API of the Parser type changed slightly; it now requires only a &mut Interner reference to work.

This change should also speedup the fuzzer, because it would skip having to initialize the builtins in order to parse.

addisoncrump · 2022-11-06T17:02:03Z

Perhaps in the future we could put this into CI? It catches problems very fast.

Yeah! Though, we'll need a proper CI platform for it. We can't run it on every PR because it could throw random errors at any time, A CI action that runs it every hour or so would be nice.

I can ask if this would be appropriate for OSS-Fuzz?

jedel1043 · 2022-11-06T17:19:07Z

Maybe! We would need to apply for it though.
cc @jasonwilliams to hear his opinion on this

RageKnify

Ty for the awesome contribution and your perseverance having had to redo it. ❤️

fuzz/fuzz_targets/common.rs

jedel1043

No more nitpicking 😆 Great job!

jedel1043 · 2022-11-06T17:47:09Z

bors r+

jedel1043 · 2022-11-06T17:59:16Z

bors r+

This Pull Request offers a fuzzer which is capable of detecting faults in the parser and interner. It does so by ensuring that the parsed AST remains the same between a parsed source and the result of parsing the `to_interned_string` result of the first parsed source. It changes the following: - Adds a fuzzer for the parser and interner. Any issues I raise in association with this fuzzer will link back to this fuzzer. You may run the fuzzer using the following commands: ```bash $ cd boa_engine $ cargo +nightly fuzz run -s none parser-idempotency ``` Co-authored-by: Addison Crump <addison.crump@cispa.de>

bors · 2022-11-06T18:08:31Z

Pull request successfully merged into main.

Build succeeded:

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: ```bash $ cd boa_engine $ cargo +nightly fuzz run -s none vm-implied ``` Co-authored-by: Addison Crump <addison.crump@cispa.de>

addisoncrump mentioned this pull request Nov 2, 2022

[Merged by Bors] - VM Fuzzer #2401

Closed

Razican reviewed Nov 4, 2022

View reviewed changes

Razican added this to the v0.17.0 milestone Nov 4, 2022

Razican requested review from jedel1043, HalidOdat, jasonwilliams, RageKnify and raskad November 4, 2022 07:35

Razican added enhancement New feature or request parser Issues surrounding the parser test Issues and PRs related to the tests. labels Nov 4, 2022

addisoncrump force-pushed the fuzz-parser branch from 49ab7a4 to 2df18bf Compare November 6, 2022 15:27

jedel1043 reviewed Nov 6, 2022

View reviewed changes

jedel1043 requested changes Nov 6, 2022

View reviewed changes

boa_ast/Cargo.toml Outdated Show resolved Hide resolved

boa_interner/src/sym.rs Outdated Show resolved Hide resolved

fuzz/Cargo.toml Outdated Show resolved Hide resolved

fuzz/Cargo.toml Outdated Show resolved Hide resolved

jedel1043 requested changes Nov 6, 2022

View reviewed changes

addisoncrump added 8 commits November 6, 2022 17:52

init parser fuzzing

43c712e

whoops, missed interner changes

92b7023

idempotency check improvement

a488751

better error output

744fc2a

missed a spot

77c7cdc

show original as well for more context

fe1b236

whoops, remove unsound use

7020569

part 2

5522083

addisoncrump added 6 commits November 6, 2022 17:52

add readme to fuzz

ab868b1

add better comments, crash output

5e0459d

move fuzzer directory

117721f

add note on how to actually run the fuzzer

a6dca71

rename {fuzzer-not-safe-for-production => fuzz}

6d4d6ff

updates for new parser module

a5350f9

addisoncrump force-pushed the fuzz-parser branch from 80bd041 to a5350f9 Compare November 6, 2022 16:58

RageKnify approved these changes Nov 6, 2022

View reviewed changes

jedel1043 requested changes Nov 6, 2022

View reviewed changes

fuzz/fuzz_targets/common.rs Outdated Show resolved Hide resolved

prefer interner over context

0f4223c

jedel1043 approved these changes Nov 6, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

bors bot changed the title ~~Parser Idempotency Fuzzer~~ [Merged by Bors] - Parser Idempotency Fuzzer Nov 6, 2022

bors bot closed this Nov 6, 2022

jedel1043 linked an issue Nov 8, 2022 that may be closed by this pull request

Catching lexer/parser bugs with fuzzed input #773

Closed

jedel1043 mentioned this pull request Nov 8, 2022

Catching lexer/parser bugs with fuzzed input #773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Parser Idempotency Fuzzer #2400

[Merged by Bors] - Parser Idempotency Fuzzer #2400

addisoncrump commented Nov 2, 2022

codecov bot commented Nov 2, 2022 •

edited

Loading

Razican Nov 4, 2022

Razican Nov 4, 2022

addisoncrump Nov 6, 2022

Razican Nov 4, 2022

Razican commented Nov 4, 2022

jedel1043 left a comment

addisoncrump commented Nov 6, 2022

jedel1043 commented Nov 6, 2022

addisoncrump commented Nov 6, 2022

jedel1043 left a comment

addisoncrump commented Nov 6, 2022

jedel1043 commented Nov 6, 2022 •

edited

Loading

jedel1043 left a comment

addisoncrump commented Nov 6, 2022

jedel1043 commented Nov 6, 2022

RageKnify left a comment

jedel1043 left a comment

jedel1043 commented Nov 6, 2022

This comment was marked as outdated.

jedel1043 commented Nov 6, 2022

bors bot commented Nov 6, 2022

[Merged by Bors] - Parser Idempotency Fuzzer #2400

[Merged by Bors] - Parser Idempotency Fuzzer #2400

Conversation

addisoncrump commented Nov 2, 2022

codecov bot commented Nov 2, 2022 • edited Loading

Codecov Report

Razican Nov 4, 2022

Choose a reason for hiding this comment

Razican Nov 4, 2022

Choose a reason for hiding this comment

addisoncrump Nov 6, 2022

Choose a reason for hiding this comment

Razican Nov 4, 2022

Choose a reason for hiding this comment

Razican commented Nov 4, 2022

jedel1043 left a comment

Choose a reason for hiding this comment

addisoncrump commented Nov 6, 2022

jedel1043 commented Nov 6, 2022

addisoncrump commented Nov 6, 2022

jedel1043 left a comment

Choose a reason for hiding this comment

addisoncrump commented Nov 6, 2022

jedel1043 commented Nov 6, 2022 • edited Loading

jedel1043 left a comment

Choose a reason for hiding this comment

addisoncrump commented Nov 6, 2022

jedel1043 commented Nov 6, 2022

RageKnify left a comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 commented Nov 6, 2022

This comment was marked as outdated.

jedel1043 commented Nov 6, 2022

bors bot commented Nov 6, 2022

codecov bot commented Nov 2, 2022 •

edited

Loading

jedel1043 commented Nov 6, 2022 •

edited

Loading