Memory problem - malloc, pointers and vector

Hi, I'm going to try to explain my problem... which is related to a bitmap painting program. I get a crash when already freed memory is getting used. It seems malloc is allocating the memory to the same location twice or something is going wrong with the pointers.

I have a std::vector<Layer> which is filled with layers. Each Layer class instance has a pointer to a memory buffer of pixels allocated using malloc. The vector is inside a class that is static and global.

When I delete a layer from the vector I do (pseudocode):

Layer layer = layers[index];

to save the layer (and the memory pointer it contains) to a place outside the vector and then I erase that Layer from the vector. I then send the saved layer to the undo system of the program. In the undo system the pixel buffer is freed when it goes out of scope.

The next time the program is run the layers are filled up again with new buffers using malloc. After that is complete it so happens that the destructor of the layer I sent to the undo system is called. It then deletes the same memory that is used by the new layer in the vector at the place where it was first deleted even though I allocated new buffers using malloc for the new layers.

Do you get it? It is kind of hard to explain...

Why isn't malloc allocating unique memory the second time?

When you free some memory, there's no reason not to use it again. So if you want unique memory, you shouldn't be free'ing the original memory.

When you do Layer layer = layers[index] does that just copy your pointer? After you free memory do you set all those copies of pointers to that memory to NULL or something, to make sure you no longer have pointers to memory you don't own?

Does layer have a working copy constructor and assignment operator? If not upon copying each copy will have a pointer to the same buffer, and each one will try to delete it in its destructor (and if one copy goes out of scope earlier, the rest will be left with a dangling pointer). You have to realize that all standard containers hold a copy of your objects, so at least one copy is bound to be created (with vector potentially more, as it may need to reallocate its memory).

If that is so, consider a) implementing correct copying behaviour (e.g copies share the same buffer through reference counting - smart pointers may help to achieve this), or b) make the class noncopyable to prevent mistakes and store (preferably smart) pointers in the vector instead.

Quote:

---

Originally Posted by tabstop

When you free some memory, there's no reason not to use it again. So if you want unique memory, you shouldn't be free'ing the original memory.

When you do Layer layer = layers[index] does that just copy your pointer? After you free memory do you set all those copies of pointers to that memory to NULL or something, to make sure you no longer have pointers to memory you don't own?

---

Yes that should just copy the pointer, after that the layer in the vector is deleted so the pointer should only exist in the copied layer at that point. I set the pointers to NULL after freeing in the destructor but somehow when layers are created again a pointer is created to the same memory.

I do:
Start the tool: layers are created.
Delete a layer: layer is sent to undo system.
Exit tool: all layers are deleted.

At that point the only memory still left should be that which is pointed to by the layer in the undo system. All the other layers in the vector are deleted and the vector is empty.

The next time I start the tool new layers are created from scratch with buffers allocated using malloc. It seems then that I get memory in the same location as that in the layer in the undo system. Not sure why this is.

Quote:

---

Originally Posted by anon

Does layer have a working copy constructor and assignment operator? If not upon copying each copy will have a pointer to the same buffer, and each one will try to delete it in its destructor (and if one copy goes out of scope earlier, the rest will be left with a dangling pointer). You have to realize that all standard containers hold a copy of your objects, so at least one copy is bound to be created (with vector potentially more, as it may need to reallocate its memory).

If that is so, consider a) implementing correct copying behaviour (e.g copies share the same buffer through reference counting - smart pointers may help to achieve this), or b) make the class noncopyable to prevent mistakes and store (preferably smart) pointers in the vector instead.

---

There is no special implementation of copy or assignment. There shouldn't be several copies of pointers pointing to the same memory. After the layer is copied the original layer is deleted so the original pointer doesn't exist anymore. I just want to move out one layer from the vector but the next time the vector is filled up it uses the same memory again. I have checked that the vector is indeed empty before I create new layers.

Quote:

---

Originally Posted by DrSnuggles

I set the pointers to NULL after freeing in the destructor but somehow when layers are created again a pointer is created to the same memory.

---

That is right and proper and exactly the best thing that could happen in the best of all possible worlds.

