0

I have a doubly threaded linked list made up of winery elements, winery being a class with its own data fields. The problem that I was having was with winery's destructor:

winery::~winery()
{
        delete[] name;
        delete[] location;
}

I received a "double free or corruption (out): 0x0000000000402940" error in Linux, and I received this error in Windows Visual Studio:

enter image description here

So I put a breakpoint on my winery's destructor and found that it was being called every time I referenced my winery object. Strangely, the only time an error was thrown was when winery's destructor was called because my list's destructor had been called, even if winery had already been called two or three times. I would like to know why my code didn't crash the second time winery's destructor was called when it wasn't called by its parent destructor.



As a side note/question: I was able to solve this issue of memory leakage by removing winery's destructor and putting its calls in it parent destructor like this:

list::~list()
{
    for (node* next; headByName; headByName = next)
    {
        next = headByName->nextByName;
        delete[] headByName->item.getName();
        delete[] headByName->item.getLocation();
        delete headByName;
    }
}

winery is a type with name 'item' and getName() and getLocation() are functions of winery which return those cstrings. This works but it's counter-intuitive, and it certainly seems inefficient. Is it bad code? Should I be using `winery's destructor and overriding when it's called somehow?

Community
  • 1
  • 1
Larrimus
  • 211
  • 1
  • 2
  • 9
  • It is very bad code. There is a bug, but without seeing the whole code it's impossible to say what. But as a basic rule allocated data should be freed where allocated, not outside. Show at least how you're referencing the objects that causes the destructor to be called. – Sami Kuhmonen Oct 10 '15 at 04:05
  • 1
    @SamiKuhmonen The first way I reference the `winery` object was by creating it from user input. Do you really want me to post my `list::addWinery(winery &item)` function? It's pretty long. The second way I referenced the winery item was by printing the list. Should I include `list::displayByName()` as well. – Larrimus Oct 10 '15 at 04:12
  • @Larrimus You're supposed to provide an [MCVE](http://stackoverflow.com/help/mcve). Doing this would probably also make it easier for you to debug but at least makes it possible for us resolve. – James Adkison Oct 10 '15 at 04:14
  • So you're not adding a winery* but a winery? That already explains it, if you don't have a copy constructor that deep copies the data. – Sami Kuhmonen Oct 10 '15 at 04:14
  • @Larrimus Please provide your code. Showing us bits and pieces here and there does not tell us exactly what is wrong. In general, you are mismanaging the pointers and in all likelihood, not following the rule of 3 properly. But without seeing the code, that's the only thing that can be done -- speculate. And if your goal is to maintain a list of fine wines, use `std::list` instead of a homemade linked-list class and get the program completed, without bugs. – PaulMcKenzie Oct 10 '15 at 04:15
  • 1
    @PaulMcKenzie should I include the code of all 5 of my files in the question, or should I provide a link to a zip file with those 5 files in them? – Larrimus Oct 10 '15 at 04:18
  • @Larrimus What you really should be doing is making sure your linked list works correctly before you used it in a larger program. Without seeing it, I can bet it can be easily broken with a simple 3 or 4 line `main()` program. – PaulMcKenzie Oct 10 '15 at 04:21

2 Answers2

1

Your code appears to be calling delete on some of the pointers multiple times.

delete[] headByName->item.getName();
delete[] headByName->item.getLocation();

and

delete[] name;
delete[] location;

I would suggest removing the first two lines.

Also, as suggested in the comments, you might be facing problems due to not implementing the copy constructor and the copy assignment operator in winery. Have a look at What is The Rule of Three? for more insight into that problem.

Community
  • 1
  • 1
R Sahu
  • 204,454
  • 14
  • 159
  • 270
0

Your linked list implementation is flawed. You are storing winery and not a pointer to it. This means that without a proper deep copy constructor the data is not duplicated, only the pointer values. This will cause the destruction.

Either modify your linked list to store pointers (the better way) or provide a copy constructor that duplicates the data. The latter will cause more memory allocations and be slower, as well as not allow you to actually change the values so that every place the specific winery is stored would have the same value.

When not using pointers what happens is this:

{
    Winery foo("name"); // constructor copies name to a new buffer
    Winery bar = foo; // A new copy is created, shallow copy of data. This means bar.name = foo.name!
    ...
}
// automatic destruction of objects, both have the same pointers resulting in double free
Sami Kuhmonen
  • 30,146
  • 9
  • 61
  • 74
  • 1
    So I should declare `winery item;` as `winery * item;` in my list header file? – Larrimus Oct 10 '15 at 04:25
  • @Larrimus Yes, and handle the deletion of it appropriately. Smart pointers might help a lot, as well as using strings and not c style strings. – Sami Kuhmonen Oct 10 '15 at 04:33
  • 1
    I did that and replaced all my pass-by-references of the winery object: `winery & item` with pass-pointer-by-references: `winery *& item`. I commented out the `delete[]` operations in my list destructor and put them in my winery destructor. Now I have several memory leaks and no idea why. – Larrimus Oct 10 '15 at 04:39
  • 1
    It appears my `winery` destructor isn't being called when my list destructor is. Do you know how to call a function destructor manually? – Larrimus Oct 10 '15 at 04:47
  • @Larimus You have to delete them when needed, if the list is the owner then you just call `delete item` when removing from the list or deleting the list – Sami Kuhmonen Oct 10 '15 at 06:15
  • I tried that but it just says that item is undefined. I've also tried `delete * item`. – Larrimus Oct 10 '15 at 07:06