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

Use FMT_THROW in fuzzing build mode #1650

Merged
merged 5 commits into from
Apr 29, 2020
Merged

Use FMT_THROW in fuzzing build mode #1650

merged 5 commits into from
Apr 29, 2020

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Apr 27, 2020

Make code properly build in fuzz mode with clang/gcc when -fno-exceptions is specified by using FMT_THROW in format files when building with FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION.

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

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

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

vitaut commented Apr 28, 2020

Thanks for the PR. Could you elaborate why this is needed? The {fmt}'s fuzz tests are always run with exceptions enabled in oss-fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/list?colspec=ID%20Type%20Component%20Status%20Proj%20Reported%20Owner%20Summary&q=proj%3Dlibfmt&can=1

@asraa
Copy link
Contributor Author

asraa commented Apr 28, 2020

Thanks! We have projects (Envoy, and others) that depend on fmt which are built with no exception mode, and are also fuzzed. These project's fuzz targets cannot build with the same production fmt lib dependency with exceptions disabled. Ideally, those project's fuzzers should model their production environment, they do not expect throws in their dependency.

@vitaut
Copy link
Contributor

vitaut commented Apr 28, 2020

I think a better fix would be to replace FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION with an {fmt}-specific macro (e.g. FMT_FUZZING) set when the FMT_FUZZ CMake compiler option is enabled:

if (FMT_FUZZ)

@asraa
Copy link
Contributor Author

asraa commented Apr 28, 2020

Thank you, that would work well for us! I will update the PR and fuzzing README to reflect that.

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

vitaut commented Apr 29, 2020

Is FUZZ_MODE documented anywhere?

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

asraa commented Apr 29, 2020

Sorry about that -- I mixed up the macros. I tested the fuzz targets in this branch and in master branch and didn't hit any OOMs at least while running fuzzer_one_arg

@asraa
Copy link
Contributor Author

asraa commented Apr 29, 2020

Both master and this branch start around 100k+ exec/sec, but my branch will go down to 40k sometimes. Any idea why that's happening?

@vitaut
Copy link
Contributor

vitaut commented Apr 29, 2020

Looks good but please

  1. Define FMT_FUZZ in CMake:
    if (FMT_FUZZ)
    target_compile_definitions(fmt PUBLIC FMT_FUZZ)
  1. Use #ifdef instead of #if to prevent warnings when FMT_FUZZ is not defined.

Any idea why that's happening?

Not sure.

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

asraa commented Apr 29, 2020

Done -- retested and the fuzzer now maintains 100k+ speed, I wonder if the warnings were slowing it down? Thank you!

@vitaut vitaut merged commit e2ff910 into fmtlib:master Apr 29, 2020
@vitaut
Copy link
Contributor

vitaut commented Apr 29, 2020

Thank you!

htuch pushed a commit to envoyproxy/envoy that referenced this pull request May 15, 2020
Fixes fuzz crashes in fmt::format (https://github.com/fmtlib/fmt/blob/0463665ef136d685fe07a564d93c782456456d3d/include/fmt/format.h#L703) on certain invalid protobuf inputs. fmt is patches with PR (fmtlib/fmt#1650) that replaces the in-house fuzzing resource management to an fmt specific fuzzing macro.
Additional Description: The regression test added shows that the proto in question is not unreasonably huge for Envoy. This is causing a high unexplained crash percentage for many fuzz tests on OSS-Fuzz. Also bump fmt.

Testing: Added regression test in server fuzz test, failed
bazel test test/server:server_fuzz_test --copt=-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION before
Related: #9623
Risk level: Low

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