[trafficserver] 01/03: TS-5065: Fix RefCountCache iterator invalidation.

[jp...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jpeach pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git

commit 23672db4f1e6b3d7b633a82666955abe6ca86950
Author: James Peach <jp...@apache.org>
AuthorDate: Fri Nov 25 23:09:47 2016 -0800

    TS-5065: Fix RefCountCache iterator invalidation.
    
    Removing items from the TSHashMap invalidates the iterator because the
    linked list pointers are embedded in the hash node, so we can't do that
    while clearing. Instead, deallocate and remove each entry explicitly.
---
 iocore/hostdb/P_RefCountCache.h | 60 +++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/iocore/hostdb/P_RefCountCache.h b/iocore/hostdb/P_RefCountCache.h
index c417a5c..15ed326 100644
--- a/iocore/hostdb/P_RefCountCache.h
+++ b/iocore/hostdb/P_RefCountCache.h
@@ -79,21 +79,23 @@ public:
   PriorityQueueEntry<RefCountCacheHashEntry *> *expiry_entry;
   RefCountCacheItemMeta meta;
 
+  // Need a no-argument constructor to use the classAllocator
+  RefCountCacheHashEntry() : item(Ptr<RefCountObj>()), expiry_entry(nullptr), meta(0, 0) {}
   void
   set(RefCountObj *i, uint64_t key, unsigned int size, int expire_time)
   {
     this->item = make_ptr(i);
     this->meta = RefCountCacheItemMeta(key, size, expire_time);
-  };
-  // Need a no-argument constructor to use the classAllocator
-  RefCountCacheHashEntry() : item(Ptr<RefCountObj>()), expiry_entry(nullptr), meta(0, 0) {}
+  }
+
   // make these values comparable -- so we can sort them
   bool
   operator<(const RefCountCacheHashEntry &v2) const
   {
     return this->meta.expiry_time < v2.meta.expiry_time;
-  };
+  }
 };
+
 // Since the hashing values are all fixed size, we can simply use a classAllocator to avoid mallocs
 extern ClassAllocator<RefCountCacheHashEntry> refCountCacheHashingValueAllocator;
 extern ClassAllocator<PriorityQueueEntry<RefCountCacheHashEntry *>> expiryQueueEntry;
@@ -134,6 +136,7 @@ public:
   void clear();
   bool is_full() const;
   bool make_space_for(unsigned int);
+  template <class Iterator> void dealloc_entry(Iterator ptr);
 
   size_t count() const;
   void copy(Vec<RefCountCacheHashEntry *> &items);
@@ -228,31 +231,43 @@ RefCountCachePartition<C>::erase(uint64_t key, ink_time_t expiry_time)
     if (expiry_time >= 0 && l.m_value->meta.expiry_time != expiry_time) {
       return;
     }
+
     // TSHashMap does NOT clean up the item-- this remove just removes it from the map
     // we are responsible for cleaning it up here
     this->item_map.remove(l);
+    this->dealloc_entry(l);
+  }
+}
 
-    // decrement usage counters
-    this->size -= l.m_value->meta.size;
+template <class C>
+template <class Iterator>
+void
+RefCountCachePartition<C>::dealloc_entry(Iterator ptr)
+{
+  if (ptr.m_value) {
+    // decrement usag are not cleaned up. The values are not touched in this method, therefore it is safe
+    // counters
+    this->size -= ptr->meta.size;
     this->items--;
 
-    this->metric_inc(refcountcache_current_size_stat, -((int64_t)l.m_value->meta.size));
+    this->metric_inc(refcountcache_current_size_stat, -((int64_t)ptr->meta.size));
     this->metric_inc(refcountcache_current_items_stat, -1);
 
     // remove from expiry queue
-    if (l.m_value->expiry_entry != nullptr) {
-      Debug("refcountcache", "partition %d deleting item from expiry_queue idx=%d\n", this->part_num,
-            l.m_value->expiry_entry->index);
-      this->expiry_queue.erase(l.m_value->expiry_entry);
-      expiryQueueEntry.free(l.m_value->expiry_entry);
-      l.m_value->expiry_entry = nullptr; // To avoid the destruction of `l` calling the destructor again-- and causing issues
+    if (ptr->expiry_entry != nullptr) {
+      Debug("refcountcache", "partition %d deleting item from expiry_queue idx=%d", this->part_num, ptr->expiry_entry->index);
+
+      this->expiry_queue.erase(ptr->expiry_entry);
+      expiryQueueEntry.free(ptr->expiry_entry);
+      ptr->expiry_entry = nullptr; // To avoid the destruction of `l` calling the destructor again-- and causing issues
     }
+
     // Since the Value is actually RefCountObj-- when this gets deleted normally it calls the wrong
     // `free` method, this forces the delete/decr to happen with the right type
-    Ptr<C> *tmp = (Ptr<C> *)&l.m_value->item;
+    Ptr<C> *tmp = (Ptr<C> *)&ptr->item;
     tmp->clear();
-    l.m_value->~RefCountCacheHashEntry();
-    refCountCacheHashingValueAllocator.free(l.m_value);
+    ptr->~RefCountCacheHashEntry();
+    refCountCacheHashingValueAllocator.free(ptr.m_value);
   }
 }
 
@@ -260,10 +275,15 @@ template <class C>
 void
 RefCountCachePartition<C>::clear()
 {
-  // this->item_map.clear() doesn't clean up anything, so instead of using that we'll iterate
-  // over every item and then call delete
-  for (RefCountCachePartition<C>::iterator_type i = this->item_map.begin(); i != this->item_map.end(); ++i) {
-    this->erase(i.m_value->meta.key, i.m_value->meta.expiry_time);
+  // Since the hash nodes embed the list pointers, you can't iterate over the
+  // hash elements and deallocate them, let alone remove them from the hash.
+  // Hence, this monstrosity.
+  while (this->item_map.count() > 0) {
+    location_type pos = this->item_map.find(this->item_map.begin().m_value);
+
+    ink_assert(pos.isValid());
+    this->item_map.remove(pos);
+    this->dealloc_entry(pos);
   }
 }
 

-- 
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>.