This is an automated email from the ASF dual-hosted git repository. zwoop pushed a commit to branch 9.1.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 1ae48be5f5970d6af2df944f8c3200c354a05715 Author: Masaori Koshiba <[email protected]> AuthorDate: Tue Feb 2 13:17:58 2021 +0900 Cleanup: Get rid of MIMEFieldWrapper from HPACK encoding (#6520) --- proxy/http2/HPACK.cc | 256 ++++++++++------------ proxy/http2/HPACK.h | 20 +- proxy/http2/unit_tests/test_HpackIndexingTable.cc | 17 +- 3 files changed, 135 insertions(+), 158 deletions(-) diff --git a/proxy/http2/HPACK.cc b/proxy/http2/HPACK.cc index df8d212..81ed795 100644 --- a/proxy/http2/HPACK.cc +++ b/proxy/http2/HPACK.cc @@ -24,6 +24,7 @@ #include "HPACK.h" #include "tscpp/util/LocalBuffer.h" +#include "tscpp/util/TextView.h" namespace { @@ -98,76 +99,71 @@ typedef enum { TS_HPACK_STATIC_TABLE_ENTRY_NUM } TS_HPACK_STATIC_TABLE_ENTRY; -struct StaticTable { - StaticTable(const char *n, const char *v) : name(n), value(v), name_size(strlen(name)), value_size(strlen(value)) {} - const char *name; - const char *value; - const int name_size; - const int value_size; -}; - -static const StaticTable STATIC_TABLE[] = {{"", ""}, - {":authority", ""}, - {":method", "GET"}, - {":method", "POST"}, - {":path", "/"}, - {":path", "/index.html"}, - {":scheme", "http"}, - {":scheme", "https"}, - {":status", "200"}, - {":status", "204"}, - {":status", "206"}, - {":status", "304"}, - {":status", "400"}, - {":status", "404"}, - {":status", "500"}, - {"accept-charset", ""}, - {"accept-encoding", "gzip, deflate"}, - {"accept-language", ""}, - {"accept-ranges", ""}, - {"accept", ""}, - {"access-control-allow-origin", ""}, - {"age", ""}, - {"allow", ""}, - {"authorization", ""}, - {"cache-control", ""}, - {"content-disposition", ""}, - {"content-encoding", ""}, - {"content-language", ""}, - {"content-length", ""}, - {"content-location", ""}, - {"content-range", ""}, - {"content-type", ""}, - {"cookie", ""}, - {"date", ""}, - {"etag", ""}, - {"expect", ""}, - {"expires", ""}, - {"from", ""}, - {"host", ""}, - {"if-match", ""}, - {"if-modified-since", ""}, - {"if-none-match", ""}, - {"if-range", ""}, - {"if-unmodified-since", ""}, - {"last-modified", ""}, - {"link", ""}, - {"location", ""}, - {"max-forwards", ""}, - {"proxy-authenticate", ""}, - {"proxy-authorization", ""}, - {"range", ""}, - {"referer", ""}, - {"refresh", ""}, - {"retry-after", ""}, - {"server", ""}, - {"set-cookie", ""}, - {"strict-transport-security", ""}, - {"transfer-encoding", ""}, - {"user-agent", ""}, - {"vary", ""}, - {"via", ""}, - {"www-authenticate", ""}}; +constexpr HpackHeaderField STATIC_TABLE[] = {{"", ""}, + {":authority", ""}, + {":method", "GET"}, + {":method", "POST"}, + {":path", "/"}, + {":path", "/index.html"}, + {":scheme", "http"}, + {":scheme", "https"}, + {":status", "200"}, + {":status", "204"}, + {":status", "206"}, + {":status", "304"}, + {":status", "400"}, + {":status", "404"}, + {":status", "500"}, + {"accept-charset", ""}, + {"accept-encoding", "gzip, deflate"}, + {"accept-language", ""}, + {"accept-ranges", ""}, + {"accept", ""}, + {"access-control-allow-origin", ""}, + {"age", ""}, + {"allow", ""}, + {"authorization", ""}, + {"cache-control", ""}, + {"content-disposition", ""}, + {"content-encoding", ""}, + {"content-language", ""}, + {"content-length", ""}, + {"content-location", ""}, + {"content-range", ""}, + {"content-type", ""}, + {"cookie", ""}, + {"date", ""}, + {"etag", ""}, + {"expect", ""}, + {"expires", ""}, + {"from", ""}, + {"host", ""}, + {"if-match", ""}, + {"if-modified-since", ""}, + {"if-none-match", ""}, + {"if-range", ""}, + {"if-unmodified-since", ""}, + {"last-modified", ""}, + {"link", ""}, + {"location", ""}, + {"max-forwards", ""}, + {"proxy-authenticate", ""}, + {"proxy-authorization", ""}, + {"range", ""}, + {"referer", ""}, + {"refresh", ""}, + {"retry-after", ""}, + {"server", ""}, + {"set-cookie", ""}, + {"strict-transport-security", ""}, + {"transfer-encoding", ""}, + {"user-agent", ""}, + {"vary", ""}, + {"via", ""}, + {"www-authenticate", ""}}; + +constexpr std::string_view HPACK_HDR_FIELD_COOKIE = STATIC_TABLE[TS_HPACK_STATIC_TABLE_COOKIE].name; +constexpr std::string_view HPACK_HDR_FIELD_AUTHORIZATION = STATIC_TABLE[TS_HPACK_STATIC_TABLE_AUTHORIZATION].name; /** Threshold for total HdrHeap size which used by HPAK Dynamic Table. @@ -227,19 +223,18 @@ hpack_parse_field_type(uint8_t ftype) namespace HpackStaticTable { HpackLookupResult - lookup(const char *name, int name_len, const char *value, int value_len) + lookup(const HpackHeaderField &header) { HpackLookupResult result; for (unsigned int index = 1; index < TS_HPACK_STATIC_TABLE_ENTRY_NUM; ++index) { - const char *table_name = STATIC_TABLE[index].name; - int table_name_len = STATIC_TABLE[index].name_size; - const char *table_value = STATIC_TABLE[index].value; - int table_value_len = STATIC_TABLE[index].value_size; + std::string_view name = STATIC_TABLE[index].name; + std::string_view value = STATIC_TABLE[index].value; + // TODO: replace `strcasecmp` with `memcmp` // Check whether name (and value) are matched - if (ptr_len_casecmp(name, name_len, table_name, table_name_len) == 0) { - if ((value_len == table_value_len) && (memcmp(value, table_value, value_len) == 0)) { + if (strcasecmp(header.name, name) == 0) { + if (memcmp(header.value, value) == 0) { result.index = index; result.index_type = HpackIndex::STATIC; result.match_type = HpackMatch::EXACT; @@ -261,19 +256,10 @@ namespace HpackStaticTable // HpackIndexingTable // HpackLookupResult -HpackIndexingTable::lookup(const MIMEFieldWrapper &field) const -{ - int target_name_len = 0, target_value_len = 0; - const char *target_name = field.name_get(&target_name_len); - const char *target_value = field.value_get(&target_value_len); - return lookup(target_name, target_name_len, target_value, target_value_len); -} - -HpackLookupResult -HpackIndexingTable::lookup(const char *name, int name_len, const char *value, int value_len) const +HpackIndexingTable::lookup(const HpackHeaderField &header) const { // static table - HpackLookupResult result = HpackStaticTable::lookup(name, name_len, value, value_len); + HpackLookupResult result = HpackStaticTable::lookup(header); // if result is not EXACT match, lookup dynamic table if (result.match_type == HpackMatch::EXACT) { @@ -281,8 +267,7 @@ HpackIndexingTable::lookup(const char *name, int name_len, const char *value, in } // dynamic table - if (HpackLookupResult dt_result = this->_dynamic_table.lookup(name, name_len, value, value_len); - dt_result.match_type == HpackMatch::EXACT) { + if (HpackLookupResult dt_result = this->_dynamic_table.lookup(header); dt_result.match_type == HpackMatch::EXACT) { return dt_result; } @@ -299,8 +284,8 @@ HpackIndexingTable::get_header_field(uint32_t index, MIMEFieldWrapper &field) co if (index < TS_HPACK_STATIC_TABLE_ENTRY_NUM) { // static table - field.name_set(STATIC_TABLE[index].name, STATIC_TABLE[index].name_size); - field.value_set(STATIC_TABLE[index].value, STATIC_TABLE[index].value_size); + field.name_set(STATIC_TABLE[index].name.data(), STATIC_TABLE[index].name.size()); + field.value_set(STATIC_TABLE[index].value.data(), STATIC_TABLE[index].value.size()); } else if (index < TS_HPACK_STATIC_TABLE_ENTRY_NUM + _dynamic_table.length()) { // dynamic table const MIMEField *m_field = _dynamic_table.get_header_field(index - TS_HPACK_STATIC_TABLE_ENTRY_NUM); @@ -322,9 +307,9 @@ HpackIndexingTable::get_header_field(uint32_t index, MIMEFieldWrapper &field) co } void -HpackIndexingTable::add_header_field(const MIMEField *field) +HpackIndexingTable::add_header_field(const HpackHeaderField &header) { - _dynamic_table.add_header_field(field); + _dynamic_table.add_header_field(header); } uint32_t @@ -376,12 +361,9 @@ HpackDynamicTable::get_header_field(uint32_t index) const } void -HpackDynamicTable::add_header_field(const MIMEField *field) +HpackDynamicTable::add_header_field(const HpackHeaderField &header) { - int name_len, value_len; - const char *name = field->name_get(&name_len); - const char *value = field->value_get(&value_len); - uint32_t header_size = ADDITIONAL_OCTETS + name_len + value_len; + uint32_t header_size = ADDITIONAL_OCTETS + header.name.size() + header.value.size(); if (header_size > _maximum_size) { // [RFC 7541] 4.4. Entry Eviction When Adding New Entries @@ -403,31 +385,28 @@ HpackDynamicTable::add_header_field(const MIMEField *field) this->_current_size += header_size; this->_evict_overflowed_entries(); - MIMEField *new_field = this->_mhdr->field_create(name, name_len); - new_field->value_set(this->_mhdr->m_heap, this->_mhdr->m_mime, value, value_len); + MIMEField *new_field = this->_mhdr->field_create(header.name.data(), header.name.size()); + new_field->value_set(this->_mhdr->m_heap, this->_mhdr->m_mime, header.value.data(), header.value.size()); this->_mhdr->field_attach(new_field); this->_headers.push_front(new_field); } } HpackLookupResult -HpackDynamicTable::lookup(const char *name, int name_len, const char *value, int value_len) const +HpackDynamicTable::lookup(const HpackHeaderField &header) const { HpackLookupResult result; const unsigned int entry_num = TS_HPACK_STATIC_TABLE_ENTRY_NUM + this->length(); for (unsigned int index = TS_HPACK_STATIC_TABLE_ENTRY_NUM; index < entry_num; ++index) { - const MIMEField *m_field = this->get_header_field(index - TS_HPACK_STATIC_TABLE_ENTRY_NUM); - - int table_name_len = 0; - const char *table_name = m_field->name_get(&table_name_len); - - int table_value_len = 0; - const char *table_value = m_field->value_get(&table_value_len); + const MIMEField *m_field = this->_headers.at(index - TS_HPACK_STATIC_TABLE_ENTRY_NUM); + std::string_view name = m_field->name_get(); + std::string_view value = m_field->value_get(); + // TODO: replace `strcasecmp` with `memcmp` // Check whether name (and value) are matched - if (ptr_len_casecmp(name, name_len, table_name, table_name_len) == 0) { - if ((value_len == table_value_len) && (memcmp(value, table_value, value_len) == 0)) { + if (strcasecmp(header.name, name) == 0) { + if (memcmp(header.value, value) == 0) { result.index = index; result.index_type = HpackIndex::DYNAMIC; result.match_type = HpackMatch::EXACT; @@ -559,7 +538,7 @@ encode_indexed_header_field(uint8_t *buf_start, const uint8_t *buf_end, uint32_t } int64_t -encode_literal_header_field_with_indexed_name(uint8_t *buf_start, const uint8_t *buf_end, const MIMEFieldWrapper &header, +encode_literal_header_field_with_indexed_name(uint8_t *buf_start, const uint8_t *buf_end, const HpackHeaderField &header, uint32_t index, HpackIndexingTable &indexing_table, HpackField type) { uint8_t *p = buf_start; @@ -570,7 +549,7 @@ encode_literal_header_field_with_indexed_name(uint8_t *buf_start, const uint8_t switch (type) { case HpackField::INDEXED_LITERAL: - indexing_table.add_header_field(header.field_get()); + indexing_table.add_header_field(header); prefix = 6; flag = 0x40; break; @@ -601,20 +580,18 @@ encode_literal_header_field_with_indexed_name(uint8_t *buf_start, const uint8_t p += len; // Value String - int value_len; - const char *value = header.value_get(&value_len); - len = xpack_encode_string(p, buf_end, value, value_len); + len = xpack_encode_string(p, buf_end, header.value.data(), header.value.size()); if (len == -1) { return -1; } p += len; - Debug("hpack_encode", "Encoded field: %d: %.*s", index, value_len, value); + Debug("hpack_encode", "Encoded field: %d: %.*s", index, static_cast<int>(header.value.size()), header.value.data()); return p - buf_start; } int64_t -encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf_end, const MIMEFieldWrapper &header, +encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf_end, const HpackHeaderField &header, HpackIndexingTable &indexing_table, HpackField type) { uint8_t *p = buf_start; @@ -625,7 +602,7 @@ encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf switch (type) { case HpackField::INDEXED_LITERAL: - indexing_table.add_header_field(header.field_get()); + indexing_table.add_header_field(header); flag = 0x40; break; case HpackField::NOINDEX_LITERAL: @@ -644,12 +621,11 @@ encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf // Convert field name to lower case to follow HTTP2 spec. // This conversion is needed because WKSs in MIMEFields is old fashioned - int name_len; - const char *original_name = header.name_get(&name_len); + int name_len = header.name.size(); + const char *original_name = header.name.data(); ts::LocalBuffer<char> local_buffer(name_len); char *lower_name = local_buffer.data(); - for (int i = 0; i < name_len; i++) { lower_name[i] = ParseRules::ink_tolower(original_name[i]); } @@ -662,16 +638,16 @@ encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf p += len; // Value String - int value_len; - const char *value = header.value_get(&value_len); - len = xpack_encode_string(p, buf_end, value, value_len); + len = xpack_encode_string(p, buf_end, header.value.data(), header.value.size()); if (len == -1) { return -1; } p += len; - Debug("hpack_encode", "Encoded field: %.*s: %.*s", name_len, lower_name, value_len, value); + Debug("hpack_encode", "Encoded field: %.*s: %.*s", name_len, lower_name, static_cast<int>(header.value.size()), + header.value.data()); + return p - buf_start; } @@ -794,7 +770,8 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start, // Incremental Indexing adds header to header table as new entry if (isIncremental) { - indexing_table.add_header_field(header.field_get()); + const MIMEField *field = header.field_get(); + indexing_table.add_header_field({field->name_get(), field->value_get()}); } // Print decoded header field @@ -930,14 +907,13 @@ hpack_encode_header_block(HpackIndexingTable &indexing_table, uint8_t *out_buf, { uint8_t *cursor = out_buf; const uint8_t *const out_buf_end = out_buf + out_buf_len; - int64_t written = 0; ink_assert(http_hdr_type_get(hdr->m_http) != HTTP_TYPE_UNKNOWN); // Update dynamic table size if (maximum_table_size >= 0) { indexing_table.update_maximum_size(maximum_table_size); - written = encode_dynamic_table_size_update(cursor, out_buf_end, maximum_table_size); + int64_t written = encode_dynamic_table_size_update(cursor, out_buf_end, maximum_table_size); if (written == HPACK_ERROR_COMPRESSION_ERROR) { return HPACK_ERROR_COMPRESSION_ERROR; } @@ -946,22 +922,25 @@ hpack_encode_header_block(HpackIndexingTable &indexing_table, uint8_t *out_buf, MIMEFieldIter field_iter; for (MIMEField *field = hdr->iter_get_first(&field_iter); field != nullptr; field = hdr->iter_get_next(&field_iter)) { - HpackField field_type; - MIMEFieldWrapper header(field, hdr->m_heap, hdr->m_http->m_fields_impl); - int name_len; - int value_len; - const char *name = header.name_get(&name_len); - header.value_get(&value_len); + std::string_view name = field->name_get(); + std::string_view value = field->value_get(); + // Choose field representation (See RFC7541 7.1.3) // - Authorization header obviously should not be indexed // - Short Cookie header should not be indexed because of low entropy - if ((ptr_len_casecmp(name, name_len, MIME_FIELD_COOKIE, MIME_LEN_COOKIE) == 0 && value_len < 20) || - (ptr_len_casecmp(name, name_len, MIME_FIELD_AUTHORIZATION, MIME_LEN_AUTHORIZATION) == 0)) { + HpackField field_type; + // TODO: replace `strcasecmp` with `memcmp` + if ((value.size() < 20 && strcasecmp(name, HPACK_HDR_FIELD_COOKIE) == 0) || + (strcasecmp(name, HPACK_HDR_FIELD_AUTHORIZATION) == 0)) { field_type = HpackField::NEVERINDEX_LITERAL; } else { field_type = HpackField::INDEXED_LITERAL; } + + HpackHeaderField header{name, value}; const HpackLookupResult result = indexing_table.lookup(header); + + int64_t written = 0; switch (result.match_type) { case HpackMatch::NONE: written = encode_literal_header_field_with_new_name(cursor, out_buf_end, header, indexing_table, field_type); @@ -974,10 +953,9 @@ hpack_encode_header_block(HpackIndexingTable &indexing_table, uint8_t *out_buf, written = encode_indexed_header_field(cursor, out_buf_end, result.index); break; default: - // Does it happen? - written = 0; break; } + if (written == HPACK_ERROR_COMPRESSION_ERROR) { return HPACK_ERROR_COMPRESSION_ERROR; } diff --git a/proxy/http2/HPACK.h b/proxy/http2/HPACK.h index ffe67eb..8c538ef 100644 --- a/proxy/http2/HPACK.h +++ b/proxy/http2/HPACK.h @@ -29,6 +29,7 @@ #include "../hdrs/XPACK.h" #include <deque> +#include <string_view> // It means that any header field can be compressed/decompressed by ATS const static int HPACK_ERROR_COMPRESSION_ERROR = -1; @@ -61,6 +62,11 @@ struct HpackLookupResult { HpackMatch match_type = HpackMatch::NONE; }; +struct HpackHeaderField { + std::string_view name; + std::string_view value; +}; + class MIMEFieldWrapper { public: @@ -113,9 +119,9 @@ public: HpackDynamicTable &operator=(const HpackDynamicTable &) = delete; const MIMEField *get_header_field(uint32_t index) const; - void add_header_field(const MIMEField *field); + void add_header_field(const HpackHeaderField &header); - HpackLookupResult lookup(const char *name, int name_len, const char *value, int value_len) const; + HpackLookupResult lookup(const HpackHeaderField &header) const; uint32_t maximum_size() const; uint32_t size() const; void update_maximum_size(uint32_t new_size); @@ -145,11 +151,10 @@ public: HpackIndexingTable(HpackIndexingTable &) = delete; HpackIndexingTable &operator=(const HpackIndexingTable &) = delete; - HpackLookupResult lookup(const MIMEFieldWrapper &field) const; - HpackLookupResult lookup(const char *name, int name_len, const char *value, int value_len) const; + HpackLookupResult lookup(const HpackHeaderField &header) const; int get_header_field(uint32_t index, MIMEFieldWrapper &header_field) const; - void add_header_field(const MIMEField *field); + void add_header_field(const HpackHeaderField &header); uint32_t maximum_size() const; uint32_t size() const; void update_maximum_size(uint32_t new_size); @@ -160,10 +165,11 @@ private: // Low level interfaces int64_t encode_indexed_header_field(uint8_t *buf_start, const uint8_t *buf_end, uint32_t index); -int64_t encode_literal_header_field_with_indexed_name(uint8_t *buf_start, const uint8_t *buf_end, const MIMEFieldWrapper &header, +int64_t encode_literal_header_field_with_indexed_name(uint8_t *buf_start, const uint8_t *buf_end, const HpackHeaderField &header, uint32_t index, HpackIndexingTable &indexing_table, HpackField type); -int64_t encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf_end, const MIMEFieldWrapper &header, +int64_t encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf_end, const HpackHeaderField &header, HpackIndexingTable &indexing_table, HpackField type); + int64_t decode_indexed_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start, const uint8_t *buf_end, HpackIndexingTable &indexing_table); int64_t decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start, const uint8_t *buf_end, diff --git a/proxy/http2/unit_tests/test_HpackIndexingTable.cc b/proxy/http2/unit_tests/test_HpackIndexingTable.cc index ff470c7..d750015 100644 --- a/proxy/http2/unit_tests/test_HpackIndexingTable.cc +++ b/proxy/http2/unit_tests/test_HpackIndexingTable.cc @@ -198,19 +198,13 @@ TEST_CASE("HPACK low level APIs", "[hpack]") for (unsigned int i = 9; i < sizeof(literal_test_case) / sizeof(literal_test_case[0]); i++) { memset(buf, 0, BUFSIZE_FOR_REGRESSION_TEST); - ats_scoped_obj<HTTPHdr> headers(new HTTPHdr); - headers->create(HTTP_TYPE_RESPONSE); - MIMEField *field = mime_field_create(headers->m_heap, headers->m_http->m_fields_impl); - MIMEFieldWrapper header(field, headers->m_heap, headers->m_http->m_fields_impl); + HpackHeaderField header{literal_test_case[i].raw_name, literal_test_case[i].raw_value}; - header.name_set(literal_test_case[i].raw_name, strlen(literal_test_case[i].raw_name)); - header.value_set(literal_test_case[i].raw_value, strlen(literal_test_case[i].raw_value)); if (literal_test_case[i].index > 0) { len = encode_literal_header_field_with_indexed_name(buf, buf + BUFSIZE_FOR_REGRESSION_TEST, header, literal_test_case[i].index, indexing_table, literal_test_case[i].type); } else { - header.name_set(literal_test_case[i].raw_name, strlen(literal_test_case[i].raw_name)); len = encode_literal_header_field_with_new_name(buf, buf + BUFSIZE_FOR_REGRESSION_TEST, header, indexing_table, literal_test_case[i].type); } @@ -383,15 +377,14 @@ TEST_CASE("HPACK high level APIs", "[hpack]") j++) { const char *expected_name = dynamic_table_response_test_case[i][j].name; const char *expected_value = dynamic_table_response_test_case[i][j].value; - int expected_name_len = strlen(expected_name); - int expected_value_len = strlen(expected_value); - if (expected_name_len == 0) { + if (strlen(expected_name) == 0) { break; } - HpackLookupResult lookupResult = - indexing_table.lookup(expected_name, expected_name_len, expected_value, expected_value_len); + HpackHeaderField expected_header{expected_name, expected_value}; + HpackLookupResult lookupResult = indexing_table.lookup(expected_header); + CHECK(lookupResult.match_type == HpackMatch::EXACT); CHECK(lookupResult.index_type == HpackIndex::DYNAMIC);
