C++ destructor runtime error: failed to munmap

Question

I've defined a class called ClusterSet that just has one field, called clusters:

class ClusterSet {
   std::map<std::string, std::map<std::string, float>* > clusters;

   public:
      typedef std::map<std::string, std::map<std::string, float> *>::iterator iterator;
      typedef std::map<std::string, std::map<std::string, float> *>::const_iterator const_iterator;

      iterator begin() { return clusters.begin(); }
      const_iterator begin() const { return clusters.begin(); }
      iterator end() { return clusters.end(); }
      const_iterator end() const { return clusters.end(); }

      void create_cluster(std::string representative);
      void add_member(std::string representative, std::string member, float similarity);
      int write_to_file(std::string outputfile);
      int size();
      ~ClusterSet();
};

In my create_cluster method, I use new to allocate memory for the inner map, and store this pointer in clusters. I defined a destructor so that I can deallocate all this memory:

ClusterSet::~ClusterSet() {
  ClusterSet::iterator clust_it;
  for (clust_it = clusters.begin(); clust_it != clusters.end(); ++clust_it) {
      std::cout << "Deleting members for " << clust_it->first << std::endl;
      delete clust_it->second;
  }
}

When my destructor is called, it seems to deallocate all the inner maps correctly (it prints out "Deleting members for..." for each one). However, once that's done I get a runtime error that says "failed to "munmap" 1068 bytes: Invalid argument". What's causing this?

I have briefly looked at the "rule of three" but I don't understand why I would need a copy constructor or an assignment operator, or how that might solve my problem. I would never need to use either directly.

Even if you don't think you're copying your object, there are all sorts of places where you might be. And what about when you do in the future? I don't see a reason why that pointer is necessary in the first place, but at least use a smart pointer. — chris, Aug 08 '13 at 18:00
Note that you can create a private copy constructor and assignment operator so that the compiler can tell you where the "hidden" copies are being made. — , Aug 08 '13 at 18:01
"I would never need to use either directly" - maybe not, but it's very easy to write code that uses them indirectly. At the very least, you should delete them to prevent invalid copying. — Mike Seymour, Aug 08 '13 at 18:02
@Lalaland, It could be happening in the class, though. You can use `= delete`. — chris, Aug 08 '13 at 18:03
Also, "I use `new` to allocate memory for the inner map." Why on earth would you do that? Just store map objects, and all your copying woes are fixed by the Rule of Zero. — Mike Seymour, Aug 08 '13 at 18:03
@chris The reason I'm using a pointer there is because I was under the impression that there would be a performance penalty with just using the object itself. If I just use the object, wouldn't the outer map have to grow every time I add a member to the inner map? I'm going to be creating inner maps and adding a lot of elements to them in no particular order. — FrancesKR, Aug 08 '13 at 18:05
@FrancesKR: There will be a performance penalty in using pointers: more allocations (of the outer map nodes *and* the manually allocated inner map objects), and more levels of indirection to access the inner elements. And no, the outer map won't grow if you modify the inner ones. — Mike Seymour, Aug 08 '13 at 18:06
@FrancesKR, Use `std::unordered_map` if you don't care about the order and never make assumptions before profiling. Move semantics should take care of it. — chris, Aug 08 '13 at 18:06
@chris I care about the order of elements in the outer map, I just meant that I won't be creating the inner maps and adding elements to them in order. I guess I'll stick with std::map. — FrancesKR, Aug 08 '13 at 18:12
@FrancesKR: Move semantics are a relatively new language feature, allowing the innards of a complex object to be efficiently moved (rather than copied) from one object to another. A lot of the time, this happens automatically (especially if you *don't* mess around trying to manage your own memory allocations); but it's worth learning about to make sure your classes can be handled as efficiently as possible. But that's beyond the scope of this question. — Mike Seymour, Aug 08 '13 at 18:17
@FrancesKR, Here's a thorough explanation: http://stackoverflow.com/questions/3106110/what-is-move-semantics — chris, Aug 08 '13 at 18:20
@MikeSeymour Thanks! However, I have to use gcc 4.2.1, which apparently doesn't support move semantics. Would you still argue against using pointers? — FrancesKR, Aug 08 '13 at 18:43
@FrancesKR: Yes; move semantics have very little to do with this question. The reasons for avoiding unnecessary memory allocation, and especially management via raw pointers and manual deletion, are just as valid even if you're stuck with a previous decade's compiler. — Mike Seymour, Aug 08 '13 at 20:41

score 1 · Accepted Answer · answered Aug 08 '13 at 18:12

There's no good reason (and plenty of disadvantages) for dynamically allocating the inner maps. Change the outer map type to

std::map<std::string, std::map<std::string, float> >

and then you won't need to implement your own destructor and copy/move semantics at all (unless you want to change those you get from the map, perhaps to prevent copying your class).

If, in other circumstances, you really do need to store pointers to objects and tie their lifetime to their presence in the map, store smart pointers:

std::map<std::string, std::unique_ptr<something> >

If you really want to manage their lifetimes by hand for some reason, then you will need to follow the Rule of Three and give your class valid copy semantics (either preventing copying by deleting the copy constructor/assignment operator, or implementing whatever semantics you want). Even if you don't think you're copying objects, it's very easy to write code that does.

C++ destructor runtime error: failed to munmap

1 Answers1