[trafficserver] 02/03: TS-5066: Fix HostDB memory leaks on serialization failure.

[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 4a380d9d683d425cb7b2af30e1d57f0260ab829a
Author: James Peach <jp...@apache.org>
AuthorDate: Sat Nov 26 11:47:04 2016 -0800

    TS-5066: Fix HostDB memory leaks on serialization failure.
    
    If the serializer fails to write the partition, the copied entries were
    being leaked. Clean up the cache entry allocation to centralize the
    pain of it and allow both the cache and the serializer to share the same
    allocation and free helpers.
---
 iocore/hostdb/HostDB.cc                   |  4 ++--
 iocore/hostdb/P_RefCountCache.h           | 28 +++++++++++++++++++---------
 iocore/hostdb/P_RefCountCacheSerializer.h | 29 ++++++++++++++++++-----------
 iocore/hostdb/RefCountCache.cc            | 14 +++++++++++++-
 4 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/iocore/hostdb/HostDB.cc b/iocore/hostdb/HostDB.cc
index 54238e7..f31fa37 100644
--- a/iocore/hostdb/HostDB.cc
+++ b/iocore/hostdb/HostDB.cc
@@ -276,8 +276,8 @@ struct HostDBSync : public HostDBBackgroundTask {
     SET_HANDLER(&HostDBSync::wait_event);
     start_time = Thread::get_hrtime();
 
-    new RefCountCacheSerializer<RefCountCache<HostDBInfo>>(this, hostDBProcessor.cache()->refcountcache, this->frequency,
-                                                           this->storage_path, this->full_path);
+    new RefCountCacheSerializer<HostDBInfo>(this, hostDBProcessor.cache()->refcountcache, this->frequency, this->storage_path,
+                                            this->full_path);
     return EVENT_DONE;
   }
 };
diff --git a/iocore/hostdb/P_RefCountCache.h b/iocore/hostdb/P_RefCountCache.h
index 15ed326..4b7998e 100644
--- a/iocore/hostdb/P_RefCountCache.h
+++ b/iocore/hostdb/P_RefCountCache.h
@@ -94,10 +94,25 @@ public:
   {
     return this->meta.expiry_time < v2.meta.expiry_time;
   }
+
+  static RefCountCacheHashEntry *alloc();
+  static void dealloc(RefCountCacheHashEntry *e);
+
+  template <typename C>
+  static void
+  free(RefCountCacheHashEntry *e)
+  {
+    // 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> *)&e->item;
+    tmp->clear();
+
+    e->~RefCountCacheHashEntry();
+    dealloc(e);
+  }
 };
 
 // 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;
 
 struct RefCountCacheHashing {
@@ -202,7 +217,7 @@ RefCountCachePartition<C>::put(uint64_t key, C *item, int size, int expire_time)
   }
 
   // Create our value-- which has a ref to the `item`
-  RefCountCacheHashEntry *val = refCountCacheHashingValueAllocator.alloc();
+  RefCountCacheHashEntry *val = RefCountCacheHashEntry::alloc();
   val->set(item, key, size, expire_time);
 
   // add expiry_entry to expiry queue, if the expire time is positive (otherwise it means don't expire)
@@ -262,12 +277,7 @@ RefCountCachePartition<C>::dealloc_entry(Iterator ptr)
       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> *)&ptr->item;
-    tmp->clear();
-    ptr->~RefCountCacheHashEntry();
-    refCountCacheHashingValueAllocator.free(ptr.m_value);
+    RefCountCacheHashEntry::free<C>(ptr.m_value);
   }
 }
 
