From 425909a2f2bd4b506ca83f2e761166688be36b0c Mon Sep 17 00:00:00 2001
From: Ammar Ben Khadra <ammarbenkhadra@gmail.com>
Date: Thu, 9 Aug 2018 11:42:35 +0200
Subject: [PATCH] elf: fix circular dependency in elf object

object elf (parent) owns the memory of its children, namely, sections
and segments. However, instances of the latter share memory ownership
of the parent elf due to its pimpl implementation using shared_ptr.
This creates a circular dependency that prevents cleaning the memory
acquired by elf objects.

I break this circular dependency through the standard mechanism
of weak_ptr. Basically, the children now own a weak_ptr to the
*implementation* of their parent elf object. They instantiate
an instance of the parent only when an action involving the parent
is needed.
---
 elf/elf++.hh |  2 ++
 elf/elf.cc   | 62 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/elf/elf++.hh b/elf/elf++.hh
index ee59ed0..be5d641 100644
--- a/elf/elf++.hh
+++ b/elf/elf++.hh
@@ -114,6 +114,8 @@ public:
          */
         const section &get_section(unsigned index) const;
 
+        friend class section;
+        friend class segment;
 private:
         struct impl;
         std::shared_ptr<impl> m;
diff --git a/elf/elf.cc b/elf/elf.cc
index 587329f..6622144 100644
--- a/elf/elf.cc
+++ b/elf/elf.cc
@@ -167,15 +167,28 @@ elf::get_segment(unsigned index) const
 
 struct segment::impl {
         impl(const elf &f)
-                : f(f) { }
+                : p(f.m) { }
 
-        const elf f;
+        elf get_elf();
+
+        const weak_ptr<elf::impl> p;
         Phdr<> hdr;
         //  const char *name;
         //  size_t name_len;
         const void *data;
 };
 
+elf
+segment::impl::get_elf()
+{
+        if (p.expired()) {
+                throw runtime_error("invalid elf reference!");
+        }
+        auto f = elf();
+        f.m = p.lock();
+        return f;
+}
+
 segment::segment(const elf &f, const void *hdr)
     : m(make_shared<impl>(f)) {
         canon_hdr(&m->hdr, hdr, f.get_hdr().ei_class, f.get_hdr().ei_data);
@@ -188,9 +201,11 @@ segment::get_hdr() const {
 
 const void *
 segment::data() const {
-        if (!m->data)
-                m->data = m->f.get_loader()->load(m->hdr.offset,
-                                                  m->hdr.filesz);
+        if (!m->data) {
+                auto f = m->get_elf();
+                m->data = f.get_loader()->load(m->hdr.offset,
+                                               m->hdr.filesz);
+        }
         return m->data;
 }
 
@@ -223,15 +238,28 @@ enums::to_string(shn v)
 struct section::impl
 {
         impl(const elf &f)
-                : f(f), name(nullptr), data(nullptr) { }
+                : p(f.m), name(nullptr), data(nullptr) { }
 
-        const elf f;
+        elf get_elf();
+
+        const weak_ptr<elf::impl> p;
         Shdr<> hdr;
         const char *name;
         size_t name_len;
         const void *data;
 };
 
+elf
+section::impl::get_elf()
+{
+        if (p.expired()) {
+                throw runtime_error("invalid elf reference!");
+        }
+        auto f = elf();
+        f.m = p.lock();
+        return f;
+}
+
 section::section(const elf &f, const void *hdr)
         : m(make_shared<impl>(f))
 {
@@ -248,9 +276,11 @@ const char *
 section::get_name(size_t *len_out) const
 {
         // XXX Should the section name strtab be cached?
-        if (!m->name)
-                m->name = m->f.get_section(m->f.get_hdr().shstrndx)
+        if (!m->name) {
+                auto f = m->get_elf();
+                m->name = f.get_section(f.get_hdr().shstrndx)
                         .as_strtab().get(m->hdr.name, &m->name_len);
+        }
         if (len_out)
                 *len_out = m->name_len;
         return m->name;
@@ -267,8 +297,10 @@ section::data() const
 {
         if (m->hdr.type == sht::nobits)
                 return nullptr;
-        if (!m->data)
-                m->data = m->f.get_loader()->load(m->hdr.offset, m->hdr.size);
+        if (!m->data) {
+                auto f = m->get_elf();
+                m->data = f.get_loader()->load(m->hdr.offset, m->hdr.size);
+        }
         return m->data;
 }
 
@@ -283,7 +315,8 @@ section::as_strtab() const
 {
         if (m->hdr.type != sht::strtab)
                 throw section_type_mismatch("cannot use section as strtab");
-        return strtab(m->f, data(), size());
+        auto f = m->get_elf();
+        return strtab(f, data(), size());
 }
 
 symtab
@@ -291,8 +324,9 @@ section::as_symtab() const
 {
         if (m->hdr.type != sht::symtab && m->hdr.type != sht::dynsym)
                 throw section_type_mismatch("cannot use section as symtab");
-        return symtab(m->f, data(), size(),
-                      m->f.get_section(get_hdr().link).as_strtab());
+        auto f = m->get_elf();
+        return symtab(f, data(), size(),
+                      f.get_section(get_hdr().link).as_strtab());
 }
 
 //////////////////////////////////////////////////////////////////