Skip to content

Commit

Permalink
reviewer comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
  • Loading branch information
@alyssawilk
alyssawilk committed Aug 27, 2018
1 parent 176472e commit f668193
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 74 deletions.
16 changes: 8 additions & 8 deletions source/common/http/header_map_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ constexpr size_t MinDynamicCapacity{32};
// This includes the NULL (StringUtil::itoa technically only needs 21).
constexpr size_t MaxIntegerLength{32};

size_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) {
return (static_cast<size_t>(existing_capacity) + size_to_append) * 2;
uint64_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) {
return (static_cast<uint64_t>(existing_capacity) + size_to_append) * 2;
}

void validateCapacity(size_t new_capacity) {
void validateCapacity(uint64_t new_capacity) {
// If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
// imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(),
Expand Down Expand Up @@ -91,10 +91,10 @@ void HeaderString::append(const char* data, uint32_t size) {
// Rather than be too clever and optimize this uncommon case, we dynamically
// allocate and copy.
type_ = Type::Dynamic;
dynamic_capacity_ =
std::max(MinDynamicCapacity, static_cast<size_t>((string_length_ + size) * 2));
const uint64_t new_capacity = newCapacity(string_length_, size);
dynamic_capacity_ = std::max(MinDynamicCapacity, new_capacity);
if (dynamic_capacity_ != MinDynamicCapacity) {
validateCapacity(newCapacity(string_length_, size));
validateCapacity(new_capacity);
}
char* buf = static_cast<char*>(malloc(dynamic_capacity_));
RELEASE_ASSERT(buf != nullptr, "");
Expand All @@ -115,7 +115,7 @@ void HeaderString::append(const char* data, uint32_t size) {
case Type::Dynamic: {
// We can get here either because we didn't fit in inline or we are already dynamic.
if (type_ == Type::Inline) {
const size_t new_capacity = newCapacity(string_length_, size);
const uint64_t new_capacity = newCapacity(string_length_, size);
validateCapacity(new_capacity);
buffer_.dynamic_ = static_cast<char*>(malloc(new_capacity));
RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
Expand All @@ -124,7 +124,7 @@ void HeaderString::append(const char* data, uint32_t size) {
type_ = Type::Dynamic;
} else {
if (size + 1 + string_length_ > dynamic_capacity_) {
const size_t new_capacity = newCapacity(string_length_, size);
const uint64_t new_capacity = newCapacity(string_length_, size);
validateCapacity(new_capacity);

// Need to reallocate.
Expand Down
131 changes: 65 additions & 66 deletions test/common/router/header_formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,10 @@ TEST_F(RequestInfoHeaderFormatterTest, UserDefinedHeadersConsideredHarmful) {
// This must be an inline header to get the append-in-place semantics.
const char* header_name = "connection";
Protobuf::RepeatedPtrField<envoy::api::v2::core::HeaderValueOption> to_add;
int loop_factor = 10;
uint64_t length = std::numeric_limits<uint32_t>::max() / loop_factor;
const uint32_t num_header_chunks = 10;
const uint64_t length = std::numeric_limits<uint32_t>::max() / num_header_chunks;
std::string really_long_string(length + 1, 'a');
std::cerr << "really long string " << really_long_string.size() << std::endl;
for (int i = 0; i < loop_factor; ++i) {
for (uint32_t i = 0; i < num_header_chunks; ++i) {
envoy::api::v2::core::HeaderValueOption* header = to_add.Add();
header->mutable_header()->set_key(header_name);
header->mutable_header()->set_value(really_long_string);
Expand All @@ -176,9 +175,9 @@ TEST_F(RequestInfoHeaderFormatterTest, UserDefinedHeadersConsideredHarmful) {

HeaderParserPtr req_header_parser = HeaderParser::configure(to_add);

Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
EXPECT_DEATH(req_header_parser->evaluateHeaders(headerMap, request_info),
EXPECT_DEATH(req_header_parser->evaluateHeaders(header_map, request_info),
"Trying to allocate overly large headers.");
}

Expand Down Expand Up @@ -383,14 +382,14 @@ TEST(HeaderParserTest, TestParseInternal) {

HeaderParserPtr req_header_parser = HeaderParser::configure(to_add);

Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
req_header_parser->evaluateHeaders(headerMap, request_info);
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
req_header_parser->evaluateHeaders(header_map, request_info);

std::string descriptor = fmt::format("for test case input: {}", test_case.input_);

EXPECT_TRUE(headerMap.has("x-header")) << descriptor;
EXPECT_TRUE(header_map.has("x-header")) << descriptor;
EXPECT_TRUE(test_case.expected_output_) << descriptor;
EXPECT_EQ(test_case.expected_output_.value(), headerMap.get_("x-header")) << descriptor;
EXPECT_EQ(test_case.expected_output_.value(), header_map.get_("x-header")) << descriptor;
}
}

Expand All @@ -410,10 +409,10 @@ TEST(HeaderParserTest, EvaluateHeaders) {
)EOF";
HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("x-client-ip"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("x-client-ip"));
}

TEST(HeaderParserTest, EvaluateEmptyHeaders) {
Expand All @@ -432,15 +431,15 @@ TEST(HeaderParserTest, EvaluateEmptyHeaders) {
)EOF";
HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
std::shared_ptr<NiceMock<Envoy::Upstream::MockHostDescription>> host(
new NiceMock<Envoy::Upstream::MockHostDescription>());
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
auto metadata = std::make_shared<envoy::api::v2::core::Metadata>();
ON_CALL(request_info, upstreamHost()).WillByDefault(Return(host));
ON_CALL(*host, metadata()).WillByDefault(Return(metadata));
req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_FALSE(headerMap.has("x-key"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_FALSE(header_map.has("x-key"));
}

TEST(HeaderParserTest, EvaluateStaticHeaders) {
Expand All @@ -459,11 +458,11 @@ TEST(HeaderParserTest, EvaluateStaticHeaders) {
)EOF";
HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("static-header"));
EXPECT_EQ("static-value", headerMap.get_("static-header"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("static-header"));
EXPECT_EQ("static-value", header_map.get_("static-header"));
}

TEST(HeaderParserTest, EvaluateCompoundHeaders) {
Expand Down Expand Up @@ -500,7 +499,7 @@ match: { prefix: "/new_endpoint" }

HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromV2Yaml(yaml).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
absl::optional<Envoy::Http::Protocol> protocol = Envoy::Http::Protocol::Http11;
ON_CALL(request_info, protocol()).WillByDefault(ReturnPointee(&protocol));
Expand All @@ -519,31 +518,31 @@ match: { prefix: "/new_endpoint" }
)EOF"));
ON_CALL(*host, metadata()).WillByDefault(Return(metadata));

req_header_parser->evaluateHeaders(headerMap, request_info);
req_header_parser->evaluateHeaders(header_map, request_info);

EXPECT_TRUE(headerMap.has("x-prefix"));
EXPECT_EQ("prefix-127.0.0.1", headerMap.get_("x-prefix"));
EXPECT_TRUE(header_map.has("x-prefix"));
EXPECT_EQ("prefix-127.0.0.1", header_map.get_("x-prefix"));

EXPECT_TRUE(headerMap.has("x-suffix"));
EXPECT_EQ("127.0.0.1-suffix", headerMap.get_("x-suffix"));
EXPECT_TRUE(header_map.has("x-suffix"));
EXPECT_EQ("127.0.0.1-suffix", header_map.get_("x-suffix"));

EXPECT_TRUE(headerMap.has("x-both"));
EXPECT_EQ("prefix-127.0.0.1-suffix", headerMap.get_("x-both"));
EXPECT_TRUE(header_map.has("x-both"));
EXPECT_EQ("prefix-127.0.0.1-suffix", header_map.get_("x-both"));

EXPECT_TRUE(headerMap.has("x-escaping-1"));
EXPECT_EQ("%127.0.0.1%", headerMap.get_("x-escaping-1"));
EXPECT_TRUE(header_map.has("x-escaping-1"));
EXPECT_EQ("%127.0.0.1%", header_map.get_("x-escaping-1"));

EXPECT_TRUE(headerMap.has("x-escaping-2"));
EXPECT_EQ("%%%", headerMap.get_("x-escaping-2"));
EXPECT_TRUE(header_map.has("x-escaping-2"));
EXPECT_EQ("%%%", header_map.get_("x-escaping-2"));

EXPECT_TRUE(headerMap.has("x-multi"));
EXPECT_EQ("HTTP/1.1 from 127.0.0.1", headerMap.get_("x-multi"));
EXPECT_TRUE(header_map.has("x-multi"));
EXPECT_EQ("HTTP/1.1 from 127.0.0.1", header_map.get_("x-multi"));

EXPECT_TRUE(headerMap.has("x-multi-back-to-back"));
EXPECT_EQ("HTTP/1.1127.0.0.1", headerMap.get_("x-multi-back-to-back"));
EXPECT_TRUE(header_map.has("x-multi-back-to-back"));
EXPECT_EQ("HTTP/1.1127.0.0.1", header_map.get_("x-multi-back-to-back"));

EXPECT_TRUE(headerMap.has("x-metadata"));
EXPECT_EQ("value", headerMap.get_("x-metadata"));
EXPECT_TRUE(header_map.has("x-metadata"));
EXPECT_EQ("value", header_map.get_("x-metadata"));
}

TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) {
Expand Down Expand Up @@ -585,29 +584,29 @@ TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) {

HeaderParserPtr req_header_parser =
Router::HeaderParser::configure(route_action.request_headers_to_add());
Http::TestHeaderMapImpl headerMap{
Http::TestHeaderMapImpl header_map{
{":method", "POST"}, {"static-header", "old-value"}, {"x-client-ip", "0.0.0.0"}};

NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
const SystemTime start_time(std::chrono::microseconds(1522796769123456));
EXPECT_CALL(request_info, startTime()).Times(3).WillRepeatedly(Return(start_time));

req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("static-header"));
EXPECT_EQ("static-value", headerMap.get_("static-header"));
EXPECT_TRUE(headerMap.has("x-client-ip"));
EXPECT_EQ("127.0.0.1", headerMap.get_("x-client-ip"));
EXPECT_TRUE(headerMap.has("x-request-start"));
EXPECT_EQ("1522796769123", headerMap.get_("x-request-start"));
EXPECT_TRUE(headerMap.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", headerMap.get_("x-request-start-default"));
EXPECT_TRUE(headerMap.has("x-request-start-range"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("static-header"));
EXPECT_EQ("static-value", header_map.get_("static-header"));
EXPECT_TRUE(header_map.has("x-client-ip"));
EXPECT_EQ("127.0.0.1", header_map.get_("x-client-ip"));
EXPECT_TRUE(header_map.has("x-request-start"));
EXPECT_EQ("1522796769123", header_map.get_("x-request-start"));
EXPECT_TRUE(header_map.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", header_map.get_("x-request-start-default"));
EXPECT_TRUE(header_map.has("x-request-start-range"));
EXPECT_EQ("123456000, 1, 12, 123, 1234, 12345, 123456, 1234560, 12345600, 123456000",
headerMap.get_("x-request-start-range"));
header_map.get_("x-request-start-range"));

typedef std::map<std::string, int> CountMap;
CountMap counts;
headerMap.iterate(
header_map.iterate(
[](const Http::HeaderEntry& header, void* cb_v) -> Http::HeaderMap::Iterate {
CountMap* m = static_cast<CountMap*>(cb_v);
std::string key = std::string{header.key().c_str()};
Expand Down Expand Up @@ -662,29 +661,29 @@ match: { prefix: "/new_endpoint" }
const auto route = parseRouteFromV2Yaml(yaml).route();
HeaderParserPtr resp_header_parser =
HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}, {"x-safe", "safe"}, {"x-nope", "nope"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}, {"x-safe", "safe"}, {"x-nope", "nope"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;

// Initialize start_time as 2018-04-03T23:06:09.123Z in microseconds.
const SystemTime start_time(std::chrono::microseconds(1522796769123456));
EXPECT_CALL(request_info, startTime()).Times(7).WillRepeatedly(Return(start_time));

resp_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("x-client-ip"));
EXPECT_TRUE(headerMap.has("x-request-start-multiple"));
EXPECT_TRUE(headerMap.has("x-safe"));
EXPECT_FALSE(headerMap.has("x-nope"));
EXPECT_TRUE(headerMap.has("x-request-start"));
EXPECT_EQ("1522796769.123", headerMap.get_("x-request-start"));
resp_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("x-client-ip"));
EXPECT_TRUE(header_map.has("x-request-start-multiple"));
EXPECT_TRUE(header_map.has("x-safe"));
EXPECT_FALSE(header_map.has("x-nope"));
EXPECT_TRUE(header_map.has("x-request-start"));
EXPECT_EQ("1522796769.123", header_map.get_("x-request-start"));
EXPECT_EQ("1522796769.123 2018-04-03T23:06:09.123Z 1522796769",
headerMap.get_("x-request-start-multiple"));
EXPECT_TRUE(headerMap.has("x-request-start-f"));
EXPECT_EQ("f", headerMap.get_("x-request-start-f"));
EXPECT_TRUE(headerMap.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", headerMap.get_("x-request-start-default"));
EXPECT_TRUE(headerMap.has("x-request-start-range"));
header_map.get_("x-request-start-multiple"));
EXPECT_TRUE(header_map.has("x-request-start-f"));
EXPECT_EQ("f", header_map.get_("x-request-start-f"));
EXPECT_TRUE(header_map.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", header_map.get_("x-request-start-default"));
EXPECT_TRUE(header_map.has("x-request-start-range"));
EXPECT_EQ("123456000, 1, 12, 123, 1234, 12345, 123456, 1234560, 12345600, 123456000",
headerMap.get_("x-request-start-range"));
header_map.get_("x-request-start-range"));
}

} // namespace Router
Expand Down

0 comments on commit f668193

Please sign in to comment.