(In other words, what you've posted above is not the problem.)

If you're getting double free errors, then somehow you are not setting all your pointers to NULL, or when you're making more copies of your pointer than you think you are making, or something. (And note: in the situation I quoted, you won't get double free errors. If you get that same pointer back from malloc again, you can (and should!) free it again later.)

Quote:

---

Yes that should just copy the pointer, after that the layer in the vector is deleted so the pointer should only exist in the copied layer at that point. I set the pointers to NULL after freeing in the destructor but somehow when layers are created again a pointer is created to the same memory.

There is no special implementation of copy or assignment.

---

It's kind of hard to follow but I think you should google "The rule of three".

Here's a quick example of what will happen if you violate it.

Code:

---

#include <iostream>
#include <vector>

struct CopyingBroken
{
    int* p;
    CopyingBroken(int value): p(new int(value)) {}
    ~CopyingBroken()
    {
        std::cout << p << " destructor\n";
        delete p;
        p = 0;
    }

    //if you don't implement anything better, this is what the compiler provides for you
    //the pointer is shared but you have no idea how many other instances might be there,
    //all pointing to the same int
    CopyingBroken(const CopyingBroken& other): p(other.p)
    {}
    const CopyingBroken& operator= (const CopyingBroken& other)
    {
        p = other.p; //leaks memory held by p, and makes this share the pointer with other
        return *this;
    }
};

int main()
{
    std::vector<CopyingBroken> vec;
    CopyingBroken broken(1);
    vec.push_back(broken);
}

---

My output (of course the program could do anything since the result of double deletion is undefined):

Quote:

---

0x3e2450 destructor
0x3e2450 destructor

---

One of the objects is called broken, and a copy is stored in the vector. When they go out of scope, they each deallocate the same memory. Setting a member's value to NULL in the destructor has naturally no effect to any other object, similarly as you won't expect that effect from this simple code:

Code:

---

int a = 10;
int b = a;
a = 0;
//what is the value of b?

---

Thanks anon, yes the pointer is shared after it is copied which could be hazardous but directly after being copied the layer where it was copied from is removed from the vector without deleting any of its memory. That memory should then be pointed to only by the copied layer.

I free the memory of the layers manually when I delete them otherwise. It is only the one in the undo system that has the automatic destructor. Could it be that since the vector is reused and cleared that the layer pointer will get the same adress as last time and therefore point to the same memory in some way? I don't get why malloc would not allocate completley new memory though when new layer pointers are created from scratch using malloc.

Quote:

---

int a = 10;
int b = a;
a = 0;
//what is the value of b?

---

Well, b is 10...:)

Quote:

---

Originally Posted by DrSnuggles

I free the memory of the layers manually when I delete them otherwise. It is only the one in the undo system that has the automatic destructor. Could it be that since the vector is reused and cleared that the layer pointer will get the same adress as last time and therefore point to the same memory in some way? I don't get why malloc would not allocate completley new memory though when new layer pointers are created from scratch using malloc.

---

Because it is. (Once the memory has been freed, it is completely new memory again.) If you are holding a pointer to memory that has been freed, then that is your error, not the system's.

What kind of error are you getting? I've kind of been assuming it was a double delete, but looking back I'm not entirely sure.

If the memory is freed, why wouldn't malloc be allowed to reuse it again?

So, Layer just holds the pointer but doesn't try to manage the resource in any way? From the description it sounds like a rather brittle system, but seeing some actual code might help.

One more thing that can commonly go wrong: you aren't erasing items in a vector while looping over the vector?

Quote:

---

Originally Posted by tabstop

Because it is. (Once the memory has been freed, it is completely new memory again.) If you are holding a pointer to memory that has been freed, then that is your error, not the system's.

What kind of error are you getting? I've kind of been assuming it was a double delete, but looking back I'm not entirely sure.

---

The thing is memory is not freed in the undo system for use by the system, until after I have deleted all remaining layers and recreated them. Freeing the memory in the undo layer will then also free the memory in the corresponding layer in the vector, even though the vector has been filled up with layers and pointers created from scratch and new memory has been allocated for each new layer. Freeing the undo memory then shouldn't affect the layers in the vector in any way... but it does.

I do get the double freeing error when trying to delete the layers in the vector when exiting the tool but the first issue I get is that it tries to acces memory in a buffer that has already been freed by the undo layer destructor.

Quote:

---

Originally Posted by anon

If the memory is freed, why wouldn't malloc be allowed to reuse it again?

So, Layer just holds the pointer but doesn't try to manage the resource in any way? From the description it sounds like a rather brittle system, but seeing some actual code might help.

One more thing that can commonly go wrong: you aren't erasing items in a vector while looping over the vector?

---

As I said in my previous post memory should not be freed in the undo layer. It should stay separate while I create new layers.

Layer holds a pointer and memory is freed for all layers in the vector when the tool exists.

I'm erasing in the vector by doing:

Code:

---

layers.erase(layers.begin()+ index);

---

I understand it is hard for you to understand exactly what is going on but it is not easy to post code examples... I might be able to get somthing together though.

Quote:

---

Originally Posted by DrSnuggles

The thing is memory is not freed in the undo system for use by the system, until after I have deleted all remaining layers and recreated them. Freeing the memory in the undo layer will then also free the memory in the corresponding layer in the vector, even though the vector has been filled up with layers and pointers created from scratch and new memory has been allocated for each new layer. Freeing the undo memory then shouldn't affect the layers in the vector in any way... but it does.

I do get the double freeing error when trying to delete the layers in the vector when exiting the tool but the first issue I get is that it tries to acces memory in a buffer that has already been freed by the undo layer destructor.

---

I'm a bit confused here. Where do you call free -- in the destructor, in this mysterious "undo layer", both, or neither?

Quote:

---

Originally Posted by tabstop

I'm a bit confused here. Where do you call free -- in the destructor, in this mysterious "undo layer", both, or neither?

---

For the layers in the vector I call free when the tool exists, then I empty the vector.

For the layer that is sent to the undo system free is called on the layer when the destructor of the undo class that holds the layer is called. That doesn't happen in my testing until after the tool has exited and started again and the layers in the vector has been recreated. Freeing the undo layer memory will then also free the memory used in the vector.

Quote:

---

Originally Posted by DrSnuggles

For the layers in the vector I call free when the tool exists, then I empty the vector.

For the layer that is sent to the undo system free is called on the layer when the destructor of the undo class that holds the layer is called. That doesn't happen in my testing until after the tool has exited and started again and the layers in the vector has been recreated. Freeing the undo layer memory will then also free the memory used in the vector.

---

That's one too many times. The layers in the vector and the layer sent to the undo system contain the same pointer, and cannot both be freed without causing double delete issues.

Quote:

---

Originally Posted by tabstop

That's one too many times. The layers in the vector and the layer sent to the undo system contain the same pointer, and cannot both be freed without causing double delete issues.

---

No, it is not that simple. Each layer contains its own pointer and memory buffer. The layers in the vector gets freed on exit but the one sent to the undo system is not in the vector then so freeing things in the vector doesn't cause any issues the first time around. The second time the vector is filled in the undo pointer starts pointing to the same memory as the vector layer though for some reason so yes they share memory in some way.

Quote:

---

Originally Posted by DrSnuggles

The layers in the vector gets freed on exit but the one sent to the undo system is not in the vector then so freeing things in the vector doesn't cause any issues the first time around.

---

But it's a copy, you said. If you're just copying the pointer, then they point to the same place.

So does your code make sure that no memory block is referenced both in the vector and in the undo system?

I have a feeling that it takes psychic powers to tell what the problem is without any actual code.

I think I may be confusing myself here a bit too.... I found the issue. The fact that I'm undoing the layer delete makes the undo buffer in fact get the deleted on tool exit because I push back the same layer onto the vector... ahhrg.:) The strange thing is it did not complain when the undo destructor was called, only later when trying to use the buffer. I just need to check in the undo class if the layer has been pushed back and if so do not delete the buffer.

So problem solved, thanks a lot for helping out!