Skip to content

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

Merged
htuch merged 2 commits intoenvoyproxy:masterfrom
asraa:fix-fuzz-builds
Mar 13, 2020
Merged

[build] fix fuzz build mode macro and test runs#10322
htuch merged 2 commits intoenvoyproxy:masterfrom
asraa:fix-fuzz-builds

Conversation

@asraa
Copy link
Copy Markdown
Contributor

@asraa asraa commented Mar 10, 2020

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.

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>
@htuch
Copy link
Copy Markdown
Member

htuch commented Mar 11, 2020

@asraa how come RE2 behaves differently under fuzzing #defines than normally?

@asraa
Copy link
Copy Markdown
Contributor Author

asraa commented Mar 11, 2020

While I think this is horrible, RE2::GlobalReplace is modified in fuzzing mode!
https://github.com/google/re2/blob/572d6abf7f227053a4f8b83381fc4378714a2552/re2/re2.cc#L407


envoy_cc_test(
name = "config_impl_test",
copts = ["-UFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How would this end up being defined normally?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it's probably redundant. If this copt gets overrides by a CLI to define it, then yeah, it's not needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmm... is there a way to explicitly undefine it so that even if someone passes in a --copt=-DFUZZING_... to build a fuzz target, that the test would be executed with the macro undefined? researching bazel... but wondering if you know

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not familiar with a way to do that, unless we wrap the test in a shell scrip that squashes it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Honestly I'm going to make that change in this PR now and test against the OSS-Fuzz build. I feel pretty hesitant removing the fuzzing build macro from our OSS-Fuzz script.

Signed-off-by: Asra Ali <asraa@google.com>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit 82e4b26 into envoyproxy:master Mar 13, 2020
htuch pushed a commit that referenced this pull request Nov 25, 2020
Adds back FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION macro to CLI.

This is the actual fix for #10322, after nocopts was a dead end from RE2 issue google/re2#272.

Now when running real unit tests in fuzzing mode, we ignore the test result since we aren't testing for functionality. (config_impl_test doesn't need to succeed for route_fuzz_test to build).

Risk Level: Low
Testing: Testing fuzz mode fuzzers and regression style fuzzers locally

Signed-off-by: Asra Ali <asraa@google.com>
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