Merge pull request #32 from embarktrucks/liam/fix_mem_leak

Fix Embag Memory Leaks
embarktrucks · Dec 6, 2021 · 4396eac · 4396eac
2 parents 4dd6278 + e331552
commit 4396eac
Show file tree

Hide file tree

Showing 10 changed files with 181 additions and 96 deletions.
diff --git a/lib/message_parser.cc b/lib/message_parser.cc
@@ -9,15 +9,15 @@
 
 namespace Embag {
 
-const RosValue* MessageParser::parse() {
+const RosValue::RosValuePointer MessageParser::parse() {
   // The lowest number of RosValues occurs when we have a message with only doubles in a single type.
   // The number of RosValues in this case is the number of doubles that can fit in our buffer,
   // plus one for the RosValue object that will point to all the doubles.
   ros_values_->reserve(message_buffer_->size() / sizeof(double) + 1);
   ros_values_->emplace_back(msg_def_.fieldIndexes());
   ros_values_offset_ = 1;
   initObject(0, msg_def_);
-  return &ros_values_->front();
+  return RosValue::RosValuePointer(ros_values_);
 }
 
 void MessageParser::initObject(size_t object_offset, const RosMsgTypes::BaseMsgDef &object_definition) {

diff --git a/lib/message_parser.h b/lib/message_parser.h
@@ -24,7 +24,7 @@ class MessageParser {
   {
   };
 
-  const RosValue* parse();
+  const RosValue::RosValuePointer parse();
 
  private:
   static std::unordered_map<std::string, size_t> primitive_size_map_;

diff --git a/lib/ros_message.h b/lib/ros_message.h
@@ -24,12 +24,12 @@ class RosMessage {
   {
   }
 
-  const RosValue &data() {
+  const RosValue::RosValuePointer &data() {
     if (!parsed_) {
       hydrate();
     }
 
-    return *data_;
+    return data_;
   }
 
   bool has(const std::string &key) {
@@ -58,7 +58,7 @@ class RosMessage {
 
  private:
   bool parsed_ = false;
-  const RosValue* data_;
+  RosValue::RosValuePointer data_;
   std::shared_ptr<RosMsgTypes::MsgDef> msg_def_;
 
   void hydrate() {

diff --git a/lib/ros_value.cc b/lib/ros_value.cc
@@ -5,23 +5,23 @@
 
 namespace Embag {
 
-const RosValue &RosValue::operator()(const std::string &key) const {
+const RosValue::RosValuePointer RosValue::operator()(const std::string &key) const {
   return get(key);
 }
 
-const RosValue &RosValue::operator[](const std::string &key) const {
+const RosValue::RosValuePointer RosValue::operator[](const std::string &key) const {
   return get(key);
 }
 
-const RosValue &RosValue::operator[](const size_t idx) const {
+const RosValue::RosValuePointer RosValue::operator[](const size_t idx) const {
   return at(idx);
 }
 
-const RosValue &RosValue::at(const std::string &key) const {
+const RosValue::RosValuePointer RosValue::at(const std::string &key) const {
   return get(key);
 }
 
-const RosValue &RosValue::get(const std::string &key) const {
+const RosValue::RosValuePointer RosValue::get(const std::string &key) const {
   if (type_ != Type::object) {
     throw std::runtime_error("Value is not an object");
   }
@@ -40,7 +40,7 @@ const std::string RosValue::as<std::string>() const {
   return std::string(string_loc, string_loc + string_length);
 }
 
-const RosValue &RosValue::at(const size_t idx) const {
+const RosValue::RosValuePointer RosValue::at(const size_t idx) const {
   if (type_ != Type::array) {
     throw std::runtime_error("Value is not an array");
   }
@@ -99,13 +99,13 @@ std::string RosValue::toString(const std::string &path) const {
       std::ostringstream output;
       for (const auto& field : *object_info_.field_indexes) {
         if (path.empty()) {
-          output << object_info_.children.at(field.second).toString(field.first);
+          output << object_info_.children.at(field.second)->toString(field.first);
         } else {
-          output << object_info_.children.at(field.second).toString(path + "." + field.first);
+          output << object_info_.children.at(field.second)->toString(path + "." + field.first);
         }
 
         // No need for a newline if our child is an object or array
-        const auto &object_type = object_info_.children.at(field.second).getType();
+        const auto &object_type = object_info_.children.at(field.second)->getType();
         if (!(object_type == Type::object || object_type == Type::array)) {
           output << std::endl;
         }
@@ -116,7 +116,7 @@ std::string RosValue::toString(const std::string &path) const {
       std::ostringstream output;
       for (size_t i = 0; i < array_info_.children.length; ++i) {
         const std::string array_path = path + "[" + std::to_string(i) + "]";
-        output << array_info_.children.at(i).toString(array_path) << std::endl;
+        output << array_info_.children.at(i)->toString(array_path) << std::endl;
       }
       return output.str();
     }
@@ -141,7 +141,7 @@ const std::string& RosValue::const_iterator<const std::string&, std::unordered_m
 }
 
 template<>
-const std::pair<const std::string&, const RosValue&> RosValue::const_iterator<const std::pair<const std::string&, const RosValue&>, std::unordered_map<std::string, size_t>::const_iterator>::operator*() const {
+const std::pair<const std::string&, const RosValue::RosValuePointer> RosValue::const_iterator<const std::pair<const std::string&, const RosValue::RosValuePointer>, std::unordered_map<std::string, size_t>::const_iterator>::operator*() const {
   return std::make_pair(index_->first, value_.object_info_.children.at(index_->second));
 }
 

diff --git a/lib/ros_value.h b/lib/ros_value.h
@@ -8,37 +8,48 @@
 #include <vector>
 
 #include "span.hpp"
+#include "util.h"
 
 namespace Embag {
 
 class RosValue {
  public:
-  struct ros_value_list_t {
-    std::shared_ptr<std::vector<RosValue>> base;
-    size_t offset;
-    size_t length;
-
-    const RosValue& at(size_t index) const {
-      return base->at(offset + index);
+  class RosValuePointer : public VectorItemPointer<RosValue> {
+   public:
+    RosValuePointer()
+    {
     }
-  };
 
-  struct RosValuePointer {
-    std::shared_ptr<std::vector<RosValue>> base;
-    size_t index;
+    RosValuePointer(const std::weak_ptr<std::vector<RosValue>>& base)
+      : RosValuePointer(base, 0)
+    {
+    }
 
-    RosValuePointer(std::shared_ptr<std::vector<RosValue>> base, size_t index)
-    : base(base)
-    , index(index)
+    RosValuePointer(const std::weak_ptr<std::vector<RosValue>>& base, size_t index)
+      : VectorItemPointer<RosValue>(base.lock(), index)
     {
     }
 
-    const RosValue *operator->() const {
-      return &base->at(index);
+    const RosValuePointer operator()(const std::string &key) const {
+      return (**this)(key);
     }
 
-    const RosValue& operator*() const {
-      return base->at(index);
+    const RosValuePointer operator[](const std::string &key) const {
+      return (**this)[key];
+    }
+
+    const RosValuePointer operator[](const size_t idx) const {
+      return (**this)[idx];
+    }
+  };
+
+  struct ros_value_list_t {
+    std::weak_ptr<std::vector<RosValue>> base;
+    size_t offset;
+    size_t length;
+
+    const RosValuePointer at(size_t index) const {
+      return RosValuePointer(base, offset + index);
     }
   };
 
@@ -244,16 +255,16 @@ class RosValue {
 
 
   // Convenience accessors
-  const RosValue &operator()(const std::string &key) const;
-  const RosValue &operator[](const std::string &key) const;
-  const RosValue &operator[](const size_t idx) const;
-  const RosValue &get(const std::string &key) const;
-  const RosValue &at(size_t idx) const;
-  const RosValue &at(const std::string &key) const;
+  const RosValuePointer operator()(const std::string &key) const;
+  const RosValuePointer operator[](const std::string &key) const;
+  const RosValuePointer operator[](const size_t idx) const;
+  const RosValuePointer get(const std::string &key) const;
+  const RosValuePointer at(size_t idx) const;
+  const RosValuePointer at(const std::string &key) const;
 
   template<typename T>
   const T &getValue(const std::string &key) const {
-    return get(key).as<T>();
+    return get(key)->as<T>();
   }
 
   template<typename T>

diff --git a/lib/util.h b/lib/util.h
@@ -11,4 +11,70 @@ std::unique_ptr<T> make_unique(Args &&... args) {
 }
 
 using message_stream = boost::iostreams::stream<boost::iostreams::array_source>;
+
+template<class T>
+class VectorItemPointer;
+
+}
+
+namespace pybind11 { namespace detail {
+  template<class Holder>
+  struct holder_helper;
+
+  template <class T>
+  struct holder_helper<Embag::VectorItemPointer<T>> {
+    static const T* get(const Embag::VectorItemPointer<T> &vip) {
+      return vip.get();
+    }
+  };
+}}
+
+namespace Embag {
+
+template<class T>
+class VectorItemPointer {
+  std::shared_ptr<std::vector<T>> base;
+  size_t index;
+
+ public:
+  VectorItemPointer(const std::shared_ptr<std::vector<T>>& base, size_t index)
+  : base(base)
+  , index(index)
+  {
+  }
+
+  VectorItemPointer(const std::weak_ptr<std::vector<T>>& base, size_t index)
+  : VectorItemPointer(base.lock(), index)
+  {
+  }
+
+  VectorItemPointer(T*& ref)
+  : VectorItemPointer()
+  {
+    throw std::runtime_error("This should never be called");
+  }
+
+  VectorItemPointer()
+  : index(0)
+  {
+  }
+
+  const T* operator->() const {
+    return get();
+  }
+
+ protected:
+  // We don't want any public interface that exposes the underlying item as without
+  // a shared_ptr to the vector that contains the item, the memory may be freed.
+  friend const T* pybind11::detail::holder_helper<VectorItemPointer<T>>::get(const VectorItemPointer<T>& vip);
+
+  const T* get() const {
+    return &**this;
+  }
+
+  const T& operator*() const {
+    return base->at(index);
+  }
+};
+
 }