Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions test/common/http/http2/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ load(
"envoy_cc_test",
"envoy_cc_test_library",
"envoy_package",
"envoy_proto_library",
)

licenses(["notice"]) # Apache 2
Expand Down Expand Up @@ -194,3 +195,23 @@ envoy_cc_fuzz_test(
"//test/common/http/http2:codec_impl_test_util",
],
)

envoy_proto_library(
name = "hpack_fuzz_proto",
srcs = ["hpack_fuzz.proto"],
deps = ["//test/fuzz:common_proto"],
)

envoy_cc_fuzz_test(
name = "hpack_fuzz_test",
srcs = ["hpack_fuzz_test.cc"],
corpus = "hpack_corpus",
external_deps = [
"nghttp2",
],
deps = [
":hpack_fuzz_proto_cc_proto",
"//test/fuzz:utility_lib",
"//test/test_common:utility_lib",
],
)

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions test/common/http/http2/hpack_corpus/example

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions test/common/http/http2/hpack_fuzz.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
syntax = "proto3";

package test.common.http.http2;

import "test/fuzz/common.proto";

import "validate/validate.proto";

// Structured input for hpack_fuzz_test.

message HpackTestCase {
test.fuzz.Headers headers = 1 [(validate.rules).message.required = true];
bool end_headers = 2;
}
147 changes: 147 additions & 0 deletions test/common/http/http2/hpack_fuzz_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
// Fuzzer for HPACK encoding and decoding.

#include "test/common/http/http2/hpack_fuzz.pb.validate.h"
#include "test/fuzz/fuzz_runner.h"
#include "test/fuzz/utility.h"
#include "test/test_common/utility.h"

#include "absl/container/fixed_array.h"
#include "nghttp2/nghttp2.h"

namespace Envoy {
namespace Http {
namespace Http2 {
namespace {

// Dynamic Header Table Size
constexpr int kHeaderTableSize = 4096;

absl::FixedArray<nghttp2_nv> createNameValueArray(const TestRequestHeaderMapImpl& input) {
const size_t nvlen = input.size();
absl::FixedArray<nghttp2_nv> nva(nvlen);
int i = 0;
input.iterate([&nva, &i](const HeaderEntry& header) -> HeaderMap::Iterate {
// TODO(asraa): Consider adding flags in fuzzed input.
uint8_t flags = 0;
nva[i] = {
const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(header.key().getStringView().data())),
const_cast<uint8_t*>(
reinterpret_cast<const uint8_t*>(header.value().getStringView().data())),
header.key().size(), header.value().size(), flags};
i++;
return HeaderMap::Iterate::Continue;
});

return nva;
}

absl::optional<Buffer::OwnedImpl> encodeHeaders(const absl::FixedArray<nghttp2_nv>& input_nv) {
// Create Deflater
nghttp2_hd_deflater* deflater;
const int rv = nghttp2_hd_deflate_new(&deflater, kHeaderTableSize);
ASSERT(rv == 0);

// Estimate the upper bound
const size_t buflen = nghttp2_hd_deflate_bound(deflater, input_nv.begin(), input_nv.size());

Buffer::RawSlice iovec;
Buffer::OwnedImpl payload;
payload.reserve(buflen, &iovec, 1);
ASSERT(iovec.len_ >= buflen);

// Encode using nghttp2
uint8_t* buf = reinterpret_cast<uint8_t*>(iovec.mem_);
const ssize_t result =
nghttp2_hd_deflate_hd(deflater, buf, buflen, input_nv.begin(), input_nv.size());
if (result < 0) {
ENVOY_LOG_MISC(trace, "Failed to decode with result {}", result);
nghttp2_hd_deflate_del(deflater);
return absl::nullopt;
}

iovec.len_ = result;
payload.commit(&iovec, 1);

// Delete deflater.
nghttp2_hd_deflate_del(deflater);

return payload;
}

TestRequestHeaderMapImpl decodeHeaders(const Buffer::OwnedImpl& payload, bool end_headers) {
// Create inflater
nghttp2_hd_inflater* inflater;
const int rv = nghttp2_hd_inflate_new(&inflater);
ASSERT(rv == 0);

// Decode using nghttp2
Buffer::RawSliceVector slices = payload.getRawSlices();
const int num_slices = slices.size();
ASSERT(num_slices == 1, absl::StrCat("number of slices ", num_slices));

nghttp2_nv decoded_nv;
TestRequestHeaderMapImpl decoded_headers;
int inflate_flags = 0;
while (slices[0].len_ > 0) {
ssize_t result = nghttp2_hd_inflate_hd2(inflater, &decoded_nv, &inflate_flags,
reinterpret_cast<uint8_t*>(slices[0].mem_),
slices[0].len_, end_headers);
// Decoding should not fail and data should not be left in slice.
ASSERT(result >= 0 && (slices[0].len_ = 0));

slices[0].mem_ = reinterpret_cast<void*>(reinterpret_cast<uint8_t*>(slices[0].mem_) + result);
Comment thread
asraa marked this conversation as resolved.
Outdated
slices[0].len_ -= result;

if (inflate_flags & NGHTTP2_HD_INFLATE_EMIT) {
// One header key value pair has been successfully decoded.
decoded_headers.addCopy(
std::string(reinterpret_cast<char*>(decoded_nv.name), decoded_nv.namelen),
std::string(reinterpret_cast<char*>(decoded_nv.value), decoded_nv.valuelen));
}
}

if (end_headers) {
nghttp2_hd_inflate_end_headers(inflater);
}

// Delete inflater
nghttp2_hd_inflate_del(inflater);

return decoded_headers;
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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."

} catch (const EnvoyException& e) {
ENVOY_LOG_MISC(trace, "EnvoyException: {}", e.what());
return;
}

// Create name value pairs from headers.
const TestRequestHeaderMapImpl headers =
Fuzz::fromHeaders<TestRequestHeaderMapImpl>(input.headers());
const absl::FixedArray<nghttp2_nv> input_nv = createNameValueArray(headers);

// Encode headers with nghttp2.
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@asraa asraa Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

return;
}

// Decode headers with nghttp2
const TestRequestHeaderMapImpl decoded_headers =
decodeHeaders(payload.value(), input.end_headers());
ENVOY_LOG_MISC(trace, "Decoded headers {}", decoded_headers);

// Verify that decoded == encoded.
FUZZ_ASSERT(headers == decoded_headers);
}

} // namespace
} // namespace Http2
} // namespace Http
} // namespace Envoy