search

google / clusterfuzzlite

ClusterFuzzLite - Simple continuous fuzzing that runs in CI.
https://google.github.io/clusterfuzzlite
Apache License 2.0
448 stars 42 forks source link

There doesn't seem to be a way to run all fuzz targets in PRs #85

Open evverx opened 2 years ago

evverx commented 2 years ago

Both CFLite and CIFuzz exit as soon as they discover a bug but it would be great if it was possible to let them run all fuzz targets and report all the bugs. It would be consistent with how test suites work in general but other than that it should help in scenarios where bugs unrelated to PRs are more likely to pop up because the latest builds aren't available: https://github.com/systemd/systemd/pull/22302/commits/d38363b96b763c48aa1fc7742db7d17301d48ced

evverx commented 2 years ago

FWIW that commit mentions https://github.com/google/clusterfuzzlite/issues/84 but even if it was possible to trim artifacts and upload the latest builds I think it would make sense to run all fuzz targets there because the latest public OSS-Fuzz corpora (https://github.com/systemd/systemd/commit/69aa4982bc514a1d096aeb0563025ece81f978f9) can trigger bugs that have nothing to do with PRs where patches are backported to stable branches

jonathanmetzman commented 2 years ago

I think I can add this feature, but can you clarify why you want to find bugs in PRs when the bugs were not caused by PRs?

evverx commented 2 years ago

CFLIte tests stable branches that are always behind upstream and I can imagine a scenario where bugs found after releases are cut are fixed upstream but still present in releases. Since the latest OSS-Fuzz corpora are in sync with the upstream repository they can trigger bugs like that in stable branches. They should be fixed there by backporting relevant patches but until then it should be possible for CFLite to also catch other bugs (possibly introduced in PRs) to let people backporting stuff focus on one patchset at a time. I hope it makes sense.

jonathanmetzman commented 2 years ago

Actually instead of adding this feature, maybe we can leverage some that we already have. Can you try to change this mode to batch: https://github.com/systemd/systemd/blob/main/.github/workflows/cflite_pr.yml#L38

evverx commented 2 years ago

Thanks! I'll try to experiment with it tomorrow.

I was actually leaning towards borrowing the "pruning" part where fuzzers don't stop as soon as they discover a bug: https://github.com/evverx/libbpf/runs/4895317343 but I don't think that I need this level of granularity for systemd forks.

evverx commented 2 years ago

Looking at https://github.com/evverx/libbpf/actions/runs/1728560391 it seems artifacts are always uploaded in that mode and I can't do that on every PR

evverx commented 2 years ago

My bad. It seems they were uploaded because the fuzzer failed there. If artifacts aren't uploaded unconditionally it should be fine.

jonathanmetzman commented 2 years ago

O you're right, it will also upload the corpus. OK I need to implement the feature you want.

evverx commented 2 years ago

@jonathanmetzman I tested it anyway to figure out whether it works with more than one fuzz target. In https://github.com/evverx/systemd/pull/8 I broke two fuzz targets and judging by https://github.com/evverx/systemd/runs/5116939636?check_suite_focus=true CFLite was able to catch it. Looking at the log it's hard to tell what failed there exactly. It would probably make sense to accumulate and report issues all at once at the end. Apart from that, those fuzz targets shouldn't have passed the "bad build" check but it wasn't mentioned anywhere (allowed-broken-targets-percentage seems to work though!). I'll try to open new issues.

jonathanmetzman commented 2 years ago

Actually, I think this feature exists, I just need to expose it to users. After my PRs land, set keep-unaffected-fuzz-targets: true in the build_fuzzers action to do this.

evverx commented 2 years ago

Thanks! When it lands could you point me in the direction of the SHA hash of the base builder image with that feature included?

jonathanmetzman commented 2 years ago

Thanks! When it lands could you point me in the direction of the SHA hash of the base builder image with that feature included?

I don't think this is affected by base-builder. I think once I land the PRs you'll need to change the SHA your workflows point to.

jonathanmetzman commented 2 years ago

I've delayed this because I'm concerned about how all of this fine grained configuration interacts with the implicit configuration of the coarse grained stuff. For example, if I make an option to keep fuzzing regardless of crashes, if that's set to true, it's clear that in batch fuzzing and code-change fuzzing the correct behavior is to keep fuzzing. But if it's set to False (as it will be by default) it's only clear what to do in the code-change case, but not so in batch fuzzing (the point of batch fuzzing is to run everything).

Creating a flag that controls whether corpus is uploaded has the same problem, if False, it's clear what to do in batch fuzzing and code-change fuzzing. If it's True, then what should we do in code-change fuzzing? I guess we could upload the corpus but I really don't think it makes sense.

All-in-all too much of these knobs adds a lot of if-cases to the code and makes things harder to maintain.

evverx commented 2 years ago

But if it's set to False (as it will be by default) it's only clear what to do in the code-change case, but not so in batch fuzzing (the point of batch fuzzing is to run everything).

I think this setting should be applicable to "code-change" only. It should probably be ignored everywhere else.

All-in-all too much of these knobs adds a lot of if-cases to the code and makes things harder to maintain.

Agreed.

jonathanmetzman commented 2 years ago

I decided to land it.

evverx commented 2 years ago

@jonathanmetzman could you reopen this issue? Even with keep-unaffected-fuzz-targets CFlite reports the fist bug and bails out while this issue is supposed to cover another use case where it should be possible to collect all the crashes instead of reporting them one by one.

It's useful to get around issues caused by broken coverage though so it's great that CFLite supports it too.

inferno-chromium commented 2 years ago

Reopened. @jonathanmetzman @oliverchang as fyi.