Valgrind- memory errors and default destructors

Pages: 12
Apologies in advance for the long post! My question is a little involved, and i tried to limit the code blocks as much as possible!

Background: A reply to a previous (unrelated) post pointed me in the direction of valgrind for tracking down memory errors. I have since been applying it to my previous code (With depressing results :D). My current issue is with a program that uses a singly linked list to store journal entry objects. Valgrind is showing me mismatched free / delete / delete [] errors that seem related to the default destructors, but I need a little help understanding exactly what is happening.

Problem Statement: When running the attached code through valgrind I get the following errors:

==4157== Mismatched free() / delete / delete []
==4157== at 0x483C74F: operator delete[](void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==4157== by 0x109316: journal_entry::~journal_entry() (in /home/jrbobdobbs83/vimc/psuprep/prac/CS163/Lab2/a.out)
==4157== by 0x10972B: node::~node() (in /home/jrbobdobbs83/vimc/psuprep/prac/CS163/Lab2/a.out)
==4157== by 0x10929B: list::~list() (in /home/jrbobdobbs83/vimc/psuprep/prac/CS163/Lab2/a.out)
==4157== by 0x109A04: main (cs163_lab2.cpp:11)
==4157== Address 0x4dba600 is 0 bytes inside a block of size 8 alloc'd
==4157== at 0x483A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==4157== by 0x109781: journal_entry::copy_entry(journal_entry const&) (cs163_entry.cpp:15)
==4157== by 0x109B2D: list::add(journal_entry&) (cs163_list.cpp:14)
==4157== by 0x109988: main (cs163_lab2.cpp:34)
==4157==

At the end of the program. From the errors and my research, it seems pretty clearly that memory is not being deallocated in the correct way by the default destructors. I've tried writing an explicit destructor which doesn't seem to help. My question is: What is specifically happening with the destructors, and how do I write a destructor that will correctly deallocate for this?

list header:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
struct node
{
       journal_entry entry;
       node * next;
};

class list
{
      public:

             list(void);
             ~list(void);

             void delist(node *); //Recursive list delete
 
             int add(journal_entry & to_add);
             int find(char matching_title[], journal_entry & found);
             int display(void);

             int append(list & source);

      private:
              node * head;
               node * tail;
};


Function that copies strings into entry char arrays:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
int journal_entry::copy_entry(const journal_entry & copy_from)
{
     if (!copy_from.title || !copy_from.notes)
     {
          return 0;
     }
     else
     {
          this->title = (char*) malloc(strlen(copy_from.title) *  sizeof(char)+1);
          strcpy(this->title, copy_from.title);
          this->notes = (char*) malloc(strlen(copy_from.notes) * sizeof(char)+1);
          strcpy(this->notes, copy_from.notes);
          return 1;
     }

}


main:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
int main()
{

    char title[SIZE], notes[SIZE];      //temporary for journals
    journal_entry entry;                //Start creating entries!
    list my_journal;



    do
    {
       cout <<"Please enter the title of your journal entry\n";
       cin.get(title, SIZE, '\n'); cin.ignore(SIZE,'\n');
       cout <<"Please enter your notes\n";
       cin.get(notes, SIZE, '\n'); cin.ignore(SIZE,'\n');

       //Now create the journal entry
       if (entry.create_entry(title, notes))
       {
          entry.display();
          if (! my_journal.add(entry))
              cerr << "We were unable to add it to the list...\n";
       }
     } while (again());

     //Now display all that we have
     cout <<"\n\nThe entire list contains the following:\n";
     if (!my_journal.display())
            cerr << "The list is empty...try again\n";



     return 0;
     
}






Why are you using malloc instead of new?
Anyway, you need to post a whole program that can be compiled and run.
A link to a zip file is handiest.
Drive link:

https://drive.google.com/file/d/12CZlzuc_6uNVByWtDEAlSnLFG0f-GlLR/view?usp=sharing

Some of the code is contained in a .o file that I don't have access to :(
it does not even matter in all cases if the destructors are correct.

look at this post: http://www.cplusplus.com/forum/general/267840/

the author there deleted his memory in his dtor and set it to null as is proper, and still managed to hose things up.

its deceptively simple: for every call to new, you need 1 call to delete.
The key is keeping track of that in your class automatically. When you chain up something like a linked list, a common destruction approach is with recursion, eg:
1
2
3
4
5
void deleteme(node* n)
{
     if(n) deleteme(n->next);  
     delete n;  //delete of null is safe.  
}


pass head to that and it will kill the whole list.
so if the list is 1-2-3-|| then it will recurse until null, then delete the null and pop, delete the 3 and pop, delete the 2 and pop, delete the 1 and pop. Set head = null after and its clean.

Is that what you want/need?
You can treat 'this' node as the 'head' for the above idea.
so what if you delete a node in the middle of the list and put the list back together after?
you need to make sure the node being deleted has its 'next' nulled out before you destroy it.
tmp = badnode;
toppartholder->next = badnode->next; //connect top and bottom
tmp-> next = nullptr;
delete tmp; //only this node will die.

or you can do this to wipe out from badnode forward, killing half your list or something:
toppartholder->next = null;
delete tmp; //takes everything with it if you used my above code.
typically the above lets you wipe the whole list safely, eg this will trigger if it is the dtor code, so in main at the end of main head suicides and takes everything under it down too.

I dunno what all you need to do to your class, but this is a way to do it.

and like anything, if you have pointers into the middle of the list external to the list and kill off the list, bad things happen there too.
Last edited on
Added this (It was actually in an earlier version, but I might not have saved the changes correctly:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

list::~list()
{
     delist(head);
}

void list::delist(node * node)
{
     if (node == nullptr)
          return;
     else
     {
          delist(node->next)
          delete node;
     }
}


Valgrind returns the same set of errors, though, and those are what I'm trying to track down. Am I correct in assuming that there's some issue with the fact that the entry objects have memory allocated using malloc, and the nodes of course are created with new?
malloc and new, delete and free do things differently internally and are NOT compatible.
if you free a pointer made with new or delete one made with malloc, you will have problems.
do not do that.
just stick to new and delete. You should generally not use malloc in c++ code unless doing something very low level and specialized, and even then, its really just to enable realloc at a level where you couldn't use a vector.

if you want to copy a string to a char array, you can use new and strcpy.
char * derpy = new char(str.length()+1);
strcpy(derpy, str.c_str());

copy entry is possibly NOT SAFE.
can you call it more than once on the same object?
if so, you leaked memory (eg is this-> title already had memory).
before you malloc it again, you should try to free it. Which again is fine if it were null by default (it should be, set ctor to do that if not already). If it was allocated before, you clear it and rebuild it. This is slow and wasteful, maybe wasting memory is better, and make title an array of size [500] or something that works for everything, and stop fooling with dynamic memory for it, or better, just use a c++ string? But the right way to do it if this is an exercise in using pointers is to free before malloc in a function that can alloc and can be called more than once. same for new delete ... if it can call new, and it can be called more than once, delete then new and it won't leak.
Last edited on
I already asked you if you were told to use malloc instead of new for some reason.
If you use malloc then you need to use free instead of delete.
But since the deallocation code seems to be in the supplied object file you need to use what the teacher used.
If it's a decent class then he should've used delete and therefore you should use new.
(This is all assuming you were told to use char arrays instead of std::string, of course.)
dutch- The instructions were "allocate memory for the arrays to be just the right size". I read that as use malloc to specify exactly how much memory this->title and this->notes should take up; I don't know of any way to do that with new?

Char arrays is specified, yes- this is partly an exercise with pointers.

jonnin- The memory leaks I encountered with valgrind were mostly problems with not allocating enough memory using malloc; possible that copy_entry is not being tested in a way that would cause the issue you describe, of course. Can I ask you for an example of how to structure the malloc/free statements correctly?

Edit: I should have read your reply more carefully, jonnin- I will try the new statement as given out! I would still like an example of how to structure a malloc statement in this, though.
Last edited on
The valgrind errors are due to you using malloc instead of new.
jonnin already showed you how to use new:

1
2
3
4
    this->title = new char[strlen(copy_from.title) + 1];
    strcpy(this->title, copy_from.title);
    this->notes = new char[strlen(copy_from.notes) + 1];
    strcpy(this->notes, copy_from.notes);

Yes, I focused on the 2nd part of his reply and initially didn't read the first part as carefully as I should have :(

Thank you both for your assistance! I hadn't realized that you could specify size allocation with new, which left me trying to troubleshoot the destructors:D
You had it correct as far as I can tell. I feel like I missed something (like a delete of a malloc variable) but your actual malloc calls look legit (though sizeof(char) ==1 and is kinda redundant).

remember this about new and delete:
if you new, you delete. If you new[] you delete[];
that is
this->title = new char[strlen(copy_from.title) + 1];
MUST BE DELETED LIKE THIS:
delete[] this->title;
and not this:
delete this->title;

I forget what it does if you missed the [] on the delete, but I am sure valgrind will blow a gasket over it and the program may do weirdness too, or it may quietly leak.
Last edited on
He needs to use new[] and to NOT define a dtor or the delist method since those are already defined in the supplied object file.
I was trying to answer:
Can I ask you for an example of how to structure the malloc/free statements correctly?

I agree, for fixing it, but he wanted to C it.
Update:
I edited the copy_entry function as follows:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
int journal_entry::copy_entry(const journal_entry & copy_from)
{
     if (!copy_from.title || !copy_from.notes)
     {
          return 0;
     }
     else
     {
          this->title = new char[8000]; //this originally read new char[strlen(copy_from.title) + 1)- I changed it 
          strcpy(this->title, copy_from.title); //while troubleshooting; made no diff
          this->notes = new char[8000];
          strcpy(this->notes, copy_from.notes);
          return 1;
     }

}


This resulted in these errors in valgrind, which came up immediately after this code executes:

==4530== Invalid write of size 1
==4530== at 0x483E0AC: strcpy (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==4530== by 0x1092B2: journal_entry::copy_entry(journal_entry const&) (cs163_entry.cpp:16)
==4530== by 0x10980F: list::add(journal_entry&) (cs163_list.cpp:14)
==4530== by 0x1095A6: main (cs163_lab2.cpp:34)
==4530== Address 0x4dba601 is 0 bytes after a block of size 1 alloc'd
==4530== at 0x483AE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==4530== by 0x10927B: journal_entry::copy_entry(journal_entry const&) (cs163_entry.cpp:15)
==4530== by 0x10980F: list::add(journal_entry&) (cs163_list.cpp:14)
==4530== by 0x1095A6: main (cs163_lab2.cpp:34)
==4530==
==4530== Invalid write of size 1
==4530== at 0x483E0BE: strcpy (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==4530== by 0x1092B2: journal_entry::copy_entry(journal_entry const&) (cs163_entry.cpp:16)
==4530== by 0x10980F: list::add(journal_entry&) (cs163_list.cpp:14)
==4530== by 0x1095A6: main (cs163_lab2.cpp:34)
==4530== Address 0x4dba607 is 6 bytes after a block of size 1 alloc'd
==4530== at 0x483AE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==4530== by 0x10927B: journal_entry::copy_entry(journal_entry const&) (cs163_entry.cpp:15)
==4530== by 0x10980F: list::add(journal_entry&) (cs163_list.cpp:14)
==4530== by 0x1095A6: main (cs163_lab2.cpp:34)

These are the same errors I got rid of by using malloc to allocate the memory- any advice on what I did wrong would be much appreciated!

I can see that the allocated memory is too short for what is being written- the thing that confuses me is that no matter how I change the allocated amount (Most recently by simply specifying 8000 rather than using strlen), the errors remain the same. So I think that I have a fundamental misunderstanding of what is happening here with the memory allocation.

Version: g++ (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008
Last edited on
> Some of the code is contained in a .o file that I don't have access to :(
not sure how bad can that be
I mean, it sounds too prone to compatibility errors (and ask for too much trust)

however, could build and got your delete/malloc mismatch
doing your last edit made all the issues disappear, so can't reproduce your error. (¿what's your input?)


> I can see that the allocated memory is too short for what is being written
the error says «0 bytes after a block of size 1 alloc'd»
so you have
1
2
char *ptr = new char;
ptr[1] = 42;


«6 bytes after a block of size 1 alloc'd»
again, out of bounds, but you have new char (or perhaps new char[1];)

you are allocating space for only one element.


https://heeris.id.au/2016/valgrind-gdb/
you may integrate your debugging with gdb



Edit: ¿perhaps you have confused brackets and parenthesis? like new char(8000) creates only a single character
Last edited on
I don't have access to the .o file either, unfortunately.

My initial input is "title 1" then "notes 1" then "title 2" and "notes 2" and so on (usually just 3-4 for testing purposes).

For the search input, usually "title 2" or "title 3".

By "last edit" do you mean:
 
this->title = new char[8000];

?

I got that much out of the errors (valgrind is delightfully clear!), but the above code clearly seems to be allocating enough space for 8000 chars. So if that is what the valgrind errors are referencing, I am totally confused. Otherwise, I need to figure out what they are referencing.

Just to be sure, went back to:

 
this->title = (char*) malloc(strlen(copy_from.title) *  sizeof(char)+1);


And those errors disappeared. Which tells me that the malloc statement is working, but new is somehow not?

Doublechecked my brackets, as well :)

I feel like this is either something stupidly simple, or a problem that requires information hidden within the .o file to resolve.

still can't reproduce
this input:
title 1
notes 1
Y
title 2
notes 2
Y
title 3
notes 3
Y
title 4
notes 4
N
title 3
which gives
No match found.
==4851==
==4851== HEAP SUMMARY:
==4851==     in use at exit: 0 bytes in 0 blocks
==4851==   total heap usage: 23 allocs, 23 frees, 141,984 bytes allocated
==4851==
==4851== All heap blocks were freed -- no leaks are possible
==4851==
==4851== For lists of detected and suppressed errors, rerun with: -s
==4851== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
so all good.


http://www.cplusplus.com/forum/beginner/267879/#msg1152512
> Added this (It was actually in an earlier version, but I might not have saved the changes correctly:
¿do you still have that?
supplied.o provides the list destructor (may check with nm), so you are redefining it there,
which should have caused a failure to build.

if you can build still, then perhaps the supplied.o is not compatible enough and the resulting program is kind of undefined.
using malloc() is incorrect
using new/new[] is also incorrect

use the inferface of the class
entry.create_entry(title, notes)
so
1
2
3
int journal_entry::copy_entry(const journal_entry & copy_from){
   this->create_entry(copy_from.title, copy_from.notes);
}
ne555- Um. Duh. My face is amazingly red right now. Thank you! I felt like there was something super obvious that I was missing :D

Still strange that you weren't able to reproduce the valgrind errors, though. Compiler or OS difference, maybe?
i find strange that you've got those errors
g++ (GCC) 9.2.0
5.3.7-arch1-1-ARCH

(I've also used another PC, but don't have access anymore)


wait... ¡¿you have a newer version on Ubuntu?! ¿did I have a reason to avoid updates?
Pages: 12