feat(fuzz): Adds a fuzz harness for the starlark executor #64

nathaniel-brough · 2023-02-17T20:59:56Z

This PR adds a basic fuzz test for the starlark parser and executor. The fuzzer has found a couple of non-critical bugs already :) so hopefully it'll be of some use to you :)

facebook-github-bot · 2023-02-17T21:00:01Z

Hi @silvergasp!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

stepancheg · 2023-02-17T21:06:26Z

What did it find?

I tried adding a fuzzer a while ago, what it found what various overflows, e.g. attempt to calculate [1] * 1000 * 1000 * 1000, which current Starlark does not protect against.

nathaniel-brough · 2023-02-17T21:29:08Z

I haven't had the chance to go through all the crashing test cases yet. So there may be more, but here are the ones I've found so far;

Stack-overflow bug (seems that there is some recursive parsing that overflows the stack)
A couple of OOM's, though I haven't confirmed if this is necessarily a bug with starlark-rust or with the fuzz harness itself, I was planning on minimizing the OOM test-cases sometime next week.

If you are keen on trying out the fuzzer you'll get the best results by copying all the *.star files from this repository into the corpus directory. So something like this;

cargo install cargo-fuzz # if you don't have it installed already
mkdir -p starlark/fuzz/corpus/starlark
find . -name '*.star' -exec cp {} starlark/fuzz/corpus/starlark \;
cd starlark
cargo +nightly fuzz run -O starlark -- -fork=$(nproc)

facebook-github-bot · 2023-02-17T23:18:50Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

stepancheg · 2023-02-21T21:51:50Z

We have discussed fuzzer in our internal meeting.

We think that we want to have fuzzer after all the problems fuzzer can find are eliminated — so we could run a few fuzzer iterations in CI. Otherwise it will be maintenance burden with little profit.

nathaniel-brough · 2023-02-21T22:15:30Z

No worries, that sounds reasonable. I'll add that having regularly scheduled fuzz testing in place can also be handy in catching regressions, rather than just existing bugs :) If you've got any queries about this fuzz harness or the maintenance required I'd be happy to clarify things :)

stepancheg · 2023-02-21T22:19:27Z

having regularly scheduled fuzz testing in place can also be handy in catching regressions

How it can be done? For example, in your case of + stack overflow, can there be some rule like if it is stack overflow, than ignore this input?

nathaniel-brough · 2023-02-21T22:41:23Z

Yeah, there is a tool that is integrated into clusterfuzz/oss-fuzz that does this quite nicely. There are some docs here. It looks something like this on the web UI;

The StackOverflow bug would look pretty much the same except with a "StackOverflow" title.

Essentially the OSS-fuzz (which is just an instance of google/clusterfuzz);

Groups on types of bugs, and allows for filtering (e.g. Buffer Overflow, Stack overflow etc.)
Group each bug based on where in the code the bug occurred (.e.g in the same function or an overflow in a global buffer). This way you don't end up with 1000 different entries that are effectively the same bug.
Automatically attempts to reproduce the bug by running the fuzzer with the same inputs multiple times over, then it reports if it's reproducible.
Automatically attempts to bisect bugs, to see where they where introduced etc.

So while it's not easily doable to do all that filtering and categorizing with just the code in this PR if this project was integrated into google/oss-fuzz it would be relatively straightforward. An alternative to oss-fuzz would be to run your own instance of google/clusterfuzz although that would likely work against the goal of minimising your maintenance burden.

starlark/fuzz/fuzz_targets/starlark.rs

ndmitchell · 2023-02-26T12:10:10Z

starlark/fuzz/fuzz_targets/starlark.rs

+    let module: Module = Module::new();
+    let mut eval: Evaluator = Evaluator::new(&module);
+    let value = std::hint::black_box(eval.eval_module(ast, &globals)?);
+    _ = std::hint::black_box(format!("{value:?}"));


I thought you would have needed to do let _ = ... here. But playground suggests I'm wrong. Surprising.

Yeah, I only found that fairly recently, too. Apparently, omitting the 'let' has been possible since rust 1.59

ndmitchell · 2023-02-26T12:12:09Z

