From adc90e4df93bf4bd3a9ade2f2c4cb43c03bd4b2e Mon Sep 17 00:00:00 2001 From: bneradt Date: Wed, 29 Jul 2026 20:22:44 -0500 Subject: [PATCH] Reuse deleted MIME field slots Long-lived MIME headers allocate field blocks as fields are added and removed. HPACK exercises this path heavily, while the current attempt to bound the block chain scans and destroys empty blocks during deletion. This maintains a deleted-slot free list through m_next_dup and consumes it before allocating another field block. Appending within a partially filled tail preserves established field order. The free list is rebuilt for copied or unmarshaled headers, and same-name duplicate order is preserved. This adds high-water churn, insertion-order, duplicate, copy, and marshal coverage. Fixes: #8466 --- include/proxy/hdrs/MIME.h | 6 +- src/proxy/hdrs/MIME.cc | 119 ++++++++++++----- src/proxy/hdrs/unit_tests/test_Hdrs.cc | 169 +++++++++++++++++++++++++ 3 files changed, 259 insertions(+), 35 deletions(-) diff --git a/include/proxy/hdrs/MIME.h b/include/proxy/hdrs/MIME.h index 932217a5e5c..e64d12f22f3 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 * @@ -304,7 +307,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]; diff --git a/src/proxy/hdrs/MIME.cc b/src/proxy/hdrs/MIME.cc index 9430385d42c..e67d219d567 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,19 @@ 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; + + field = mh->m_free_slot == MIME_FIELD_FREE_SLOT_NONE ? nullptr : mime_hdr_field_get_slotnum(mh, mh->m_free_slot); + while (field != nullptr) { + ink_release_assert(field->m_readiness == MIME_FIELD_SLOT_READINESS_DELETED); + ink_release_assert(mime_hdr_field_slotnum(mh, field) >= 0); + ink_release_assert(++free_count <= deleted_count); + field = field->m_next_dup; + } + ink_release_assert(free_count == deleted_count); + } } #endif @@ -949,6 +965,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 +1020,8 @@ mime_hdr_destroy(HdrHeap *heap, MIMEHdrImpl *mh) // heap->deallocate_obj(mh); } +static void mime_hdr_rebuild_field_free_list(MIMEHdrImpl *mh); + void mime_hdr_copy_onto(MIMEHdrImpl *s_mh, HdrHeap *s_heap, MIMEHdrImpl *d_mh, HdrHeap *d_heap, bool inherit_strs) { @@ -1047,6 +1067,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); @@ -1338,6 +1359,28 @@ mime_field_init(MIMEField *field) field->m_wks_idx = -1; } +static void +mime_hdr_rebuild_field_free_list(MIMEHdrImpl *mh) +{ + MIMEField *free_field = nullptr; + 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_next_dup = free_field; + free_field = field; + mh->m_free_slot = slotnum + static_cast(index); + } + } + slotnum += MIME_FIELD_BLOCK_SLOTS; + } +} + MIMEField * mime_field_create(HdrHeap *heap, MIMEHdrImpl *mh) { @@ -1345,14 +1388,34 @@ 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; + } + + if (mh->m_free_slot == MIME_FIELD_FREE_SLOT_UNINITIALIZED) { + mime_hdr_rebuild_field_free_list(mh); } + if (mh->m_free_slot != MIME_FIELD_FREE_SLOT_NONE) { + field = mime_hdr_field_get_slotnum(mh, mh->m_free_slot); + ink_release_assert(field != nullptr); + ink_release_assert(field->m_readiness == MIME_FIELD_SLOT_READINESS_DELETED); + + mh->m_free_slot = field->m_next_dup ? mime_hdr_field_slotnum(mh, field->m_next_dup) : MIME_FIELD_FREE_SLOT_NONE; + 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; @@ -1563,32 +1626,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); @@ -1662,10 +1699,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_next_dup = mh->m_free_slot == MIME_FIELD_FREE_SLOT_NONE ? nullptr : mime_hdr_field_get_slotnum(mh, 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 @@ -3544,6 +3589,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 +3603,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 +3691,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 +3701,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..0b8a8aa5869 100644 --- a/src/proxy/hdrs/unit_tests/test_Hdrs.cc +++ b/src/proxy/hdrs/unit_tests/test_Hdrs.cc @@ -657,6 +657,175 @@ 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("Reused duplicate fields remain ordered by slot") + { + 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") == reused); + CHECK(reused->m_next_dup == last); + CHECK(last->m_next_dup == nullptr); + CHECK(filler->is_live()); + } + + 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();