@@ -332,7 +342,7 @@ void
 RefCountCachePartition<C>::copy(Vec<RefCountCacheHashEntry *> &items)
 {
   for (RefCountCachePartition<C>::iterator_type i = this->item_map.begin(); i != this->item_map.end(); ++i) {
-    RefCountCacheHashEntry *val = refCountCacheHashingValueAllocator.alloc();
+    RefCountCacheHashEntry *val = RefCountCacheHashEntry::alloc();
     val->set(i.m_value->item.get(), i.m_value->meta.key, i.m_value->meta.size, i.m_value->meta.expiry_time);
     items.push_back(val);
   }
diff --git a/iocore/hostdb/P_RefCountCacheSerializer.h b/iocore/hostdb/P_RefCountCacheSerializer.h
index 63bc8ca..4004f37 100644
--- a/iocore/hostdb/P_RefCountCacheSerializer.h
+++ b/iocore/hostdb/P_RefCountCacheSerializer.h
@@ -39,8 +39,8 @@
 template <class C> class RefCountCacheSerializer : public Continuation
 {
 public:
-  size_t partition; // Current partition
-  C *cache;         // Pointer to the entire cache
+  size_t partition;        // Current partition
+  RefCountCache<C> *cache; // Pointer to the entire cache
   Continuation *cont;
 
   int copy_partition(int event, Event *e);
@@ -55,7 +55,7 @@ public:
   // helper method to spin on writes to disk
   int write_to_disk(const void *, size_t);
 
-  RefCountCacheSerializer(Continuation *acont, C *cc, int frequency, std::string dirname, std::string filename);
+  RefCountCacheSerializer(Continuation *acont, RefCountCache<C> *cc, int frequency, std::string dirname, std::string filename);
   ~RefCountCacheSerializer();
 
 private:
@@ -77,7 +77,7 @@ private:
 };
 
 template <class C>
-RefCountCacheSerializer<C>::RefCountCacheSerializer(Continuation *acont, C *cc, int frequency, std::string dirname,
+RefCountCacheSerializer<C>::RefCountCacheSerializer(Continuation *acont, RefCountCache<C> *cc, int frequency, std::string dirname,
                                                     std::string filename)
   : Continuation(nullptr),
     partition(0),
@@ -107,6 +107,11 @@ template <class C> RefCountCacheSerializer<C>::~RefCountCacheSerializer()
     socketManager.close(fd);
   }
 
+  forv_Vec (RefCountCacheHashEntry, entry, this->partition_items) {
+    RefCountCacheHashEntry::free<C>(entry);
+  }
+  this->partition_items.clear();
+
   Debug("refcountcache", "finished serializer %p", this);
 
   // Note that we have to do the unlink before we send the completion event, otherwise
@@ -153,15 +158,15 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
   // write to disk with headers per item
 
   for (unsigned int i = 0; i < this->partition_items.length(); i++) {
-    RefCountCacheHashEntry *it = this->partition_items[i];
+    RefCountCacheHashEntry *entry = this->partition_items[i];
 
     // check if the item has expired, if so don't persist it to disk
-    if (it->meta.expiry_time < curr_time) {
+    if (entry->meta.expiry_time < curr_time) {
       continue;
     }
 
     // Write the RefCountCacheItemMeta (as our header)
-    int ret = this->write_to_disk((char *)&it->meta, sizeof(it->meta));
+    int ret = this->write_to_disk((char *)&entry->meta, sizeof(entry->meta));
     if (ret < 0) {
       Warning("Error writing cache item header to %s: %d", this->tmp_filename.c_str(), ret);
       delete this;
@@ -169,7 +174,7 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
     }
 
     // write the actual object now
-    ret = this->write_to_disk((char *)it->item.get(), it->meta.size);
+    ret = this->write_to_disk((char *)entry->item.get(), entry->meta.size);
     if (ret < 0) {
       Warning("Error writing cache item to %s: %d", this->tmp_filename.c_str(), ret);
       delete this;
@@ -177,11 +182,13 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
     }
 
     this->total_items++;
-    this->total_size += it->meta.size;
-    refCountCacheHashingValueAllocator.free(it);
+    this->total_size += entry->meta.size;
   }
 
-  // Clear partition-- for the next user
+  // Clear the copied partition for the next round.
+  forv_Vec (RefCountCacheHashEntry, entry, this->partition_items) {
+    RefCountCacheHashEntry::free<C>(entry);
+  }
   this->partition_items.clear();
 
   SET_HANDLER(&RefCountCacheSerializer::pause_event);
diff --git a/iocore/hostdb/RefCountCache.cc b/iocore/hostdb/RefCountCache.cc
index 1d35ecb..26abc75 100644
--- a/iocore/hostdb/RefCountCache.cc
+++ b/iocore/hostdb/RefCountCache.cc
@@ -22,10 +22,22 @@
 #include <P_RefCountCache.h>
 
 // Since the hashing values are all fixed size, we can simply use a classAllocator to avoid mallocs
-ClassAllocator<RefCountCacheHashEntry> refCountCacheHashingValueAllocator("refCountCacheHashingValueAllocator");
+static ClassAllocator<RefCountCacheHashEntry> refCountCacheHashingValueAllocator("refCountCacheHashingValueAllocator");
 
 ClassAllocator<PriorityQueueEntry<RefCountCacheHashEntry *>> expiryQueueEntry("expiryQueueEntry");
 
+RefCountCacheHashEntry *
+RefCountCacheHashEntry::alloc()
+{
+  return refCountCacheHashingValueAllocator.alloc();
+}
+
+void
+RefCountCacheHashEntry::dealloc(RefCountCacheHashEntry *e)
+{
+  return refCountCacheHashingValueAllocator.free(e);
+}
+
 RefCountCacheHeader::RefCountCacheHeader(VersionNumber object_version)
   : magic(REFCOUNTCACHE_MAGIC_NUMBER), object_version(object_version)
 {

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