Thread: Memory problem - malloc, pointers and vector

  1. #1
    Registered User
    Join Date
    Oct 2007
    Posts
    166

    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?

  2. #2
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    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?

  3. #3
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  4. #4
    Registered User
    Join Date
    Oct 2007
    Posts
    166
    Quote Originally Posted by tabstop View Post
    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.

  5. #5
    Registered User
    Join Date
    Oct 2007
    Posts
    166
    Quote Originally Posted by anon View Post
    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.

  6. #6
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Quote Originally Posted by DrSnuggles View Post
    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.)

  7. #7
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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):

    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?
    Last edited by anon; 08-14-2009 at 08:03 AM.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  8. #8
    Registered User
    Join Date
    Oct 2007
    Posts
    166
    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.

    int a = 10;
    int b = a;
    a = 0;
    //what is the value of b?
    Well, b is 10...
    Last edited by DrSnuggles; 08-14-2009 at 08:36 AM.

  9. #9
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Quote Originally Posted by DrSnuggles View Post
    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.

  10. #10
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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?
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  11. #11
    Registered User
    Join Date
    Oct 2007
    Posts
    166
    Quote Originally Posted by tabstop View Post
    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.

  12. #12
    Registered User
    Join Date
    Oct 2007
    Posts
    166
    Quote Originally Posted by anon View Post
    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.

  13. #13
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Quote Originally Posted by DrSnuggles View Post
    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?

  14. #14
    Registered User
    Join Date
    Oct 2007
    Posts
    166
    Quote Originally Posted by tabstop View Post
    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.

  15. #15
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Quote Originally Posted by DrSnuggles View Post
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. vector of arrays of pointers to structures
    By Marksman in forum C++ Programming
    Replies: 13
    Last Post: 02-01-2008, 04:44 AM
  2. Problems defining classes
    By esmeco in forum C++ Programming
    Replies: 47
    Last Post: 10-24-2007, 01:13 PM
  3. vector of pointers
    By gamer4life687 in forum C++ Programming
    Replies: 3
    Last Post: 09-26-2005, 10:49 PM
  4. Locating A Segmentation Fault
    By Stack Overflow in forum C Programming
    Replies: 12
    Last Post: 12-14-2004, 01:33 PM
  5. Certain functions
    By Lurker in forum C++ Programming
    Replies: 3
    Last Post: 12-26-2003, 01:26 AM