1

When I compile with Xcode and run this, I get the error, at least 100 times in a row, malloc: *** error for object 0x100180: double free, with the debugger pointing to line C. Strangely, In the code I distilled this from, the exact same error occurs, but the debugger points to the equivalent of line B. I tried but couldn't reproduce that.

If I remove line A, the code works but I get a major memory leak that crashes the program in about 1 minute. Removing line C solves the issue but isn't a valid solution because then a_class doesn't have a proper destructor.

#include <iostream>
#include <vector>

struct a_struct
{
    int* dynamic_array;
    a_struct(int length) {dynamic_array = new int [length];}
    a_struct() : dynamic_array(NULL) {}
    ~a_struct() { if (dynamic_array != NULL) {delete [] dynamic_array;} }  //Line A
};

class a_class
    {
    public:
        a_struct* the_structs;
        a_class() {Init();}
        a_class(a_class const & origin) {Init();}
        void Init(){
            the_structs = new a_struct [10];                              //Line B 
            for(int q=0; q<10; q++)
                the_structs[q] = a_struct(7);}
        ~a_class() { if (the_structs != NULL) {delete [] the_structs;} }  //Line C
    };

int main () 
{
    std::vector <a_class> the_objects;
    for(int q=0; q<10; q++)
        the_objects.push_back(a_class());

    while(1)
        for(int q=0; q <10; q++)
            for(int w=0; w <10; w++)
                the_objects[q].the_structs[w] = a_struct(7);
}
Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
Matt Munson
  • 2,903
  • 5
  • 33
  • 52
  • PS. The test for NULL in the destructor is a waste of time. It is perfectly valid to call delete on NULL. – Martin York Jul 18 '11 at 04:28
  • Worth using valgrind to help track down this kind of issue. It will tell you where the memory is deleted and where it is allocated when a double delete occurs. That usually makes it pretty easy to track down the cause. – Michael Anderson Jul 18 '11 at 04:40

4 Answers4

4

Fix both a_struct and a_class to have proper copy constructors and assignment operators. Then come back if there's still a problem.

Make sure all of your classes always follow The Rule of 3. I seem to be linking to that page a lot lately.

Community
  • 1
  • 1
Benjamin Lindley
  • 101,917
  • 9
  • 204
  • 274
2

The problem is you are trying to do your own memory management.

Use the built in containers:

struct a_struct
{
    std::vector<int>    dynamic_array;
    a_struct(int length) : dynamic_array(length){}
    a_struct()           : dynamic_array() {}
};

