[fuzz] add fuzz tests for hpack encoding and decoding by asraa · Pull Request #13315 · envoyproxy/envoy

asraa · 2020-09-29T20:12:02Z

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

Commit Message: Adds tests for HPACK encoding and decoding with nghttp2. The fuzz test takes a set of headers, encodes them, decodes, and checks the results is equal to the input.
Performance: 700-800 exec/sec
Risk Level: Low
Testing: Ran with libfuzzer

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

asraa · 2020-09-29T20:13:13Z

/cc @antoniovicente @yanavlasov

htuch

Looks good. What does the performance looks like? Maybe file a bug in nghttp2 repo for them to create a similar fuzzer?

htuch · 2020-09-29T20:28:48Z

test/common/http/http2/hpack_fuzz_test.cc

+  ENVOY_LOG_MISC(trace, "Encoding headers {}", headers);
+  const absl::optional<Buffer::OwnedImpl> payload = encodeHeaders(input_nv);
+  if (!payload.has_value() || payload.value().getRawSlices().size() == 0) {
+    // An empty header map produces no payload, skip decoding.


What if the empty payload is due to some nghttp2 failure?

If nghttp2 throws a negative error code, payload will be a nullopt, which will return. I think this boils down to whether the team wants scope of fuzzer to be to simply check if encode/decode always work by sending headers through both and checking for equality, or if the team wants to explore header values that could cause nghttp2 to throw errors.

I think catching unexpected errors from nghttp2 would be a nice bonus, since we have a lot of ASSERTs in places around the returns from nghttp2 calls.

Done. Empty payload is now handled earlier, since in our codebase and here we pass .data(), which, if nullptr because of empty header map, will crash nghttp2 at runtime. That's kinda bad, but I went through the codebase and there's nowhere we supply headers via data() without at least one header. We should add a statement enforcing this though, maybe.

Since empty payload is not worrisome in encode, I catch errors with an ASSERT(result >= 0) now.

htuch · 2020-09-29T20:29:49Z

test/common/http/http2/hpack_fuzz_test.cc

+DEFINE_PROTO_FUZZER(const test::common::http::http2::HpackTestCase& input) {
+  // Validate headers.
+  try {
+    TestUtility::validate(input);


Maybe leave a TODO to make this even faster by skipping LPM and working with direct byte array representing lists of headers (let's say separated by any characters that aren't valid HTTP header vals).

done. using vectors rather than HeaderMap sped it up to about 800, adding verification via qsort and compare brought it back down to 700.

I was actually suggesting to skip using proto entirely, rather than skipping the HeaderMap, but up to you, I think it's fine to merge as is.

Yep, acked. The TODO about that is at the top of the file "// TODO(asraa): Speed up by using raw byte input and seperators rather than protobuf input."

asraa · 2020-09-29T20:54:35Z

Good question, performance started at around 650-700, and tapered down to 450. The obvious way to optimize would be to remove the header map construction and just stick to comparisons between protos or the nv arrays. originally i just had the comparison go test::fuzz::Headers input to test::fuzz::Headers decoded headers. But it has the overhead of protos or array sorting

I will do this, it's worth getting it to 1000.

htuch · 2020-09-29T22:30:18Z

@asraa up to you, happy to review in either state, just want to throw it out there.

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

htuch

LGTM, thanks!

htuch · 2020-10-02T21:11:27Z

test/common/http/http2/hpack_fuzz_test.cc

+DEFINE_PROTO_FUZZER(const test::common::http::http2::HpackTestCase& input) {
+  // Validate headers.
+  try {
+    TestUtility::validate(input);


I was actually suggesting to skip using proto entirely, rather than skipping the HeaderMap, but up to you, I think it's fine to merge as is.

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

github-actions · 2020-12-09T03:10:59Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

htuch · 2020-12-09T22:32:43Z

@asraa looks like this one was approved but there were some final CI check issues blocking merge.

jmarantz · 2020-12-21T18:40:47Z

ping when ready for review

/wait

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

jmarantz

looks fine modulo some nits.

test/common/http/http2/hpack_fuzz_test.cc

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

jmarantz · 2020-12-21T20:12:03Z

test/common/http/http2/hpack_fuzz_test.cc

+    // TODO(asraa): Consider adding flags in fuzzed input.
+    nva[i++] = {const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(header.key().data())),
+                const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(header.value().data())),
+                header.key().size(), header.value().size(), /*flags = */ 0};


I think we've not been using this pattern of arg-documentation in Envoy. However I like the previous state of having flags as a variable that's passed in, as it's self-documenting. The flags var could be const though.

Should be all set (only diff)

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

* master: (30 commits) Deflaked: Guarddog_impl_test (envoyproxy#14475) [fuzz] add fuzz tests for hpack encoding and decoding (envoyproxy#13315) [filters] Prevent a filter from sending local reply and continue (envoyproxy#14416) oauth2: improving coverage (envoyproxy#14479) owners: Change dio email address (envoyproxy#14498) macos build: Fix ninja install (envoyproxy#14495) http: use OptRef helper to reduce some boilerplate (envoyproxy#14361) doc: update test/integration/README.md (envoyproxy#14485) server: wait workers to start before draining parent. (envoyproxy#14319) api: relax inline_string length limitation in DataSource (envoyproxy#14461) oauth: properly stop filter chain when a response was sent (envoyproxy#14476) listener: deprecate use_proxy_proto (envoyproxy#14406) deps: update cel and remove a patch (envoyproxy#14473) preconnect: rename: (envoyproxy#14474) coverage: ratcheting limits (envoyproxy#14472) grpc mux: fix sending node again after stream is reset (envoyproxy#14080) [test] Replace printers_include with printers_lib. (envoyproxy#14442) tcp: nodelay in the new pool (envoyproxy#14453) test: replace mock_methodn macros with mock_method (envoyproxy#14450) tcp: extending tcp integration test (envoyproxy#14451) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

asraa added 2 commits September 29, 2020 16:06

add fuzz test

b33f075

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

fixup

55c4590

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

asraa requested a review from zasweq September 29, 2020 20:12

asraa assigned htuch Sep 29, 2020

htuch reviewed Sep 29, 2020

View reviewed changes

asraa added 2 commits October 2, 2020 16:06

don't ignore statuses and compare with vector

844f98f

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

fixup

c909e82

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

htuch previously approved these changes Oct 2, 2020

View reviewed changes

fix spelling

a9b2344

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

asraa dismissed htuch’s stale review via a9b2344 October 5, 2020 13:54

fix another spelling

f99ca68

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

htuch previously approved these changes Oct 7, 2020

View reviewed changes

fix compilation error

c89915d

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

asraa dismissed htuch’s stale review via c89915d October 7, 2020 14:21

asraa added 2 commits October 14, 2020 09:22

Merge remote-tracking branch 'upstream/master' into hpack-tests

fa4614c

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

fix compilation

c1a512a

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

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 10, 2020

mattklein123 assigned jmarantz and unassigned htuch Dec 18, 2020

repokitteh-read-only bot added the waiting label Dec 21, 2020

asraa added 2 commits December 21, 2020 13:47

Merge remote-tracking branch 'upstream/master' into hpack-tests

9ec6761

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

address clang tidy

e68ad9c

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

repokitteh-read-only bot removed the waiting label Dec 21, 2020

jmarantz reviewed Dec 21, 2020

View reviewed changes

address comments

f30ec09

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

jmarantz previously approved these changes Dec 21, 2020

View reviewed changes

make flags const

fd7af21

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

asraa dismissed jmarantz’s stale review via fd7af21 December 21, 2020 20:34

jmarantz approved these changes Dec 21, 2020

View reviewed changes

mattklein123 merged commit e90d674 into envoyproxy:master Dec 21, 2020

Conversation

asraa commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asraa commented Sep 29, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asraa Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asraa commented Sep 29, 2020

Uh oh!

htuch commented Sep 29, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 9, 2020

Uh oh!

htuch commented Dec 9, 2020

Uh oh!

jmarantz commented Dec 21, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asraa commented Sep 29, 2020 •

edited

Loading

asraa Oct 2, 2020 •

edited

Loading