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

Add fuzz test for the CDN-Loop header parser #13183

Merged
merged 6 commits into from
Oct 2, 2020

Conversation

justin-mp
Copy link
Contributor

The CDN-Loop header parser is a non-trivial parser of untrusted user
data (headers!), so it’s worth fuzzing it.

Fixes #13179.

Risk Level: Low
Testing: bazel run //test/extensions/filters/http/cdn_loop:parser_fuzz_test --config asan-fuzzer
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Justin Mazzola Paluska [email protected]

The CDN-Loop header parser is a non-trivial parser of untrusted user
data (headers!), so it’s worth fuzzing it.

Fixes envoyproxy#13179.

Risk Level: Low
Testing: bazel run //test/extensions/filters/http/cdn_loop:parser_fuzz_test --config asan-fuzzer
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Justin Mazzola Paluska <[email protected]>
@justin-mp
Copy link
Contributor Author

Hi! Is there anything I can do to help push this review along?

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Cool! Thanks so much @justin-mp, this looks great as is! I have a couple of comments I'm curious to know about.

It looks like parseCdnInfoList is the highest level piece of the parser, so it should cover most other functions.

One thing to check might be how much coverage you get over cdn_loop/ directory -- you can do this with
VALIDATE_COVERAGE=false test/run_envoy_bazel_coverage.sh test/extensions/filters/http/cdn_loop:parser_fuzz_test. Are there major gaps after adding the unit testcases?

They’re named by the parser_test.cc test suite name and test name.

Signed-off-by: Justin Mazzola Paluska <[email protected]>
Signed-off-by: Justin Mazzola Paluska <[email protected]>
@justin-mp
Copy link
Contributor Author

One thing to check might be how much coverage you get over cdn_loop/ directory -- you can do this with
VALIDATE_COVERAGE=false test/run_envoy_bazel_coverage.sh test/extensions/filters/http/cdn_loop:parser_fuzz_test. Are there major gaps after adding the unit testcases?

I could not get this command to spit out anything! bazel reports ERROR: No test targets were found, yet testing was requested. When I try with FUZZ_COVERAGE=true to get it to run the fuzz test, I get complaints about missing coverage files!

$FUZZ_COVERAGE=true VALIDATE_COVERAGE=false test/run_envoy_bazel_coverage.sh test/extensions/filters/http/cdn_loop:parser_fuzz_test
..
[2,915 / 2,917] Testing //test/extensions/filters/http/cdn_loop:parser_fuzz_test; 40s linux-sandbox
rules_foreign_cc: Cleaning temp directories
INFO: LCOV coverage report is located at /home/justinmp/.cache/bazel/_bazel_justinmp/40165bea23e238c9a757d812b2d9ba58/execroot/envoy/bazel-out/_coverage/_coverage_report.dat
 and execpath is bazel-out/_coverage/_coverage_report.dat
INFO: From CoverageReport _coverage/_coverage_report.dat:
Oct 02, 2020 8:39:40 AM com.google.devtools.coverageoutputgenerator.Main getTracefiles
INFO: Found 1 tracefiles.
Oct 02, 2020 8:39:41 AM com.google.devtools.coverageoutputgenerator.Main lambda$parseFilesInParallel$0
INFO: Parsing file bazel-out/k8-fastbuild/testlogs/test/extensions/filters/http/cdn_loop/parser_fuzz_test/coverage.dat
Oct 02, 2020 8:39:41 AM com.google.devtools.coverageoutputgenerator.Main getGcovInfoFiles
INFO: No gcov info file found.
Oct 02, 2020 8:39:41 AM com.google.devtools.coverageoutputgenerator.Main getGcovJsonInfoFiles
INFO: No gcov json file found.
Oct 02, 2020 8:39:41 AM com.google.devtools.coverageoutputgenerator.Main getProfdataFileOrNull
INFO: No .profdata file found.
Oct 02, 2020 8:39:41 AM com.google.devtools.coverageoutputgenerator.Main runWithArgs
WARNING: There was no coverage found.
bazel: Leaving directory `/home/justinmp/.cache/bazel/_bazel_justinmp/40165bea23e238c9a757d812b2d9ba58/execroot/envoy/'
Target //test/extensions/filters/http/cdn_loop:parser_fuzz_test up-to-date:
  bazel-bin/test/extensions/filters/http/cdn_loop/parser_fuzz_test
INFO: Elapsed time: 308.369s, Critical Path: 122.36s
INFO: 1215 processes: 1215 linux-sandbox.
INFO: Build completed successfully, 1244 total actions
//test/extensions/filters/http/cdn_loop:parser_fuzz_test                 PASSED in 66.8s

Executed 1 out of 1 test: 1 test passes.
INFO: Build completed successfully, 1244 total actions
genhtml: ERROR: no valid records found in tracefile /home/justinmp/git/envoy/generated/fuzz_coverage/coverage.dat
Reading data file /home/justinmp/git/envoy/generated/fuzz_coverage/coverage.dat
HTML coverage report is in /home/justinmp/git/envoy/generated/fuzz_coverage/index.html

@asraa
Copy link
Contributor

asraa commented Oct 2, 2020

When I try with FUZZ_COVERAGE=true to get it to run the fuzz test, I get complaints about missing coverage files!

Hey Justin! Thanks for catching that mistake, in a rush I forgot to add that line.

These lines are expected:

INFO: No gcov info file found.
Oct 02, 2020 8:39:41 AM com.google.devtools.coverageoutputgenerator.Main getGcovJsonInfoFiles
INFO: No gcov json file found.
Oct 02, 2020 8:39:41 AM com.google.devtools.coverageoutputgenerator.Main getProfdataFileOrNull
INFO: No .profdata file found

WARNING: There was no coverage found.
I've hit this line before, trying to search what context I have about it, I definitely should add this to the README. This line happens when there's a clang mismatch going on (maybe clang-10 is used to build, clang-9 is used for coverage utilities like llvm-cov). It is probably likely to work with bazel.fuzz_coverage.
The PR's coverage script passed, though: https://storage.googleapis.com/envoy-pr/13183/fuzz_coverage/source/extensions/filters/http/cdn_loop/index.html and it looks like 93% coverage over parser.cc!

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me besides one comment
/wait

test/extensions/filters/http/cdn_loop/parser_fuzz_test.cc Outdated Show resolved Hide resolved
Signed-off-by: Justin Mazzola Paluska <[email protected]>
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks!

@asraa asraa merged commit ee65568 into envoyproxy:master Oct 2, 2020
@justin-mp justin-mp deleted the cdn-loop-parser-fuzzing branch October 5, 2020 10:13
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.

Fuzz test the CDN-Loop header parser
2 participants