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.

Sign up for GitHub

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

headers: fixing fast fail of size-validation #4269

Merged
merged 7 commits into from
Aug 30, 2018
Merged

headers: fixing fast fail of size-validation #4269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
176472e
one more OOM vector
alyssawilk Aug 27, 2018
f668193
reviewer comments
alyssawilk Aug 27, 2018
f39ec4c
Hopefully unbreaking the mac build. will write test tomorrow
alyssawilk Aug 27, 2018
8745ebd
one more test, fixing EXPECT_DEATH macro for asan/tsan
alyssawilk Aug 28, 2018
580e760
disabling proof of concept test
alyssawilk Aug 28, 2018
d3ffa9d
trying to fix coverage build
alyssawilk Aug 28, 2018
eefc16f
Merge branch 'master' into RELEASE_ASSERT
alyssawilk Aug 29, 2018
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
10 changes: 7 additions & 3 deletions source/common/http/header_map_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,12 @@ void HeaderString::append(const char* data, uint32_t size) {
// allocate and copy.
type_ = Type::Dynamic;
const uint64_t new_capacity = newCapacity(string_length_, size);
dynamic_capacity_ = std::max(MinDynamicCapacity, new_capacity);
if (dynamic_capacity_ != MinDynamicCapacity) {
if (new_capacity > MinDynamicCapacity) {
// TODO(alyssawilk) unit test.
validateCapacity(new_capacity);
Copy link
Member

Choose a reason for hiding this comment

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

For some reason, my comment was lost here. I was wondering if we could move validateCapacity up and make it unconditional?

dynamic_capacity_ = new_capacity;
} else {
dynamic_capacity_ = MinDynamicCapacity;
Copy link
Member

Choose a reason for hiding this comment

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

Yep, much cleaner now, thanks.

}
char* buf = static_cast<char*>(malloc(dynamic_capacity_));
RELEASE_ASSERT(buf != nullptr, "");
Expand All @@ -104,7 +107,8 @@ void HeaderString::append(const char* data, uint32_t size) {
}

case Type::Inline: {
if (size + 1 + string_length_ <= sizeof(inline_buffer_)) {
const uint64_t new_capacity = static_cast<uint64_t>(size) + 1 + string_length_;
if (new_capacity <= sizeof(inline_buffer_)) {
// Already inline and the new value fits in inline storage.
break;
}
Expand Down
7 changes: 7 additions & 0 deletions test/common/http/header_map_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,13 @@ TEST(HeaderMapImplTest, TestAppendHeader) {
}
}

TEST(HeaderMapImplTest, TestHeaderLengthChecks) {
HeaderString value;
value.setCopy("some;", 5);
EXPECT_DEATH(value.append(nullptr, std::numeric_limits<uint32_t>::max()),
"Trying to allocate overly large headers.");
}

TEST(HeaderMapImplTest, PseudoHeaderOrder) {
typedef testing::MockFunction<void(const std::string&, const std::string&)> MockCb;
MockCb cb;
Expand Down