bneradt commented on code in PR #11396:
URL: https://github.com/apache/trafficserver/pull/11396#discussion_r1617650194
##########
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:
It may be should be redundant, but this is the overrun that this patch
addresses. Before this patch, this overrun happens because `_make_space` didn't
expand _storage before this patch. Per your observation, that expansion is
being added here.
In any case, rather than risk an overrun, I'm trying to add a defensive
check. I understand your comment though. With the patch, it should be
redundant. Maybe I should make this a release assert instead.
--
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