I don't think we want to run our own instance of clusterfuzz. But if we can easily ignore the issues we don't care about (e.g. those which are resource exhaustion of some form - either stack overflow or using too much memory) then that sounds good. Are you @silvergasp volunteering to set up oss-fuzz and ignore those? If so, the value seems good.

nathaniel-brough · 2023-02-26T16:35:13Z

I don't think we want to run our own instance of clusterfuzz. But if we can easily ignore the issues we don't care about (e.g. those which are resource exhaustion of some form - either stack overflow or using too much memory) then that sounds good. Are you @silvergasp volunteering to set up oss-fuzz and ignore those? If so, the value seems good.

Yeah, I'm happy to do that. I'll open up a PR with oss fuzz over the next couple days.

facebook-github-bot · 2023-03-01T15:42:06Z

@ndmitchell has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ndmitchell · 2023-03-01T15:52:46Z

Great, I'll see to getting this code merged. Thanks for your efforts on fuzz testing!

facebook-github-bot · 2023-03-01T17:28:58Z

@ndmitchell merged this pull request in 158a5a8.

Summary: This PR adds a basic fuzz test for the starlark parser and executor. The fuzzer has found a couple of non-critical bugs already :) so hopefully it'll be of some use to you :) X-link: facebook/starlark-rust#64 Reviewed By: stepancheg Differential Revision: D43697975 Pulled By: ndmitchell fbshipit-source-id: f8274390e804bafc4003d7018930ede14a506dd4

nathaniel-brough mentioned this pull request Feb 17, 2023

Integrate with google/oss-fuzz for continuous fuzz testing? #65

Closed

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 17, 2023

nathaniel-brough mentioned this pull request Feb 21, 2023

Stack overflow bug #66

Open

ndmitchell reviewed Feb 26, 2023

View reviewed changes

feat(fuzz): Adds a fuzz harness for the starlark executor

e4a2010

nathaniel-brough force-pushed the main branch from 001ad4a to e4a2010 Compare February 26, 2023 18:26

nathaniel-brough mentioned this pull request Feb 26, 2023

starlark-rust: Initial integration google/oss-fuzz#9809

Merged

facebook-github-bot closed this in 158a5a8 Mar 1, 2023

facebook-github-bot added the Merged label Mar 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fuzz): Adds a fuzz harness for the starlark executor #64

feat(fuzz): Adds a fuzz harness for the starlark executor #64

nathaniel-brough commented Feb 17, 2023

facebook-github-bot commented Feb 17, 2023

stepancheg commented Feb 17, 2023

nathaniel-brough commented Feb 17, 2023 •

edited

Loading

facebook-github-bot commented Feb 17, 2023

stepancheg commented Feb 21, 2023

nathaniel-brough commented Feb 21, 2023

stepancheg commented Feb 21, 2023

nathaniel-brough commented Feb 21, 2023 •

edited

Loading

ndmitchell Feb 26, 2023

nathaniel-brough Feb 26, 2023

ndmitchell commented Feb 26, 2023

nathaniel-brough commented Feb 26, 2023

facebook-github-bot commented Mar 1, 2023

ndmitchell commented Mar 1, 2023

facebook-github-bot commented Mar 1, 2023

feat(fuzz): Adds a fuzz harness for the starlark executor #64

feat(fuzz): Adds a fuzz harness for the starlark executor #64

Conversation

nathaniel-brough commented Feb 17, 2023

facebook-github-bot commented Feb 17, 2023

Action Required

Process

stepancheg commented Feb 17, 2023

nathaniel-brough commented Feb 17, 2023 • edited Loading

facebook-github-bot commented Feb 17, 2023

stepancheg commented Feb 21, 2023

nathaniel-brough commented Feb 21, 2023

stepancheg commented Feb 21, 2023

nathaniel-brough commented Feb 21, 2023 • edited Loading

ndmitchell Feb 26, 2023

Choose a reason for hiding this comment

nathaniel-brough Feb 26, 2023

Choose a reason for hiding this comment

ndmitchell commented Feb 26, 2023

nathaniel-brough commented Feb 26, 2023

facebook-github-bot commented Mar 1, 2023

ndmitchell commented Mar 1, 2023

facebook-github-bot commented Mar 1, 2023

nathaniel-brough commented Feb 17, 2023 •

edited

Loading

nathaniel-brough commented Feb 21, 2023 •

edited

Loading