-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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] add fuzz tests for hpack encoding and decoding #13315
Changes from 12 commits
b33f075
55c4590
844f98f
c909e82
a9b2344
f99ca68
c89915d
fa4614c
c1a512a
9ec6761
e68ad9c
f30ec09
fd7af21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,154 @@ | ||
// Fuzzer for HPACK encoding and decoding. | ||
// TODO(asraa): Speed up by using raw byte input and separators rather than protobuf input. | ||
|
||
#include <algorithm> | ||
|
||
#include "test/common/http/http2/hpack_fuzz.pb.validate.h" | ||
#include "test/fuzz/fuzz_runner.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; | ||
|
||
std::vector<nghttp2_nv> createNameValueArray(const test::fuzz::Headers& input) { | ||
const size_t nvlen = input.headers().size(); | ||
std::vector<nghttp2_nv> nva(nvlen); | ||
int i = 0; | ||
for (const auto& header : input.headers()) { | ||
// 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}; | ||
} | ||
|
||
return nva; | ||
} | ||
|
||
Buffer::OwnedImpl encodeHeaders(nghttp2_hd_deflater* deflater, | ||
const std::vector<nghttp2_nv>& input_nv) { | ||
// Estimate the upper bound | ||
const size_t buflen = nghttp2_hd_deflate_bound(deflater, input_nv.data(), 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_); | ||
ASSERT(input_nv.data() != nullptr); | ||
const ssize_t result = | ||
nghttp2_hd_deflate_hd(deflater, buf, buflen, input_nv.data(), input_nv.size()); | ||
ASSERT(result >= 0, absl::StrCat("Failed to decode with result ", result)); | ||
|
||
iovec.len_ = result; | ||
payload.commit(&iovec, 1); | ||
|
||
return payload; | ||
} | ||
|
||
std::vector<nghttp2_nv> decodeHeaders(nghttp2_hd_inflater* inflater, | ||
const Buffer::OwnedImpl& payload, bool end_headers) { | ||
// 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)); | ||
|
||
std::vector<nghttp2_nv> decoded_headers; | ||
int inflate_flags = 0; | ||
nghttp2_nv decoded_nv; | ||
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].mem_ = reinterpret_cast<uint8_t*>(slices[0].mem_) + result; | ||
slices[0].len_ -= result; | ||
|
||
if (inflate_flags & NGHTTP2_HD_INFLATE_EMIT) { | ||
// One header key value pair has been successfully decoded. | ||
decoded_headers.push_back(decoded_nv); | ||
} | ||
} | ||
|
||
if (end_headers) { | ||
nghttp2_hd_inflate_end_headers(inflater); | ||
} | ||
|
||
return decoded_headers; | ||
} | ||
|
||
struct NvComparator { | ||
inline bool operator()(const nghttp2_nv& a, const nghttp2_nv& b) { | ||
absl::string_view a_str(reinterpret_cast<char*>(a.name), a.namelen); | ||
absl::string_view b_str(reinterpret_cast<char*>(b.name), b.namelen); | ||
return a_str.compare(b_str); | ||
} | ||
}; | ||
|
||
DEFINE_PROTO_FUZZER(const test::common::http::http2::HpackTestCase& input) { | ||
// Validate headers. | ||
try { | ||
TestUtility::validate(input); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
std::vector<nghttp2_nv> input_nv = createNameValueArray(input.headers()); | ||
// Skip encoding empty headers. nghttp2 will throw a nullptr error on runtime if it receives a | ||
// nullptr input. | ||
if (!input_nv.data()) { | ||
return; | ||
} | ||
|
||
// Create Deflater and Inflater | ||
nghttp2_hd_deflater* deflater = nullptr; | ||
int rc = nghttp2_hd_deflate_new(&deflater, kHeaderTableSize); | ||
ASSERT(rc == 0); | ||
nghttp2_hd_inflater* inflater = nullptr; | ||
rc = nghttp2_hd_inflate_new(&inflater); | ||
ASSERT(rc == 0); | ||
|
||
// Encode headers with nghttp2. | ||
const Buffer::OwnedImpl payload = encodeHeaders(deflater, input_nv); | ||
ASSERT(!payload.getRawSlices().empty()); | ||
|
||
// Decode headers with nghttp2 | ||
std::vector<nghttp2_nv> output_nv = decodeHeaders(inflater, payload, input.end_headers()); | ||
|
||
// Verify that decoded == encoded. | ||
ASSERT(input_nv.size() == output_nv.size()); | ||
std::sort(input_nv.begin(), input_nv.end(), NvComparator()); | ||
std::sort(output_nv.begin(), output_nv.end(), NvComparator()); | ||
for (size_t i = 0; i < input_nv.size(); i++) { | ||
absl::string_view in_name = {reinterpret_cast<char*>(input_nv[i].name), input_nv[i].namelen}; | ||
absl::string_view out_name = {reinterpret_cast<char*>(output_nv[i].name), output_nv[i].namelen}; | ||
absl::string_view in_val = {reinterpret_cast<char*>(input_nv[i].value), input_nv[i].valuelen}; | ||
absl::string_view out_val = {reinterpret_cast<char*>(output_nv[i].value), | ||
output_nv[i].valuelen}; | ||
ASSERT(in_name == out_name); | ||
ASSERT(in_val == out_val); | ||
} | ||
|
||
// Delete inflater | ||
nghttp2_hd_inflate_del(inflater); | ||
// Delete deflater. | ||
nghttp2_hd_deflate_del(deflater); | ||
} | ||
|
||
} // namespace | ||
} // namespace Http2 | ||
} // namespace Http | ||
} // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be all set (only diff)