bneradt commented on code in PR #11396:
URL: https://github.com/apache/trafficserver/pull/11396#discussion_r1619002253
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -339,14 +339,22 @@ XpackDynamicTable::insert_entry(const char *name, size_t
name_len, const char *v
}
// Make enough space to insert a new entry
- uint64_t required_size = static_cast<uint64_t>(name_len) +
static_cast<uint64_t>(value_len) + ADDITIONAL_32_BYTES;
+ uint64_t const required_storage_size = static_cast<uint64_t>(name_len) +
static_cast<uint64_t>(value_len);
+ uint64_t const required_size = required_storage_size +
ADDITIONAL_32_BYTES;
if (required_size > this->_available) {
if (!this->_make_space(required_size - this->_available)) {
// We can't insert a new entry because some stream(s) refer an entry
that need to be evicted or the header is too big to
// store. This is fine with HPACK, but not with QPACK.
return {UINT32_C(0), XpackLookupResult::MatchType::NONE};
}
}
+ // Protect against overrun: ensure that our storage has space for the new
+ // header.
+ if (this->_storage.will_overrun(required_storage_size)) {
Review Comment:
I appreciate the concern about release asserts, but guarding overruns is one
place I suggest we should use them. Overruns cause mysterious crashes anyway
(if we're lucky - otherwise we may be sending sensitive data to a host,
leading to a security issue), but it can be very difficult to find the
culprit because the crash happens after the fact with little or no evidence as
to who corrupted the memory. A release_assert at least makes the crash happen
deterministically at the point of where the overrun will happen.
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -339,14 +339,22 @@ XpackDynamicTable::insert_entry(const char *name, size_t
name_len, const char *v
}
// Make enough space to insert a new entry
- uint64_t required_size = static_cast<uint64_t>(name_len) +
static_cast<uint64_t>(value_len) + ADDITIONAL_32_BYTES;
+ uint64_t const required_storage_size = static_cast<uint64_t>(name_len) +
static_cast<uint64_t>(value_len);
+ uint64_t const required_size = required_storage_size +
ADDITIONAL_32_BYTES;
if (required_size > this->_available) {
if (!this->_make_space(required_size - this->_available)) {
// We can't insert a new entry because some stream(s) refer an entry
that need to be evicted or the header is too big to
// store. This is fine with HPACK, but not with QPACK.
return {UINT32_C(0), XpackLookupResult::MatchType::NONE};
}
}
+ // Protect against overrun: ensure that our storage has space for the new
+ // header.
+ if (this->_storage.will_overrun(required_storage_size)) {
Review Comment:
I appreciate the concern about release asserts, but guarding overruns is one
place I suggest we should use them. Overruns cause mysterious crashes anyway
(if we're lucky - otherwise we may be sending sensitive data to a host, leading
to a security issue), but it can be very difficult to find the culprit because
the crash happens after the fact with little or no evidence as to who corrupted
the memory. A release_assert at least makes the crash happen deterministically
at the point of where the overrun will happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
