[tools] initial fuzz coverage script by asraa · Pull Request #10289 · envoyproxy/envoy

asraa · 2020-03-06T18:54:34Z

Fuzz coverage script to run all fuzz targets (or ones specified) for 1 min creating a temporary corpus for testcases with new coverage, and then runs a coverage script over all the binaries produced from bazel coverage.

usage:
test/run_envoy_fuzz_coverage.sh (runs all fuzz targets)
test/run_envoy_fuzz_coverage.sh //test/common/common:base64_fuzz_test //test/common/common:hash_fuzz_test (runs just specified)

Risk level: Low
Testing: Local

Signed-off-by: Asra Ali asraa@google.com

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-03-06T18:58:09Z

/review @htuch

Questions:

Was trying to minimize # of targets built. Bazel reports it discards crash when I change the --test_arg. It seems like I can't avoid building the fuzz running target and the corresponding coverage one separately

Signed-off-by: Asra Ali <asraa@google.com>

htuch

This is super awesome, really looking forward to being able to gate commits on fuzz coverage and the flow here makes a lot of sense (generate tmp corpus from real fuzz runs, then do a normal coverage build).

htuch · 2020-03-08T20:48:42Z

test/run_envoy_fuzz_coverage.sh

+if [[ $# -gt 0 ]]; then
+  FUZZ_TARGETS=$*
+else
+  # By default, this will be all fuzz targets.


Something like this also exists in the OSS-fuzz runner script. Could we define this in a common bash utility script and import it in both locations?

What do you think of the tag? makes it an easy bazel query

htuch · 2020-03-08T20:49:25Z

test/run_envoy_fuzz_coverage.sh

+echo "Building fuzz targets..."
+for t in ${FUZZ_TARGETS}
+do
+  bazel build "${t}_with_libfuzzer" --config asan-fuzzer -c opt


Can you do a single build invocation by listing all the targets on the bazel build CLI? That should be faster.

htuch · 2020-03-08T20:50:50Z

test/run_envoy_fuzz_coverage.sh

+for t in ${FUZZ_TARGETS}
+do
+  # Get the original corpus for the fuzz target
+  ORIGINAL_CORPUS=$(bazel query "labels(srcs, ${t}_corpus)" | head -1)


Does this work for a generated corpus?

Yes, modified to pull data attribute, so now it works for something like route fuzz test

test/run_envoy_fuzz_coverage.sh

htuch · 2020-03-08T20:55:15Z

test/run_envoy_fuzz_coverage.sh

+done
+
+# Wait for background process to run.
+# TODO? Processes will still run in background if user ctrl-c.


Consider adding trap handlers, see the trap Bash builtin.

test/run_envoy_fuzz_coverage.sh

htuch · 2020-03-08T21:00:13Z

@lizan probably should also give this a pass once it's ready to ship.

lizan

I see a lot of duplication from the coverage script, is it possible to reuse possible parts from there? Or, use a environment var or parameter to control that script to generate fuzz before run coverage?

I'm seeing a failure in OSS-Fuzz build, as well as fuzz CI tool #10289 due to fuzzing build macro FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION. Basically, PR #10050 introduced regex rewrites that are tested in config_impl_test, which is executed to create the corpus for route_fuzz_test. Fuzz builds are built with the FUZZING_BUILD_MODE... macro, and the RE2 library that does the regex rewrites in the test only does single replacements in fuzzing mode rather than the global replacement in the tests, so the test fails, and the fuzz build fails. This change defined FUZZING_BUILD_MODE... only for fuzzing build rules, and explicitly undefines them for the config_impl_test. RE OSS-Fuzz builds, this is odd. Bazel CLI flags take precedence, so I'm not sure what to do when we build in that environment. I can't seem to #undef in the tests, but I need to come up with some solution.I don't like manually removing the CLI flag in our build script, but maybe? https://github.com/google/oss-fuzz/blob/66e0e379395d3a3ee741b08f4d306e1ed71c4003/infra/base-images/base-clang/Dockerfile Testing: bazel build //test/common/router:route_fuzz_test_with_libfuzzer --config=asan-fuzzer Signed-off-by: Asra Ali <asraa@google.com>

Signed-off-by: Asra Ali <asraa@google.com>

test/build_and_run_fuzz_targets.sh

htuch · 2020-03-17T17:58:48Z

test/build_and_run_fuzz_targets.sh

+  TEMP_CORPORA+="${CORPUS_DIR} "
+  # Run fuzzing process.
+  TARGET_BINARY="${t/://}"
+  bazel-bin/${TARGET_BINARY:2}_with_libfuzzer -max_total_time=60 ${CORPUS_DIR} $(pwd)${ORIGINAL_CORPUS:1} &


Nit: some unescaped expansions.

test/run_envoy_bazel_coverage.sh

htuch · 2020-03-17T18:05:36Z

The convergence with the coverage script looks really nice!

Signed-off-by: Asra Ali <asraa@google.com>

test/build_and_run_fuzz_targets.sh

test/run_envoy_bazel_coverage.sh

htuch

Other than various nits and @lizan comment, this LGTM.
/wait

test/build_and_run_fuzz_targets.sh

test/run_envoy_bazel_coverage.sh

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-03-26T19:06:29Z

Fixed up the rest of the comments + tested with full and partial runs of the fuzz mode and regular mode...

I did change the script to build and run fuzz targets to not exit if it fails, the reason is sometimes the fuzz targets find unrelated blockers (for eg ones on OSS-Fuzz) within the 60 seconds, and I don't think we want to totally block devs from submission if the coverage script fails from an existing bug it finds. wdyt?

Signed-off-by: Asra Ali <asraa@google.com>

htuch · 2020-03-27T21:46:19Z

@asraa seems fine to power on thru those failure although one day I would hope we can remove that; ultimately the main thing we want is to be able to gate on coverage here.

htuch

LGTM modulo comments, will leave it with @lizan for final review and merge, thanks!

htuch · 2020-03-27T21:47:16Z

test/build_and_run_fuzz_targets.sh

+  LIBFUZZER_TARGETS+="${t}_with_libfuzzer "
+done
+
+bazel build ${LIBFUZZER_TARGETS} --config asan-fuzzer -c opt


Should we be using ${BAZEL_BUiLD_OPTIONS} here @lizan ?

Yes unless there's reason not to do so.

Done! Sorry about that

htuch · 2020-03-27T21:50:46Z

test/run_envoy_bazel_coverage.sh

+    TARGET="${t}_with_libfuzzer"
+  else
+    TEST_ARGS=(--test_arg="--log-path /dev/null" --test_arg="-l trace")
+    OBJECTS="bazel-bin/test/coverage/coverage_tests"


It feels like this should be outside the loop, as it's only done once.

For clarity? In the case that it's just coverage_tests the loop only has one item.

Yeah, maybe. I don't feel that strongly, it just took a somewhat higher cognitive hit processing that code in the loop :)

Hm to resolve, I added a comment addressing the loop -- what I didn't want was to copy/paste the bazel command per loop, but if that makes more sense let me know!

stale · 2020-04-07T07:42:34Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Asra Ali <asraa@google.com>

htuch · 2020-04-07T13:47:04Z

@lizan any final comments before merging?

stale · 2020-04-14T14:03:40Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

lizan

Sorry for the delay, can you merge master?

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-04-21T19:55:32Z

All set -- sorry there were a couple coverage related PRs submitted very recently

htuch · 2020-04-22T00:32:53Z

LGTM, will let @lizan do final approval and merge.

Signed-off-by: Asra Ali <asraa@google.com>

stale · 2020-04-29T18:37:55Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

lizan · 2020-04-29T20:19:48Z

@asraa can you check CI?

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-04-29T20:51:42Z

On it -- need to figure out why coverage script seems to have a regression on master when testing locally.

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-04-30T16:55:39Z

I couldn't repro locally with docker bazel.coverage or locally with test/run_envoy_bazel_coverage.sh.

asraa · 2020-05-01T18:42:31Z

friendly ping @lizan -- looks like it's passing CI

initial coverage script

fd15170

Signed-off-by: Asra Ali <asraa@google.com>

repokitteh-read-only bot requested a review from htuch March 6, 2020 18:58

fix script because i misunderstood merge

83cb80b

Signed-off-by: Asra Ali <asraa@google.com>

htuch assigned htuch and lizan Mar 8, 2020

htuch requested a review from lizan March 8, 2020 20:58

htuch suggested changes Mar 8, 2020

View reviewed changes

asraa mentioned this pull request Mar 10, 2020

[build] fix fuzz build mode macro and test runs #10322

Merged

lizan reviewed Mar 13, 2020

View reviewed changes

asraa added 4 commits March 16, 2020 12:05

Merge remote-tracking branch 'upstream/master' into fuzz-coverage

540dd43

Signed-off-by: Asra Ali <asraa@google.com>

fixup

616a834

Signed-off-by: Asra Ali <asraa@google.com>

build in single invokation

45411d0

Signed-off-by: Asra Ali <asraa@google.com>

fixup more comments

8ea16bf

Signed-off-by: Asra Ali <asraa@google.com>

htuch reviewed Mar 17, 2020

View reviewed changes

fixup

ae48280

Signed-off-by: Asra Ali <asraa@google.com>

lizan reviewed Mar 18, 2020

View reviewed changes

test/build_and_run_fuzz_targets.sh Outdated Show resolved Hide resolved

test/run_envoy_bazel_coverage.sh Outdated Show resolved Hide resolved

htuch suggested changes Mar 19, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 19, 2020

asraa added 2 commits March 25, 2020 15:19

Merge remote-tracking branch 'upstream/master' into fuzz-coverage

48f51ed

Signed-off-by: Asra Ali <asraa@google.com>

fixup

de89135

Signed-off-by: Asra Ali <asraa@google.com>

repokitteh-read-only bot removed the waiting label Mar 26, 2020

oops

e60cc43

Signed-off-by: Asra Ali <asraa@google.com>

htuch reviewed Mar 27, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 7, 2020

add comment

bf70062

Signed-off-by: Asra Ali <asraa@google.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 7, 2020

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Apr 14, 2020

lizan reviewed Apr 15, 2020

View reviewed changes

asraa added 3 commits April 16, 2020 10:10

Merge remote-tracking branch 'upstream/master' into fuzz-coverage

73ddc72

Signed-off-by: Asra Ali <asraa@google.com>

Merge remote-tracking branch 'upstream/master' into fuzz-coverage

97b3b41

Signed-off-by: Asra Ali <asraa@google.com>

Merge remote-tracking branch 'upstream/master' into fuzz-coverage

e8b0cf2

Signed-off-by: Asra Ali <asraa@google.com>

add build options

42a9e20

Signed-off-by: Asra Ali <asraa@google.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 29, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 29, 2020

Merge remote-tracking branch 'upstream/master' into fuzz-coverage

178d684

Signed-off-by: Asra Ali <asraa@google.com>

Merge remote-tracking branch 'upstream/master' into fuzz-coverage

d14e72a

Signed-off-by: Asra Ali <asraa@google.com>

lizan approved these changes May 1, 2020

View reviewed changes

htuch merged commit 28af32e into envoyproxy:master May 3, 2020

asraa deleted the fuzz-coverage branch May 4, 2020 13:37

lizan mentioned this pull request May 12, 2020

coverage: use lcov to merge multi-binary coverage report #10909

Merged

Conversation

asraa commented Mar 6, 2020

Uh oh!

asraa commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch commented Mar 8, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented Mar 17, 2020

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asraa commented Mar 26, 2020

Uh oh!

htuch commented Mar 27, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Apr 7, 2020

Uh oh!

htuch commented Apr 7, 2020

Uh oh!

asraa commented Mar 6, 2020 •

edited

Loading