Skip to content
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

Reimplement fuzzing instrumentation using Bazel transitions. #86

Merged
merged 12 commits into from
Dec 4, 2020

Conversation

stefanbucur
Copy link
Collaborator

This approach eliminates the need for inlining the instrumentation options in the bazelrc file and simplifies the adoption of the rules.

This approach eliminates the need for inlining the instrumentation options in the bazelrc file and simplifies the adoption of the rules.
Copy link

@markus-kusano markus-kusano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only nits

run: |
bazel run //examples:empty_fuzz_test_run --config=asan-libfuzzer -- --timeout_secs=5
- name: Run advanced smoke test

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion (I might be missing context): would it make sense to have a smoke test also for honggfuzz? Also perhaps the reproduction mode for libFuzzer? Maybe MSAN as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great point. I've replaced this set of steps with a separate "Smoke testing" job and enabled a matrix of configuration to test multiple combinations of config x fuzz target.

On this occasion, I discovered that the RE2 example actually triggers an MSAN error :) I will report this on the RE2 project itself and we can decide if we want to include this simple fuzz target as a RE2 fuzzer for OSS-Fuzz too. CC @inferno-chromium @oliverchang

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, feel free to resolve, leaving open for other's to take a look.

fuzzing/BUILD Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/private/instrument.bzl Show resolved Hide resolved
Copy link
Collaborator Author

@stefanbucur stefanbucur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough review! PTAL.

run: |
bazel run //examples:empty_fuzz_test_run --config=asan-libfuzzer -- --timeout_secs=5
- name: Run advanced smoke test
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great point. I've replaced this set of steps with a separate "Smoke testing" job and enabled a matrix of configuration to test multiple combinations of config x fuzz target.

On this occasion, I discovered that the RE2 example actually triggers an MSAN error :) I will report this on the RE2 project itself and we can decide if we want to include this simple fuzz target as a RE2 fuzzer for OSS-Fuzz too. CC @inferno-chromium @oliverchang

fuzzing/BUILD Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/instrum_opts.bzl Outdated Show resolved Hide resolved
fuzzing/private/instrument.bzl Show resolved Hide resolved
@stefanbucur stefanbucur merged commit e03b32a into master Dec 4, 2020
@stefanbucur stefanbucur deleted the fuzz-test-rule-provider branch December 4, 2020 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants