The first working draft of the rules_fuzzing integration in Envoy. by stefanbucur · Pull Request #14675 · envoyproxy/envoy

stefanbucur · 2021-01-12T19:07:00Z

Commit Message: Use the new rules_fuzzing Bazel repository for fuzzing support.

Additional Description: The new fuzzing rules provide an improved fuzzing development experience and simplifies a number of tasks, such as OSS-Fuzz integration.

Risk Level: High

Testing: Used RBE to test the changes on all fuzz tests: bazel test --config=remote --config=asan-fuzzer -c opt $(bazel query 'let all_fuzz_tests = attr(tags, "fuzz_target", "...") in $all_fuzz_tests - attr(tags, "no_fuzz", $all_fuzz_tests)')

Docs Changes: None yet.

Release Notes: N/A

Platform Specific Features: Fuzz tests run on Linux platforms only.

repokitteh-read-only · 2021-01-12T19:07:03Z

Hi @stefanbucur, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14675 was opened by stefanbucur.

see: more, trace.

repokitteh-read-only · 2021-01-12T19:07:06Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #14675 was opened by stefanbucur.

see: more, trace.

stefanbucur · 2021-01-12T19:08:41Z

@asraa This is the PR with all my changes so far, PTAL when you get the chance.

asraa · 2021-01-13T15:14:12Z

bazel/envoy_test.bzl

why does the raw binary name get compiled as a cc_test?

Ah, good question - so I left the original binary as a cc_test since I didn't get the chance to look at coverage support, so I left in place the original cc_test + its data dependency on the corpus so we can still use the old way of collecting code coverage. Maybe we can migrate this part to bazel_rules in a subsequent step, or you think we should do it now?

Hmmm fuzz coverage actually uses the libfuzzer linked target (and normal test coverage ignores fuzzers).

Let me clone locally and see what happens when running fuzz_coverage. It might work since the fuzz coverage script expects a libfuzzer linked binary ~~but the tag might need to change to filter for fuzz_target_binary targets~~.

asraa · 2021-01-13T15:15:57Z

Looks good to me otherwise! Thank you so much --

to fix the format CI, you can run ./tools/code_format/check_format.py fix locally

stefanbucur · 2021-01-14T03:37:07Z

Looks good to me otherwise! Thank you so much --

to fix the format CI, you can run ./tools/code_format/check_format.py fix locally

Ah, great, thanks for the pointer!

BTW, I realized there are two failures here. The other one is:

Dependency validation failed, please check metadata in bazel/repository_locations.bzl
Missing deps in test_only "use_category": ['rules_fuzzing_oss_fuzz', 'honggfuzz', 'fuzzing_py_deps_pypi__absl_py_0_11_0']

Are there other places in the Bazel dependency configuration where we need to add these?

asraa · 2021-01-19T19:43:56Z

Checking out coverage, there is a fuzz coverage script here: https://github.com/envoyproxy/envoy/blob/main/bazel/coverage/fuzz_coverage_wrapper.sh

that now needs to run ${TEST_BINARY}_instrum, and also grab the corpus via:
cp -r "${TEST_BINARY}"_corpus fuzz_corpus/seed_corpus.

This is currently how bazel coverage is co-opted to run the fuzzing binary

asraa · 2021-01-25T15:19:03Z

@stefanbucur it looks like it passes CI now (including the fuzz coverage script, which runs fuzzing as normal and produces the output!)

Should this be moved out of DRAFT now?

stefanbucur · 2021-01-26T17:10:39Z

@stefanbucur it looks like it passes CI now (including the fuzz coverage script, which runs fuzzing as normal and produces the output!)

Should this be moved out of DRAFT now?

Yes, I think it's time to move this out of draft! This now works, but unfortunately I had to reduce some functionality due to the impact on build output size, as well as the build effort. More specifically:

The configuration transition that instruments the binary is now disabled, so we're still relying on the .bazelrc file to specify the instrumentation flags. This keeps the build output size and build effort low, while we can re-enable the transitions once Build failure caused by no-op starlark transition (friction between Starlark None and java null) bazelbuild/bazel#12888 is resolved.
The regression test that ships with rules_fuzzing is also disabled, thus still relying on the old way of using the cc_test with the args attribute configured. This is due to an unexpected interaction with RBE, which pulls in local build artifacts when running in sanitizer mode. This needs more investigation and we can easily enable this back once we have a fix.
Honggfuzz support is not yet enabled, since it pulls an extra Honggfuzz dependency that would need to be explicitly declared here. There is no issue with that, it's just that it felt like this should be an add-on PR sent out separately, if needed.

I would suggest though to do this PR in smaller, incremental sub-PRs, to minimize the risk of any disruption:

I will first send a PR that adds in the new rules_fuzzing dependencies and enables the new targets. All the old targets will still be there, including the OSS-Fuzz one, such that the OSS-Fuzz builds are not affected.
I will then send out an OSS-Fuzz PR that switches over the build.sh script from the old rules to the new rules. This should substantially simplify the build script.
Once OSS-Fuzz is switched over, I will send out a clean-up PR to remove any unused rules from the macro.

@asraa How does the plan sound?

asraa · 2021-01-26T17:25:39Z

Transition plan sounds great re OSS-Fuzz. My read is that between 1 and 2 there will never be a time where OSS-Fuzz can't build. That's ideal.

Tag me on PRs!