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

fuzz: route lookup and header finalization fuzzer. #4116

Merged
merged 3 commits into from
Aug 14, 2018

Conversation

htuch
Copy link
Member

@htuch htuch commented Aug 12, 2018

This was a pretty easy small test to add to gain coverage over all route resolution and header
finalization. Automatically generated corpus from //test/common/router:config_impl_test. To regenerate the corpus:

bazel run //test/common/router:config_impl_test \
     --test_env="ROUTE_CORPUS_PATH=$PWD/test/common/router/route_corpus"

Risk level: Low
Testing: Corpus coverage of 72.4% for config_impl.cc.

Signed-off-by: Harvey Tuch htuch@google.com

@htuch
Copy link
Member Author

htuch commented Aug 12, 2018

@mattklein123 I can't say I'm a fan of checking in a corpus this massive, even though it is automatically generated and we have instructions for regenerating it in the PR. The main issue I see in general is that corpus files will require care and feeding as APIs evolve (in particular here, where there is a wide API surface included and we know there will be churn over time).

An alternative is that I keep this PR sans the checked in corpus, and do the generation action as an explicit bazel run step in the oss-fuzz Docker image. This will be problematic though when we integrate with our internal fuzzers, which are Bazel driven.

Possibly could figure out something with a genrule, but this won't help in oss-fuzz land, since oss-fuzz only uses Bazel to build, then scrapes out the contents of the Bazel cache for the final link and execution outside of Bazel.

Thoughts?

This was a pretty easy small test to add to gain coverage over all route resolution and header
finalization.

Risk level: Low
Testing: automatically generated corpus from //test/common/router:config_impl_test. Instructions for
regeneration are at the top of the file. This provides a corpus coverage of 72.4% for
config_impl.cc.

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@mattklein123
Copy link
Member

An alternative is that I keep this PR sans the checked in corpus, and do the generation action as an explicit bazel run step in the oss-fuzz Docker image. This will be problematic though when we integrate with our internal fuzzers, which are Bazel driven.

Sorry I don't follow this. If it's a bazel step, why wouldn't it work with the internal fuzzers?

Possibly could figure out something with a genrule, but this won't help in oss-fuzz land, since oss-fuzz only uses Bazel to build, then scrapes out the contents of the Bazel cache for the final link and execution outside of Bazel.

Still confused why this wouldn't work. Couldn't we have a scrip that runs that puts the files in the right place?

@htuch
Copy link
Member Author

htuch commented Aug 13, 2018

If it's a bazel run step as written today, writing files outside of the sandbox, it doesn't integrate properly with the normal build dependency graph, it's a separate shell invocation rather than a Bazel target.

I'll take a look to see if our internal corpus inputs can take a genrule as a dependency, if that's the case, we could always do a separate bazel step in the oss-fuzz Docker image to invoke the genrule and scrape stuff out of the cache. It's a bit icky; this is the first fuzz test where we would have special case logic in the Docker image.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Member Author

htuch commented Aug 14, 2018

@mattklein123 I've removed all of the corpus except a single file and left a TODO to figure out the genrule, this will let us get started with adding this test into internal/external fuzz frameworks and we can iterate from there.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

I'm sorry you weren't able to pad your GH stats as much. 😉

@htuch htuch merged commit 5a7152d into envoyproxy:master Aug 14, 2018
@htuch htuch deleted the route-fuzz-test branch August 14, 2018 04:45
snowp added a commit to snowp/envoy that referenced this pull request Aug 14, 2018
* origin/master: (38 commits)
  test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114)
  perf: reduce the memory usage of LC Trie construction (envoyproxy#4117)
  test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127)
  test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141)
  rbac: add rbac network filter. (envoyproxy#4083)
  fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116)
  Set content-type and content-length (envoyproxy#4113)
  fault: use FractionalPercent for percent (envoyproxy#3978)
  test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134)
  Added cluster_name to load assignment config for static cluster (envoyproxy#4123)
  ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115)
  syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111)
  thrift_proxy: fix oneway bugs (envoyproxy#4025)
  Do not crash when converting YAML to JSON fails (envoyproxy#4110)
  config: allow unknown fields flag (take 2) (envoyproxy#4096)
  Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108)
  bazel: use GCS remote cache (envoyproxy#4050)
  Add thread local cache of overload action states (envoyproxy#4090)
  Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079)
  secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086)
  ...

Signed-off-by: Snow Pettersen <snowp@squareup.com>
@zuercher zuercher mentioned this pull request Aug 14, 2018
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