class a_class
{
    public:
        std::vector<a_struct>   the_structs;
        a_class()                       {Init();}
        a_class(a_class const & origin) {Init();}
        void Init()
        {
            the_structs.resize(10); 
            for(int q=0; q<10; q++)
                the_structs[q] = a_struct(7);
        }
};
Martin York
  • 257,169
  • 86
  • 333
  • 562
  • I think everyone should know how to manage memory, whether they ever do it or not. As such, I don't think telling people they shouldn't when they are trying to learn how is constructive. – IronMensan Jul 18 '11 at 04:55
  • I agree that everybody should learn how to do it. But what is more important is to learn when to do it. This is obviously the incorrect place to do it. The learning point here is `Separation of concerns`. A class should either do resource management or business logic not both. – Martin York Jul 18 '11 at 06:37
  • I think the best way to really grok containers is first implement your own half-baked buggy and slow ones (to understand implications) and then "discover" standard libraries one and being able to understand how they work and trash your own bad versions. Just learning about standard containers is not IMO a good path because a lot of the implications look "magic" and are just hard to memorize strange rules if you have no idea of what happens behind the curtains. In `std::vector` for example what `reserve` do, why `v.erase(v.begin())` is slow or why `push_back` may invalidate iterators. – 6502 Jul 18 '11 at 07:01
  • Martin, you haven't taken care for the error scenario if `length` is too large to accomodate the `vector` in `a_struct(int length) : dynamic_array(length)`; also using `vector` would slow down the execution (which would defeat the purpose of OP) to use a raw pointer. It's stupid that you downvoted my answer for one memory leak, but you yourself has put some flaws in your code. Anyways, I am not one of them who will downvote just to bring someone else down. – iammilind Jul 18 '11 at 07:18
  • @iammilind: 1) Yes I have take into account different values of length. 2) Using vector will definitely **NOT** slow down execution (it is at least as fast and can be faster see: http://stackoverflow.com/questions/3664272/stdvector-is-so-much-slower-than-plain-arrays/3664349#3664349 ). I did give you the down vote. But it was not for one memory leak. It was for a host of problems in your code (which if you fix I will remove). To be blunt your code is seriously flawed (and I given you hints to fix it). On the other hand this code works perfectly. – Martin York Jul 18 '11 at 15:28
  • @Martin: After struggling with memory leaks for a couple of days I have come to see your wisdom here Martin. For now, properly learning memory management is definitely a fight for another day. – Matt Munson Jul 19 '11 at 07:52
  • @Matt: Most of what you need is here: http://stackoverflow.com/questions/255612/c-dynamically-allocating-an-array-of-objects/255744#255744 – Martin York Jul 19 '11 at 08:37
0

The problem is a_struct doesn't have proper copy constructor and operator =. Due to this at a time two a_struct::dynamic_array are pointing to the same memory location and both will be calling delete[] dynamic_array; in the destructor. Hence, double delete.

Always, remember that whenever you have a row pointer inside a class you should make a deep copy(where you copy the contents pointed by pointer) and not a shallow copy (just the address).

In your case, you can change the definition like this:

struct a_struct
{
  int length_;  //<<--- add this for record
  int* dynamic_array;
  a_struct(int length) : length_(length) {dynamic_array = new int [length_]; }
  a_struct() : dynamic_array(NULL) {}
  a_struct(const a_struct &copy)  // <<--- copy ctor
  {
    do_copy(copy);
  }
  a_struct& operator = (const a_struct &copy)  // <<--- assignment operator
  {
    delete[] dynamic_array; // clear earlier array
    do_copy(copy);
    return *this;
  }
  ~a_struct() { if (dynamic_array != NULL) {delete [] dynamic_array;} }  //Line A

  void do_copy (const a_struct &copy)
  {
    // do necessary null checks for "copy"
    dynamic_array = new int[length_ = copy.length];
    memcpy(dynamic_array, copy.dynamic_array, length_);
  }
};

Same kind of precaution you take for any such class.

iammilind
  • 68,093
  • 33
  • 169
  • 336
  • Where does this function `memcpy(dynamic_array, copy.dynamic_array, length_);` come from and what does it do? – Matt Munson Jul 18 '11 at 04:49
  • @Matt, `memcpy()` is a function in ``. It copies the content of `copy.dynamic_array` into `this->dynamic_array` for given `length_`. See here: http://www.cplusplus.com/reference/clibrary/cstring/memcpy/ – iammilind Jul 18 '11 at 04:52
  • It actually turns out that a copy constructor is not necessary here, contrary to what many have suggested: just an assignment operator is needed. But you were right to see the need for the assignment operator and your code allowed me figure it out. Actually, the assignment operator doesn'tneed to copy anything, it just needs to be there to preclude the default shallow copy. It may seem odd, but the way I'm implementing these I don't need the dynamic array information to be copied (actually, I need it to be reset). Anyways, thanks for the help. – Matt Munson Jul 18 '11 at 05:18
  • Please if you are going to try and give advice, please learn to do it correctly. Your assignment operator is completely broken. Leaks under some conditions de-references in NULL in others. – Martin York Jul 18 '11 at 06:35
  • @Matt Munson: You **ABSOLUTELY** do need both. If you don;t have both your code will be broken because the default copy constructor is doing the wrong things. Please go take the advice of the other answers and implement the rule of three, including the copy constructor. Even if this means the copy constructor resets the object. The answers by @Benjamin and @Mark are much better than this one (mainly because of the flawed code). – Martin York Jul 18 '11 at 06:39
  • @Martin, can elaborate more what is going wrong ? Here, it was not expected that, I should take care of every error scenario (like null check, `std::bad_alloc`, `delete/delete[]`). I have provided this pseudo code to just give an idea. It's left on OP to get those things corrected. – iammilind Jul 18 '11 at 06:42
  • @iammilind: When you do an assignment what happens to the old value of dynamic_array? Also what happens if the object on the RHS has a dynamic_array that is NULL? The code to do it correctly is so simple that yes I would expect a lot better tan this: Read here: http://stackoverflow.com/questions/255612/c-dynamically-allocating-an-array-of-objects/255744#255744 – Martin York Jul 18 '11 at 06:50
  • @Martin, I agree that, `delete[] dynamic_array;` was missed in assignment operator (which is now edited). Which was the only flaw in fact. – iammilind Jul 18 '11 at 06:57
  • @6502, you haven't seen the edited version. @Martin had already pointed out the same, which I edited when you were reading the older version. – iammilind Jul 18 '11 at 06:58
  • The edited version wasn't there, but now that's visible still this is bad code (in case of an exception during `do_copy` you end up with a dangling pointer in your instance because you already deleted old content but you're still pointing at it). The best approach is to implement `swap` and then implement `operator=` combining copy constructor and swap: see http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom – 6502 Jul 18 '11 at 07:06
  • @6502, I am not begging to take back your downvote. I already mention that, I wrote my answer to give an idea. There can be N number of error scenarios. OP should be the best person to figure it out. Thanks. – iammilind Jul 18 '11 at 07:12
  • @iammilind: Sorry but I see no reason for which bad code should be shown as an example without saying it's bad code. Moreover your code is also bad for a few other reasons, for example `if (dynamic_array != NULL) { delete[] dynamic_array; }` suggests the idea that calling `delete[]` on a null pointer does something bad (while instead is just a NOP) and `dynamic_array = new int[length_ = copy.length_];` is bad code because in case of an exception you already modified the `length_` member. – 6502 Jul 18 '11 at 08:25
  • @6502, `if (dynamic_array != NULL)` is part of OP's code. My code, I have put explicit comments. Exception and all part has to be taken care in production code, not in example code. – iammilind Jul 18 '11 at 08:30
  • @iammilind: Still broken. What happens when the RHS has a NULL dynamic_array. It difenitely does not provide any exception grantees (like it should). – Martin York Jul 18 '11 at 15:20
0

The problem is that the implicitly generated copy constructor in a_struct doesn't do a deep copy, but the destructor always assumes that memory was allocated for each instance. You need to create a copy constructor for it that does a deeps copy. But better is to use just std::vector instead of new and delete. Then you don't have to worry about remembering to keep all your constructors correct.

Additionally the copy constructor for a_class doesn't actually copy an object. This is almost surely going to cause you problems sometime down the line when it doesn't support expected copy semantics.

Mark B
  • 95,107
  • 10
  • 109
  • 188
  • I'm pretty confused. I added a copy constructor `a_struct(a_struct const & origin) : dynamic_array(NULL) {}` and the problem is still there. – Matt Munson Jul 18 '11 at 04:46