diff --git a/include/proxy/hdrs/MIME.h b/include/proxy/hdrs/MIME.h index 932217a5e5c..82bf78e9775 100644 --- a/include/proxy/hdrs/MIME.h +++ b/include/proxy/hdrs/MIME.h @@ -90,6 +90,9 @@ enum class MimeParseState { #define MIME_FIELD_SLOTNUM_MAX (MIME_FIELD_SLOTNUM_MASK - 1) #define MIME_FIELD_SLOTNUM_UNKNOWN MIME_FIELD_SLOTNUM_MAX +#define MIME_FIELD_FREE_SLOT_NONE -1 +#define MIME_FIELD_FREE_SLOT_UNINITIALIZED -2 + /*********************************************************************** * * * MIMEField & MIMEFieldBlockImpl * @@ -99,16 +102,19 @@ enum class MimeParseState { struct MIMEHdrImpl; struct MIMEField { - const char *m_ptr_name; // 4 - const char *m_ptr_value; // 4 - MIMEField *m_next_dup; // 4 - int16_t m_wks_idx; // 2 - uint16_t m_len_name; // 2 - uint32_t m_len_value : 24; // 3 - uint8_t m_n_v_raw_printable : 1; // 1/8 - uint8_t m_n_v_raw_printable_pad : 3; // 3/8 - uint8_t m_readiness : 2; // 2/8 - uint8_t m_flags : 2; // 2/8 + const char *m_ptr_name; // 4 + const char *m_ptr_value; // 4 + union { + MIMEField *m_next_dup; // 4 + int32_t m_free_next; ///< Slot number of the next deleted field. + }; + int16_t m_wks_idx; // 2 + uint16_t m_len_name; // 2 + uint32_t m_len_value : 24; // 3 + uint8_t m_n_v_raw_printable : 1; // 1/8 + uint8_t m_n_v_raw_printable_pad : 3; // 3/8 + uint8_t m_readiness : 2; // 2/8 + uint8_t m_flags : 2; // 2/8 bool is_dup_head() const @@ -304,7 +310,8 @@ struct MIMEHdrImpl : public HdrHeapObjImpl { friend struct MIMEHdrImpl; }; - // HdrHeapObjImpl is 4 bytes, so this will result in 4 bytes padding + // HdrHeapObjImpl is 4 bytes, so this uses the 4 bytes that would otherwise be padding. + int32_t m_free_slot; ///< Slot number at the head of the deleted field free list. uint64_t m_presence_bits; uint32_t m_slot_accelerators[4]; @@ -749,6 +756,7 @@ int mime_hdr_fields_count(MIMEHdrImpl *mh); void mime_field_init(MIMEField *field); MIMEField *mime_field_create(HdrHeap *heap, MIMEHdrImpl *mh); +MIMEField *mime_field_create_for_name(HdrHeap *heap, MIMEHdrImpl *mh, std::string_view name); MIMEField *mime_field_create_named(HdrHeap *heap, MIMEHdrImpl *mh, std::string_view name); void mime_hdr_field_attach(MIMEHdrImpl *mh, MIMEField *field, int check_for_dups, MIMEField *prev_dup); @@ -1184,7 +1192,7 @@ MIMEHdr::fields_count() const inline MIMEField * MIMEHdr::field_create(std::string_view name) { - MIMEField *field = mime_field_create(m_heap, m_mime); + MIMEField *field = name.empty() ? mime_field_create(m_heap, m_mime) : mime_field_create_for_name(m_heap, m_mime, name); if (!name.empty()) { auto length{static_cast(name.length())}; diff --git a/include/proxy/http2/HPACK.h b/include/proxy/http2/HPACK.h index f9fcc0a9dc6..446fcbb8aac 100644 --- a/include/proxy/http2/HPACK.h +++ b/include/proxy/http2/HPACK.h @@ -71,10 +71,16 @@ class MIMEFieldWrapper { public: MIMEFieldWrapper(MIMEField *f, HdrHeap *hh, MIMEHdrImpl *impl) : _field(f), _heap(hh), _mh(impl) {} + MIMEFieldWrapper(HdrHeap *hh, MIMEHdrImpl *impl) : _heap(hh), _mh(impl) {} bool name_set(const char *name, int name_len) { - return _field->name_set(_heap, _mh, std::string_view{name, static_cast(name_len)}); + std::string_view name_view{name, static_cast(name_len)}; + + if (_field == nullptr) { + _field = mime_field_create_for_name(_heap, _mh, name_view); + } + return _field->name_set(_heap, _mh, name_view); } bool @@ -101,8 +107,14 @@ class MIMEFieldWrapper return _field; } + MIMEField * + field_get() + { + return _field; + } + private: - MIMEField *_field; + MIMEField *_field = nullptr; HdrHeap *_heap; MIMEHdrImpl *_mh; }; diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 6d085befbb4..a57f1aa31cb 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -1770,7 +1770,7 @@ TSMimeHdrFieldAppend(TSMBuffer bufp, TSMLoc mh_mloc, TSMLoc field_mloc) HdrHeap *heap = ((reinterpret_cast(bufp))->m_heap); // allocate a new hdr field and copy any pre-set info - mh_field = mime_field_create(heap, mh); + mh_field = mime_field_create_for_name(heap, mh, field_handle->field_ptr->name_get()); // FIX: is it safe to copy everything over? memcpy(mh_field, field_handle->field_ptr, sizeof(MIMEField)); diff --git a/src/proxy/hdrs/MIME.cc b/src/proxy/hdrs/MIME.cc index 9430385d42c..c92b029faff 100644 --- a/src/proxy/hdrs/MIME.cc +++ b/src/proxy/hdrs/MIME.cc @@ -542,6 +542,7 @@ mime_hdr_sanity_check(MIMEHdrImpl *mh) MIMEField *field, *next_dup; uint32_t slot_index, index; uint64_t masksum; + size_t deleted_count = 0; ink_assert(mh != nullptr); @@ -617,13 +618,15 @@ mime_hdr_sanity_check(MIMEHdrImpl *mh) ink_release_assert(found); } // re-find the field --- should always find the head dup - MIMEField *mf = mime_hdr_field_find(mh, field->m_ptr_name, field->m_len_name); + MIMEField *mf = mime_hdr_field_find(mh, {field->m_ptr_name, field->m_len_name}); ink_release_assert(mf != nullptr); if (mf == field) { ink_release_assert((field->m_flags & MIME_FIELD_SLOT_FLAGS_DUP_HEAD) != 0); } else { ink_release_assert((field->m_flags & MIME_FIELD_SLOT_FLAGS_DUP_HEAD) == 0); } + } else if (field->m_readiness == MIME_FIELD_SLOT_READINESS_DELETED) { + ++deleted_count; } ++slot_index; @@ -633,6 +636,20 @@ mime_hdr_sanity_check(MIMEHdrImpl *mh) ink_release_assert(last_fblock == mh->m_fblock_list_tail); ink_release_assert(masksum == mh->m_presence_bits); + + if (mh->m_free_slot != MIME_FIELD_FREE_SLOT_UNINITIALIZED) { + size_t free_count = 0; + int32_t free_slot = mh->m_free_slot; + + while (free_slot != MIME_FIELD_FREE_SLOT_NONE) { + field = mime_hdr_field_get_slotnum(mh, free_slot); + ink_release_assert(field != nullptr); + ink_release_assert(field->m_readiness == MIME_FIELD_SLOT_READINESS_DELETED); + ink_release_assert(++free_count <= deleted_count); + free_slot = field->m_free_next; + } + ink_release_assert(free_count == deleted_count); + } } #endif @@ -949,6 +966,8 @@ mime_hdr_cooked_stuff_init(MIMEHdrImpl *mh, MIMEField *changing_field_or_null) void mime_hdr_init(MIMEHdrImpl *mh) { + mh->m_free_slot = MIME_FIELD_FREE_SLOT_NONE; + mime_hdr_init_accelerators_and_presence_bits(mh); mime_hdr_cooked_stuff_init(mh, nullptr); @@ -1002,6 +1021,26 @@ mime_hdr_destroy(HdrHeap *heap, MIMEHdrImpl *mh) // heap->deallocate_obj(mh); } +static void +mime_hdr_rebuild_field_free_list(MIMEHdrImpl *mh) +{ + int32_t slotnum = 0; + + mh->m_free_slot = MIME_FIELD_FREE_SLOT_NONE; + for (MIMEFieldBlockImpl *fblock = &mh->m_first_fblock; fblock != nullptr; fblock = fblock->m_next) { + for (uint32_t index = 0; index < fblock->m_freetop; ++index) { + MIMEField *field = &fblock->m_field_slots[index]; + + if (field->m_readiness == MIME_FIELD_SLOT_READINESS_DELETED || field->m_readiness == MIME_FIELD_SLOT_READINESS_EMPTY) { + field->m_readiness = MIME_FIELD_SLOT_READINESS_DELETED; + field->m_free_next = mh->m_free_slot; + mh->m_free_slot = slotnum + static_cast(index); + } + } + slotnum += MIME_FIELD_BLOCK_SLOTS; + } +} + void mime_hdr_copy_onto(MIMEHdrImpl *s_mh, HdrHeap *s_heap, MIMEHdrImpl *d_mh, HdrHeap *d_heap, bool inherit_strs) { @@ -1047,6 +1086,7 @@ mime_hdr_copy_onto(MIMEHdrImpl *s_mh, HdrHeap *s_heap, MIMEHdrImpl *d_mh, HdrHea } mime_hdr_field_block_list_adjust(block_count, &(s_mh->m_first_fblock), &(d_mh->m_first_fblock)); + mime_hdr_rebuild_field_free_list(d_mh); MIME_HDR_SANITY_CHECK(s_mh); MIME_HDR_SANITY_CHECK(d_mh); @@ -1345,14 +1385,20 @@ mime_field_create(HdrHeap *heap, MIMEHdrImpl *mh) MIMEFieldBlockImpl *tail_fblock, *new_fblock; tail_fblock = mh->m_fblock_list_tail; - if (tail_fblock->m_freetop >= MIME_FIELD_BLOCK_SLOTS) { - new_fblock = (MIMEFieldBlockImpl *)heap->allocate_obj(sizeof(MIMEFieldBlockImpl), HdrHeapObjType::FIELD_BLOCK); - _mime_hdr_field_block_init(new_fblock); - tail_fblock->m_next = new_fblock; - tail_fblock = new_fblock; - mh->m_fblock_list_tail = new_fblock; + if (tail_fblock->m_freetop < MIME_FIELD_BLOCK_SLOTS) { + field = &(tail_fblock->m_field_slots[tail_fblock->m_freetop]); + ++tail_fblock->m_freetop; + + mime_field_init(field); + return field; } + new_fblock = (MIMEFieldBlockImpl *)heap->allocate_obj(sizeof(MIMEFieldBlockImpl), HdrHeapObjType::FIELD_BLOCK); + _mime_hdr_field_block_init(new_fblock); + tail_fblock->m_next = new_fblock; + tail_fblock = new_fblock; + mh->m_fblock_list_tail = new_fblock; + field = &(tail_fblock->m_field_slots[tail_fblock->m_freetop]); ++tail_fblock->m_freetop; @@ -1361,10 +1407,59 @@ mime_field_create(HdrHeap *heap, MIMEHdrImpl *mh) return field; } +MIMEField * +mime_field_create_for_name(HdrHeap *heap, MIMEHdrImpl *mh, std::string_view name) +{ + if (mh->m_fblock_list_tail->m_freetop < MIME_FIELD_BLOCK_SLOTS) { + return mime_field_create(heap, mh); + } + + int last_dup_slot = -1; + + if (MIMEField *last_dup = name.empty() ? nullptr : mime_hdr_field_find(mh, name); last_dup != nullptr) { + while (last_dup->m_next_dup != nullptr) { + last_dup = last_dup->m_next_dup; + } + last_dup_slot = mime_hdr_field_slotnum(mh, last_dup); + ink_release_assert(last_dup_slot >= 0); + } + + if (mh->m_free_slot == MIME_FIELD_FREE_SLOT_UNINITIALIZED) { + mime_hdr_rebuild_field_free_list(mh); + } + + int32_t previous_free_slot = MIME_FIELD_FREE_SLOT_NONE; + int32_t free_slot = mh->m_free_slot; + + while (free_slot != MIME_FIELD_FREE_SLOT_NONE) { + MIMEField *field = mime_hdr_field_get_slotnum(mh, free_slot); + ink_release_assert(field != nullptr); + ink_release_assert(field->m_readiness == MIME_FIELD_SLOT_READINESS_DELETED); + + if (free_slot > last_dup_slot) { + if (previous_free_slot == MIME_FIELD_FREE_SLOT_NONE) { + mh->m_free_slot = field->m_free_next; + } else { + MIMEField *previous_free = mime_hdr_field_get_slotnum(mh, previous_free_slot); + + ink_release_assert(previous_free != nullptr); + previous_free->m_free_next = field->m_free_next; + } + mime_field_init(field); + return field; + } + + previous_free_slot = free_slot; + free_slot = field->m_free_next; + } + + return mime_field_create(heap, mh); +} + MIMEField * mime_field_create_named(HdrHeap *heap, MIMEHdrImpl *mh, std::string_view name) { - MIMEField *field = mime_field_create(heap, mh); + MIMEField *field = mime_field_create_for_name(heap, mh, name); int field_name_wks_idx = hdrtoken_tokenize(name.data(), static_cast(name.length())); if (!mime_field_name_set(heap, mh, field, field_name_wks_idx, name, true)) { // The name exceeds the uint16_t field-length limit and was rejected. Tear @@ -1563,32 +1658,6 @@ mime_hdr_field_delete(HdrHeap *heap, MIMEHdrImpl *mh, MIMEField *field, bool del MIME_HDR_SANITY_CHECK(mh); mime_field_destroy(mh, field); - - MIMEFieldBlockImpl *prev_block = nullptr; - bool can_destroy_block = true; - for (auto fblock = &(mh->m_first_fblock); fblock != nullptr; fblock = fblock->m_next) { - if (prev_block != nullptr) { - if (fblock->m_freetop == MIME_FIELD_BLOCK_SLOTS && fblock->contains(field)) { - // Check if fields in all slots are deleted - for (auto &m_field_slot : fblock->m_field_slots) { - if (m_field_slot.m_readiness != MIME_FIELD_SLOT_READINESS_DELETED) { - can_destroy_block = false; - break; - } - } - // Destroy a block and maintain the chain - if (can_destroy_block) { - prev_block->m_next = fblock->m_next; - _mime_field_block_destroy(heap, fblock); - if (prev_block->m_next == nullptr) { - mh->m_fblock_list_tail = prev_block; - } - } - break; - } - } - prev_block = fblock; - } } MIME_HDR_SANITY_CHECK(mh); @@ -1646,7 +1715,7 @@ mime_hdr_prepare_for_value_set(HdrHeap *heap, MIMEHdrImpl *mh, std::string_view if (field == nullptr) // no fields of this name { wks_idx = hdrtoken_tokenize(name.data(), static_cast(name.length())); - field = mime_field_create(heap, mh); + field = mime_field_create_for_name(heap, mh, name); mime_field_name_set(heap, mh, field, wks_idx, name, true); mime_hdr_field_attach(mh, field, 0, nullptr); @@ -1654,7 +1723,7 @@ mime_hdr_prepare_for_value_set(HdrHeap *heap, MIMEHdrImpl *mh, std::string_view { wks_idx = field->m_wks_idx; mime_hdr_field_delete(heap, mh, field, true); - field = mime_field_create(heap, mh); + field = mime_field_create_for_name(heap, mh, name); mime_field_name_set(heap, mh, field, wks_idx, name, true); mime_hdr_field_attach(mh, field, 0, nullptr); } @@ -1662,10 +1731,18 @@ mime_hdr_prepare_for_value_set(HdrHeap *heap, MIMEHdrImpl *mh, std::string_view } void -mime_field_destroy(MIMEHdrImpl * /* mh ATS_UNUSED */, MIMEField *field) +mime_field_destroy(MIMEHdrImpl *mh, MIMEField *field) { ink_assert(field->m_readiness == MIME_FIELD_SLOT_READINESS_DETACHED); field->m_readiness = MIME_FIELD_SLOT_READINESS_DELETED; + + if (mh->m_free_slot == MIME_FIELD_FREE_SLOT_UNINITIALIZED) { + mime_hdr_rebuild_field_free_list(mh); + } else { + field->m_free_next = mh->m_free_slot; + mh->m_free_slot = mime_hdr_field_slotnum(mh, field); + ink_release_assert(mh->m_free_slot >= 0); + } } std::string_view @@ -2568,7 +2645,7 @@ mime_parser_parse(MIMEParser *parser, HdrHeap *heap, MIMEHdrImpl *mh, const char // build and insert the new field object // /////////////////////////////////////////// - MIMEField *field = mime_field_create(heap, mh); + MIMEField *field = mime_field_create_for_name(heap, mh, field_name); mime_field_name_value_set(heap, mh, field, field_name_wks_idx, field_name, field_value, raw_print_field, parsed.size(), false); mime_hdr_field_attach(mh, field, 1, nullptr); } @@ -3544,6 +3621,8 @@ MIMEFieldBlockImpl::marshal(MarshalXlate *ptr_xlate, int num_ptr, MarshalXlate * if (field->m_next_dup) { HDR_MARSHAL_PTR_1(field->m_next_dup, MIMEField, ptr_xlate); } + } else { + field->m_next_dup = nullptr; } } } else { @@ -3556,6 +3635,8 @@ MIMEFieldBlockImpl::marshal(MarshalXlate *ptr_xlate, int num_ptr, MarshalXlate * if (field->m_next_dup) { HDR_MARSHAL_PTR(field->m_next_dup, MIMEField, ptr_xlate, num_ptr); } + } else { + field->m_next_dup = nullptr; } } } @@ -3642,6 +3723,7 @@ int MIMEHdrImpl::marshal(MarshalXlate *ptr_xlate, int num_ptr, MarshalXlate *str_xlate, int num_str) { // printf("MIMEHdrImpl:marshal num_ptr = %d num_str = %d\n", num_ptr, num_str); + m_free_slot = MIME_FIELD_FREE_SLOT_UNINITIALIZED; HDR_MARSHAL_PTR(m_fblock_list_tail, MIMEFieldBlockImpl, ptr_xlate, num_ptr); return m_first_fblock.marshal(ptr_xlate, num_ptr, str_xlate, num_str); } @@ -3651,6 +3733,7 @@ MIMEHdrImpl::unmarshal(intptr_t offset) { HDR_UNMARSHAL_PTR(m_fblock_list_tail, MIMEFieldBlockImpl, offset); m_first_fblock.unmarshal(offset); + m_free_slot = MIME_FIELD_FREE_SLOT_UNINITIALIZED; } void diff --git a/src/proxy/hdrs/unit_tests/test_Hdrs.cc b/src/proxy/hdrs/unit_tests/test_Hdrs.cc index 71d52af9a23..5d94811331d 100644 --- a/src/proxy/hdrs/unit_tests/test_Hdrs.cc +++ b/src/proxy/hdrs/unit_tests/test_Hdrs.cc @@ -657,6 +657,196 @@ test_arena_aux(Arena &arena, int len) } // end anonymous namespace +TEST_CASE("MIME fields reuse deleted slots", "[proxy][hdrtest][mime]") +{ + mime_init(); + http_init(); + + auto add_field = [](HTTPHdr &hdr, std::string_view name) { + MIMEField *field = hdr.field_create(name); + + hdr.field_attach(field); + return field; + }; + + SECTION("A fully deleted field block is retained and reused") + { + HTTPHdr hdr; + hdr.create(HTTPType::RESPONSE); + ts::PostScript cleanup([&]() -> void { hdr.destroy(); }); + + std::array fields; + for (unsigned index = 0; index < fields.size(); ++index) { + fields[index] = add_field(hdr, "X-Field-" + std::to_string(index)); + } + + MIMEFieldBlockImpl *second_block = hdr.m_mime->m_first_fblock.m_next; + REQUIRE(second_block != nullptr); + REQUIRE(second_block == hdr.m_mime->m_fblock_list_tail); + + for (unsigned index = MIME_FIELD_BLOCK_SLOTS; index < fields.size(); ++index) { + hdr.field_delete(fields[index], false); + } + + CHECK(hdr.m_mime->m_first_fblock.m_next == second_block); + CHECK(hdr.m_mime->m_fblock_list_tail == second_block); + + MIMEField *reused = hdr.field_create("X-Reused"); + CHECK(reused == fields.back()); + CHECK(hdr.m_mime->m_fblock_list_tail == second_block); + } + + SECTION("Duplicate fields skip earlier free slots") + { + HTTPHdr hdr; + hdr.create(HTTPType::RESPONSE); + ts::PostScript cleanup([&]() -> void { hdr.destroy(); }); + + MIMEField *first = add_field(hdr, "X-Duplicate"); + MIMEField *filler = add_field(hdr, "X-Filler"); + MIMEField *last = add_field(hdr, "X-Duplicate"); + for (unsigned index = 3; index < MIME_FIELD_BLOCK_SLOTS; ++index) { + add_field(hdr, "X-Filler-" + std::to_string(index)); + } + + REQUIRE(first->m_next_dup == last); + hdr.field_delete(first, false); + REQUIRE(hdr.field_find("X-Duplicate") == last); + + MIMEField *reused = add_field(hdr, "X-Duplicate"); + CHECK(reused != first); + CHECK(hdr.field_find("X-Duplicate") == last); + CHECK(last->m_next_dup == reused); + CHECK(reused->m_next_dup == nullptr); + CHECK(mime_hdr_field_slotnum(hdr.m_mime, last) < mime_hdr_field_slotnum(hdr.m_mime, reused)); + CHECK(filler->is_live()); + } + + SECTION("A reused earlier slot preserves same-name field order") + { + HTTPHdr hdr; + hdr.create(HTTPType::RESPONSE); + ts::PostScript cleanup([&]() -> void { hdr.destroy(); }); + + MIMEField *victim = add_field(hdr, "X-Victim"); + for (unsigned index = 1; index < MIME_FIELD_BLOCK_SLOTS - 1; ++index) { + add_field(hdr, "X-Filler-" + std::to_string(index)); + } + MIMEField *first = add_field(hdr, "Set-Cookie"); + REQUIRE(hdr.m_mime->m_first_fblock.m_freetop == MIME_FIELD_BLOCK_SLOTS); + + hdr.field_delete(victim, false); + MIMEField *second = add_field(hdr, "Set-Cookie"); + + CHECK(hdr.field_find("Set-Cookie") == first); + CHECK(mime_hdr_field_slotnum(hdr.m_mime, first) < mime_hdr_field_slotnum(hdr.m_mime, second)); + } + + SECTION("Unused tail slots preserve field insertion order") + { + HTTPHdr hdr; + hdr.create(HTTPType::RESPONSE); + ts::PostScript cleanup([&]() -> void { hdr.destroy(); }); + + MIMEField *deleted = add_field(hdr, "X-Deleted"); + MIMEField *kept = add_field(hdr, "X-Kept"); + + hdr.field_delete(deleted, false); + MIMEField *appended = add_field(hdr, "X-Appended"); + + CHECK(appended != deleted); + CHECK(mime_hdr_field_slotnum(hdr.m_mime, kept) < mime_hdr_field_slotnum(hdr.m_mime, appended)); + } + + SECTION("Repeated field churn stays at the high-water block count") + { + HTTPHdr hdr; + hdr.create(HTTPType::RESPONSE); + ts::PostScript cleanup([&]() -> void { hdr.destroy(); }); + + MIMEField *field = nullptr; + for (unsigned index = 0; index < MIME_FIELD_BLOCK_SLOTS * 2; ++index) { + field = add_field(hdr, "X-Field-" + std::to_string(index)); + } + + MIMEField *slot = field; + MIMEFieldBlockImpl *second_block = hdr.m_mime->m_first_fblock.m_next; + bool reused_slot = true; + constexpr unsigned churn_cycles = 1024; + + for (unsigned index = 0; index < churn_cycles; ++index) { + hdr.field_delete(field, false); + field = add_field(hdr, "X-Churn"); + reused_slot &= field == slot; + } + + CHECK(reused_slot); + CHECK(hdr.m_mime->m_first_fblock.m_next == second_block); + CHECK(hdr.m_mime->m_fblock_list_tail == second_block); + CHECK(second_block->m_freetop == MIME_FIELD_BLOCK_SLOTS); + } + + SECTION("Copied headers rebuild the free list") + { + HTTPHdr source; + HTTPHdr copy; + source.create(HTTPType::RESPONSE); + copy.create(HTTPType::RESPONSE); + ts::PostScript cleanup([&]() -> void { + source.destroy(); + copy.destroy(); + }); + + std::array fields; + for (unsigned index = 0; index < fields.size(); ++index) { + fields[index] = add_field(source, "X-Field-" + std::to_string(index)); + } + source.field_delete(fields[4], false); + source.field_delete(fields[MIME_FIELD_BLOCK_SLOTS + 4], false); + + copy.copy(&source); + MIMEFieldBlockImpl *tail = copy.m_mime->m_fblock_list_tail; + MIMEField *expected_slot = mime_hdr_field_get_slotnum(copy.m_mime, MIME_FIELD_BLOCK_SLOTS + 4); + + CHECK(copy.field_create("X-Reused") == expected_slot); + CHECK(copy.m_mime->m_fblock_list_tail == tail); + } + + SECTION("Headers copied from an unmarshaled heap rebuild the free list") + { + HTTPHdr source; + source.create(HTTPType::RESPONSE); + ts::PostScript cleanup([&]() -> void { source.destroy(); }); + + std::array fields; + for (unsigned index = 0; index < fields.size(); ++index) { + fields[index] = add_field(source, "X-Field-" + std::to_string(index)); + } + source.field_delete(fields[MIME_FIELD_BLOCK_SLOTS + 4], false); + + std::vector marshal_buffer(source.m_heap->marshal_length()); + int marshal_length = source.m_heap->marshal(marshal_buffer.data(), marshal_buffer.size()); + REQUIRE(marshal_length > 0); + + TestRefCountObj ref; + ref.refcount_inc(); + + HTTPHdr unmarshaled; + REQUIRE(unmarshaled.unmarshal(marshal_buffer.data(), marshal_length, &ref) > 0); + + HTTPHdr writable; + writable.create(HTTPType::RESPONSE); + ts::PostScript writable_cleanup([&]() -> void { writable.destroy(); }); + writable.copy(&unmarshaled); + + MIMEFieldBlockImpl *tail = writable.m_mime->m_fblock_list_tail; + MIMEField *expected_slot = mime_hdr_field_get_slotnum(writable.m_mime, MIME_FIELD_BLOCK_SLOTS + 4); + + CHECK(writable.field_create("X-Reused") == expected_slot); + CHECK(writable.m_mime->m_fblock_list_tail == tail); + } +} + TEST_CASE("HdrTest", "[proxy][hdrtest]") { hdrtoken_init(); diff --git a/src/proxy/http2/HPACK.cc b/src/proxy/http2/HPACK.cc index 792fa1d3203..35f31a60c20 100644 --- a/src/proxy/http2/HPACK.cc +++ b/src/proxy/http2/HPACK.cc @@ -722,9 +722,9 @@ hpack_decode_header_block(HpackIndexingTable &indexing_table, HTTPHdr *hdr, cons while (cursor < in_buf_end) { int64_t read_bytes = 0; - // decode a header field encoded by HPACK - MIMEField *field = mime_field_create(heap, hh->m_fields_impl); - MIMEFieldWrapper header(field, heap, hh->m_fields_impl); + // Decode a header field encoded by HPACK. The wrapper allocates the field + // after its name is known so the selected slot preserves duplicate order. + MIMEFieldWrapper header(heap, hh->m_fields_impl); HpackField ftype = hpack_parse_field_type(*cursor); switch (ftype) { @@ -762,8 +762,8 @@ hpack_decode_header_block(HpackIndexingTable &indexing_table, HTTPHdr *hdr, cons continue; } - auto name{field->name_get()}; - auto value{field->value_get()}; + auto name{header.name_get()}; + auto value{header.value_get()}; // [RFC 7540] 6.5.2. SETTINGS_MAX_HEADER_LIST_SIZE: // The value is based on the uncompressed size of header fields, including the length of the name and value in octets plus an @@ -774,8 +774,7 @@ hpack_decode_header_block(HpackIndexingTable &indexing_table, HTTPHdr *hdr, cons return HPACK_ERROR_SIZE_EXCEEDED_ERROR; } - // Store to HdrHeap - mime_hdr_field_attach(hh->m_fields_impl, field, 1, nullptr); + mime_hdr_field_attach(hh->m_fields_impl, header.field_get(), 1, nullptr); } // Parsing all headers is done if (has_http2_violation) { diff --git a/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc b/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc index 0e7385113e8..8d2d09df067 100644 --- a/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc +++ b/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc @@ -104,8 +104,7 @@ TEST_CASE("HPACK low level APIs", "[hpack]") for (const auto &i : indexed_test_case) { std::unique_ptr headers(new HTTPHdr, destroy_http_hdr); headers->create(HTTPType::REQUEST); - 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); + MIMEFieldWrapper header(headers->m_heap, headers->m_http->m_fields_impl); int len = decode_indexed_header_field(header, i.encoded_field, i.encoded_field + i.encoded_field_len, indexing_table); REQUIRE(len == i.encoded_field_len); @@ -254,8 +253,7 @@ TEST_CASE("HPACK low level APIs", "[hpack]") for (const auto &i : literal_test_case) { std::unique_ptr headers(new HTTPHdr, destroy_http_hdr); headers->create(HTTPType::REQUEST); - 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); + MIMEFieldWrapper header(headers->m_heap, headers->m_http->m_fields_impl); int len = decode_literal_header_field(header, i.encoded_field, i.encoded_field + i.encoded_field_len, indexing_table, MAX_FIELD_SIZE);