fuzz: bare bones HCM fuzzer. #4118
Conversation
See caveats at the top of conn_manager_impl_fuzz_test.cc. HCM might not be the best place to be investing fuzzing effort, would like to see if a basic fuzzer can provide some win before investing more time in modeling HCM internals and building a corpus. Risk level: Low Testing: Supplied corpus has 56.7% coverage. Signed-off-by: Harvey Tuch <htuch@google.com>
|
@mattklein123 I'm a bit ambivalent about investing more time in HCM fuzzing until I've taken a step back and evaluated the efficacy of our existing fuzzers and this initial implementation. Once this PR and #4116 land, we'll have pretty good coverage of most of the data plane I think. What I'm planning on doing is working with the coverage/invocation/stability stats from ClusterFuzz and open issues to try and tune existing fuzzers, measure how good a job they are doing and tackle the blocker high priority issues that exist in the bug database. There's a bunch of optimization in terms of effectively using entropy that we're not doing in places. Ideally we drive future fuzzer development and work based on metrics such as coverage and bug reports, together with cost-benefit analysis. My experience so far has shown that some fuzzers are very easy to write (including corpus development) and give good pay off in terms of issues raised, others (including this PR) are really complicated and we need to see some reasonable benefit in terms of issues uncovered or peace-of-mind with complex implementations (e.g. header_map_impl.cc and its fuzzer) to justify them. |
There was a problem hiding this comment.
Agreed on burning down the existing bugs before investing more. I agree this will get increasingly complicated, and you are going to run up against bad filter behavior that we only have ASSERTs for today and not real error checking. (Long term though I think this would be a great way to check that we protect against filters doing obvious things that can crash us instead of at least panic/etc. with some useful error message).
Previously, we were picking up the default copy constructor, despite having effectively a copy constructor for the base class HeaderMap. This appears to resolve the leak in envoyproxy#4118. Risk level: Low Testing: Unit test added. Signed-off-by: Harvey Tuch <htuch@google.com>
Previously, since we had an implicitly deleted copy constructor, we picked up the implicitly created move constructor. This then performed a move operation on HeaderList, which is not safe to move because it contains iterator elements, which can point to end() and so are not safe when when moved, see the Notes in https://en.cppreference.com/w/cpp/container/list/list. In this PR, we explicitly mark HeaderList as non-copyable (also suppresses the implicit move constructor), then provide an explicit copy constructor and assignment operator for TestHeaderImpl. This appears to resolve the leak in #4118 (another win for fuzzing). Risk level: Low Testing: Unit test added. Signed-off-by: Harvey Tuch <htuch@google.com>
…#1938) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/envoyproxy/envoy: c1cc68d stats: refactoring MetricImpl without strings (envoyproxy#4190) 36809d8 fuzz: coverage profile generation instructions. (envoyproxy#4193) ba40cc9 upstream: rebuild cluster when health check config is changed (envoyproxy#4075) 05c0d52 build: use clang-6.0. (envoyproxy#4168) 01f403e thrift_proxy: introduce header transport (envoyproxy#4082) 564d256 tcp: allow connection pool callers to store protocol state (envoyproxy#4131) 3e1d643 configs: match latest API changes (envoyproxy#4185) bc6a10c Fix wrong mock function name. (envoyproxy#4187) e994c1c Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) 3d1325e Converting envoy configs to V2 (envoyproxy#2957) 8d0680f Add timestamp to HealthCheckEvent definition (envoyproxy#4119) 497efb9 server: handle non-EnvoyExceptions safely if thrown in constructor. (envoyproxy#4173) 6d6fafd config: strengthen validation for gRPC config sources. (envoyproxy#4171) 132302c fuzz: reduce log level when running under fuzz engine. (envoyproxy#4161) 7c04ac2 test: fix V6EmptyOptions in coverage with IPv6 enable (envoyproxy#4155) 1b2219b ci: remove deprecated bazel --batch option (envoyproxy#4166) 2db6a4c ci: update clang to version 6.0 in the Ubuntu build image. (envoyproxy#4157) 71152b7 ratelimit: Add ratelimit custom response headers (envoyproxy#4015) 3062874 ssl: make Ssl::Connection const everywhere (envoyproxy#4179) 706e262 Handle validation of non expiring tokens in jwt_authn filter (envoyproxy#4007) f06e958 fuzz: tag trivial fuzzers with no_fuzz. (envoyproxy#4156) 27fb1d3 thrift_proxy: add service name matching to router implementation (envoyproxy#4130) 8c189a5 Make over provisioning factor configurable (envoyproxy#4003) 6c08fb4 Making test less flaky (envoyproxy#4149) 592775b fuzz: bare bones HCM fuzzer. (envoyproxy#4118) For istio/istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Based around the mocking in //test/common/http:conn_manager_impl_test. See caveats at the top of conn_manager_impl_fuzz_test.cc. HCM might not be the best place to be
investing fuzzing effort, would like to see if a basic fuzzer can provide some win before investing
more time in modeling HCM internals and building a corpus.
Risk level: Low
Testing: Supplied corpus has 56.7% coverage.
Signed-off-by: Harvey Tuch htuch@google.com