-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Continuous Fuzzing Integration with Fuzzit #7509
Conversation
13d63ca
to
206ec5f
Compare
ci/run_fuzzit.sh
Outdated
export ENVOY_SRC_PATH=`pwd` | ||
cd oss-fuzz | ||
python infra/helper.py build_image --pull envoy | ||
python infra/helper.py build_fuzzers --sanitizer=address envoy ${ENVOY_SRC_PATH} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One key issue here is build performance; if this takes a significant amount of time (which anecdotally it does from local runs), e.g. > 1 hour, it will become a CI bottleneck. Many of our CI jobs now benefit from Bazel caching and (in the future) RBE enablement, this one is going to be inherently stuck without this, since the oss-fuzz image doesn't have any plumbing for this. CC @lizan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch This is exactly what I'm trying to do just wanted to see if it works with the current oss-fuzz images. Anyway I was able to compile the targets with bazel for example
using bazel build //test/server:server_fuzz_test
the build completes successfully but it seems that it doesn't link against libFuzzer. what would be the right command to link it against libfuzzer (i.e -fsanitize=fuzzer) as I don't wont to use the oss docker both to use the caching and also not to use the nested docker in Circle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm afraid you're going to have to do some gnarly Bazel hacking to get it to link against libfuzzer under Bazel. We would be super appreciative of this work, but it will involve diving into https://github.com/envoyproxy/envoy/blob/master/bazel/envoy_test.bzl#L66 and figuring out how to cleanly offer both the corpus run (using the existing test driver) and a way to on the Bazel CLI configure a link against libfuzzer (and somehow getting libfuzzer included in the Envoy build).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll find the way to do that. I just wanted to make sure that I'm not doing double work and it's not implemented yet.
ci/run_fuzzit.sh
Outdated
git clone https://github.com/google/oss-fuzz.git | ||
export ENVOY_SRC_PATH=`pwd` | ||
cd oss-fuzz | ||
python infra/helper.py build_image --pull envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we doing nested Docker here? I.e. running oss-fuzz Docker build inside a CircleCI Docker env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm exactly working on this to make it work not inside the oss-fuzz docker.
206ec5f
to
5594a38
Compare
e1b4af6
to
1178f97
Compare
1178f97
to
a0cb5b4
Compare
a0cb5b4
to
b704a74
Compare
@htuch looks like I get some no-credits error at CircleCI? any particular reason for this to happen? I can test my changes. |
b704a74
to
958e2c6
Compare
@yevgenypats this is a general Envoy CI issue, we are working to resolve. |
/retest |
🔨 rebuilding |
958e2c6
to
3c3ec56
Compare
@htuch thx. just an update. look like it works now. I'll update when the PR will be ready for review. should be soon. cheers. |
59876ab
to
e1b1881
Compare
ci/do_ci.sh
Outdated
FUZZ_TEST_TARGETS="$(bazel query "attr('tags','fuzzer',${TEST_TARGETS})")" | ||
echo "bazel ASAN libFuzzer build with fuzz tests ${FUZZ_TEST_TARGETS}" | ||
echo "Building envoy fuzzers and executing 100 fuzz iterations..." | ||
bazel_with_collection build ${BAZEL_BUILD_OPTIONS} --config=asan-fuzzer ${FUZZ_TEST_TARGETS} --test_arg="-runs=10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need --test_arg
ci/do_ci.sh
Outdated
FUZZ_TEST_TARGETS="$(bazel query "attr('tags','fuzzer',${TEST_TARGETS})")" | ||
echo "bazel ASAN libFuzzer build with fuzz tests ${FUZZ_TEST_TARGETS}" | ||
echo "Building envoy fuzzers and executing 100 fuzz iterations..." | ||
bazel_with_collection build ${BAZEL_BUILD_OPTIONS} --config=asan-fuzzer ${FUZZ_TEST_TARGETS} --test_arg="-runs=10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -12,6 +12,8 @@ jobs: | |||
CI_TARGET: 'bazel.compile_time_options' | |||
fuzz: | |||
CI_TARGET: 'bazel.fuzz' | |||
fuzzit: | |||
CI_TARGET: 'bazel.fuzzit_regression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you intended to do this in every PR? This doesn't provide anything in addition to bazel.fuzz above, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. I think it is and this was one of the main reasons integrating fuzzit in addition to OSS Fuzz. this downloads the current corpus+fixed_crashes from Fuzzit Servers and runs the fuzzers through those test-cases which is stronger then just a run for 10 seconds with empty corpus (which is fine but that just checks if the fuzzers compiled successfully)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we merge those two jobs into one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
sudo service docker restart | ||
displayName: "Enable IPv6" | ||
|
||
- script: ci/run_envoy_docker.sh 'ci/do_ci.sh bazel.fuzzit_fuzzing' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add FUZZIT_API_KEY env (it can be in FUZZIT_API_KEY=$(FuzzitApiKey)
before ci/do_ci.sh, or env below and add propagate it in ci/run_envoy_docker.sh
.
61d474a
to
562831f
Compare
ci/do_ci.sh
Outdated
setup_clang_toolchain | ||
FUZZ_TEST_TARGETS="$(bazel query "attr('tags','fuzzer',${TEST_TARGETS})")" | ||
echo "bazel ASAN libFuzzer build with fuzz tests ${FUZZ_TEST_TARGETS}" | ||
echo "Building envoy fuzzers and executing 100 fuzz iterations..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update comment as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -12,6 +12,8 @@ jobs: | |||
CI_TARGET: 'bazel.compile_time_options' | |||
fuzz: | |||
CI_TARGET: 'bazel.fuzz' | |||
fuzzit: | |||
CI_TARGET: 'bazel.fuzzit_regression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we merge those two jobs into one?
562831f
to
5c127f8
Compare
@lizan fixed the review:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/wait
@@ -53,3 +53,36 @@ jobs: | |||
pathtoPublish: "$(Build.StagingDirectory)/envoy" | |||
artifactName: $(CI_TARGET) | |||
condition: always() | |||
|
|||
- job: fuzzit_fuzzing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some comments here explaining when this runs? Does this happen on every PR, does it block PRs, is this contributing to the PR critical path on CI? I think this PR generally looks awesome, but this needs some clarification, preferably in source comments. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, as explained in the comments there are two workflows:
- Fuzzing - This will run on every push/merge to master, will build the fuzzers and will upload them to Fuzzit where they will run asynchronous. This will ensure the latest version of the code is always being fuzzed and new bugs are found as new code is added.
- Regression - This will run on every commit/PR and will run the fuzzers inline in the CI together with the corpus generated on Fuzzit as well as previous fixed crashes. This will ensure bugs are found BEFORE merge.
Signed-off-by: Yevgeny Pats <[email protected]>
5c127f8
to
54dc342
Compare
/retest |
🔨 rebuilding |
@htuch passing!:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yevgenypats. Appreciate your patience and work on iterating to find the best fit with Envoy CI and fuzzing. Also thanks @lizan for all the help with the CI aspects.
@htuch @lizan sure thing, thank you! and feel free to ping me if there are any issues/changes you need. also @lizan are you sure the FUZZIT_API_KEY is available because I see that the script didn't find it (I couldn't check it before merge as this is environment variable is not available for forked-PRs) Also looks there are some crashes already https://app.fuzzit.dev/orgs/envoyproxy/dashboard and of course feel free to RT https://twitter.com/fuzzitdev/status/1176246876821217283 Thanks! |
@yevgenypats are these crashes limited in visibility? We would like to maintain embargo on any zero days and only have https://github.com/google/oss-fuzz/blob/master/projects/envoy/project.yaml#L3 see these. |
@htuch yes of course, like we discussed. currently only @lizan has access I can add you to the email notifications but you need to sign-up at https://app.fuzzit.dev so I can add you to the envoy account so you will have access to the crashes and other data. |
I set the api key right now, should be fixed in future builds |
Great.
…On Mon, Sep 23, 2019, 5:55 PM Lizan Zhou ***@***.***> wrote:
I set it right now, should be fixed in future builds
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7509?email_source=notifications&email_token=AD52CDUYCVHBV4TWPG5AYHTQLE3LVA5CNFSM4H7ICZEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7MMOZY#issuecomment-534300519>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD52CDRJ4YHQ2IFSKL2UCEDQLE3LVANCNFSM4H7ICZEA>
.
|
@yevgenypats now it is failing with
|
This will introduce another platform (apart from oss) fuzz that will run the long-running fuzzers as well as will introduce "sanity fuzzers" that will run the accumlated corpus and crashes on every Pull-Request to detect bugs early-on in the development cycle. Risk Level: Low - as this will introduce only another step in CircleCI where the fuzzers will be uploaded to Fuzzit and the heavy lifting will be there. Testing: No code is added just a CI code in Circle Signed-off-by: Yevgeny Pats <[email protected]>
This will introduce another platform (apart from oss) fuzz that will run the long-running fuzzers as well as will introduce "sanity fuzzers" that will run the accumlated corpus and crashes on every Pull-Request to detect bugs early-on in the development cycle. Risk Level: Low - as this will introduce only another step in CircleCI where the fuzzers will be uploaded to Fuzzit and the heavy lifting will be there. Testing: No code is added just a CI code in Circle Signed-off-by: Yevgeny Pats <[email protected]>
This will introduce another platform (apart from oss) fuzz that will run the long-running fuzzers as well as will introduce "sanity fuzzers" that will run the accumlated corpus and crashes on every Pull-Request to detect bugs early-on in the development cycle. Risk Level: Low - as this will introduce only another step in CircleCI where the fuzzers will be uploaded to Fuzzit and the heavy lifting will be there. Testing: No code is added just a CI code in Circle Signed-off-by: Yevgeny Pats <[email protected]>
Please do not merge yet. This is work in progress.
The contribution was discuss in the mailing-list with @htuch , @mattklein123 .
Description:
This will introduce another platform (apart from oss) fuzz that
will run the long-running fuzzers as well as will introduce
"sanity fuzzers" that will run the accumlated corpus and crashes
on every Pull-Request to detect bugs early-on in the development
cycle.
Risk Level:
Low - as this will introduce only another step in CircleCI where the fuzzers will be uploaded to Fuzzit and the heavy lifting will be there.
Testing:
No code is added just a CI code in Circle
Docs Changes:
Will be updated later.
Release Notes: None
[Optional Fixes #Issue] None
[Optional Deprecated:] None