Seg fault after is item pushed onto STL container

Question

typedef struct temp  
{  
        int a,b;  
        char  *c;  
        temp(){ c = (char*)malloc(10);};  
        ~temp(){free(c);};  
}temp;  

int main()  
{  
   temp a;  
   list<temp>   l1;  
   l1.push_back(a);  
   l1.clear();  
   return 0;  

}

giving segmentation fault.

Answer 1

Misread question while it was initially being added (answer withdrawn).

Answer 2

You don't have a copy constructor.

When you push 'a' into the list, it gets copied. Because you don't have a copy constructor (to allocate memory for c and copy from old c to new c) c is the same pointer in a and the copy of a in the list.

The destructor for both a's gets called, the first will succeed, the second will fail because the memory c points to has already been freed.

You need a copy constructor.

To see whats happening, put some couts in the constructors and destructors and step through the code.

Answer 3

You need a copy constructor & an assignment operator - things stored in an STL collection are stored as copies and can be re-assigned as the vector changes size.

It's difficult to see from your code exactly what the semantics of the copy constructor should be, but at minimum it's cot to allocate some memory so that (if nothing else) the destructor has something to free. The assignment operator is equally difficult to specify without more details of your class.

Answer 4

You need a deep-copy constructor to avoid double free(). You have a variable of temp class (a), then you add it to the list. The variable is copied. Then you clear the list, the element inside is destroyed and free() is called. Then a variable is destroyed and free() is called again for the same address which leads to segmentation fault.

You need a copy constructor for deep copying class temp variables which would malloc() another buffer and copy data.

Answer 5

At the time when you call l1.push_back(a) a second copy of 'a' is copy-constructed. As a result there are now two classes that believe they own the memory from the original malloc call, and when the second is deleted it will try to free memory deleted by the first.

The solution is to add a copy constructor that deals with the fact that instance of the class does not actually own the data. Typically you would do this by having some form of reference count.

Answer 6

You need to define a copy constructor for temp. Right now what happens when you push a into the list is a copy of a is made. The copy (call it a2) is initialized by saying a2.c = a.c. This means both a and a2 point to the same block of memory. When their destructors are called, that block of memory is free'd twice, leading to the segmentation fault.

Given what you've posted, the copy constructor should probably be something like this:

temp::temp (temp const &rhs)
{
    this->a = rhs.a;
    this->b = rhs.b;
    this->c = (char *) malloc (10);
    memcpy (this->c, rhs.c, 10);
}

This assumes that what c points to is always 10 chars long...

Answer 7

If you don't want to add copy constructor you can consider list of pointers to values instead of list of values.

list<temp*>   l1;
l1.push_back( new temp() );

But then you have to delete manually each object in list to prevent memory leak.

Also a,b members in your struct are not initialized. Be careful.

Answer 8

Apart from the fixes given, you should avoid using malloc/free in C++. In your particular case, i'd go with a vector :

#include <vector>

 typedef struct temp  
{  
        int a,b;  
        std::vector<char> c;  
        temp(){ c.reserve(10);};  
}temp;  

int main()  
{  
   temp a;  
   list<temp>   l1;  
   l1.push_back(a);  
   l1.clear();  
   return 0;  

}

Answer 9

In addition to the copy constructor, it is wise to provide an = operator too in this case.

struct temp {   // typedef is implicit in C++
  int a,b;
  char * c;

  // Constructor
  temp() { c = malloc(10); }
  // Destructor
  ~temp() { free(c); }
  // Copy constructor
  temp(const temp & x) { c = malloc(10); setTo(x); }
  // Operator =
  temp & operator = (const temp & x) { setTo(x); return *this; }

  // Initialize THIS to X
  void setTo(const temp & x) { a = x.a; b = x.b; memcpy(c,x.c,10); }
};

Answer 10

Another problem with this code are extra semicolons in

temp(){ c = (char*)malloc(10);};
~temp(){free(c);};

it is better to remove them:

temp(){ c = (char*)malloc(10);}
~temp(){free(c);}

Answer 11

How ironic that this had the "STL" tag but the lack of STL is what's causing the problem.