Memory Leak in C

Question

Hello,

I am a C beginner, and I am writing a very simple linked-list. I am wondering if there would be a memory leak in the following code:

void removeListEntry(struct tableEntry *symp, struct tableEntry *previous) {
  if (symp->next = 0){
    symbolList.tail = previous;
    previous->next =0;
  } else {
    previous->next = symp->next;
    symp->next = 0;
  }
}

I am pretty sure if the pointer symp is not stored in another variable, there's no way of accessing the list entry that was pointed by the pointer, thus I will have a memory leak. In C, we use malloc() function to allocate memory space for a data structure, and I remember using new keyword to "dynamically" allocate memory in C++. What are the differences between allocating memory using malloc() and using new? Is there indeed a memory leak in my code?

Answer 1

You need to call free() once you've finished using memory alloc'd by malloc().

Answer 2

For differences between malloc() and new, see this article.

Answer 3

if (symp->next = 0) {

This if-"condition" is an assignment, setting symp->next to 0. If a pointer to another object was stored in symp->next, that object is lost and the objects memory will not be freed.

For a comparision you need to use == instead:

if (symp->next == 0) {

or do it without an explicit comparision:

if (!symp->next) {

In the else case you remove symp from the list (assuming previous actually contains the element before symp), but you don't free it's memory. This might be a memory leak, but it depends on the code calling the function: That code might still free symp or do something else with the removed element, or it might just forget about it and leak it's memory.

Answer 4

I am curious about what is supposed to be happening in your code:

if (symp->next = 0){
  symbolList.tail = previous;
  previous->next =0;
} else {
  previous->next = symp->next;
  symp->next = 0;
}

When would symb->next not be zero? You aren't doing anything if symb is empty as the head node would also be null.

The confusing part is that you are appending previous to symb in the first if (which should always be the case) but in the next one you are appending symb to previous. What rationale is there for this second one, and in what case will it ever happen?

As others have mentioned, if you allocate memory you need to free it, else you have a memory leak, as there is no garbage collector in C/C++, so every node that is going to be freed needs to be deallocated.

The symb->next = 0 is probably just a typo, as was pointed out, as that will always be true, and is a frequent bug. What I started to do to help catch this is to do: if (0 == symb->next), so if you did 0=symb->next then you will get a compiler error.

UPDATE:

As was pointed out in a comment, this function will always go to the 'else' clause, which may be expected behavior, actually.

Answer 5

As pointed out above if(symp->next = 0) is an assignment operation and the if statement will evaluate to false. Because of this you will not only lose the pointer to the next table entry after symp, but you also lose the previous->next pointer (which I assume points to symp anyway?).

Just a matter of style, but personally I would rewrite the function to be more like this:

void removeNextListEntry(struct tableEntry *previous) {
  struct tableEntry *dummy = previous->next;
  if (dummy->next == 0){
    symbolList.tail = previous;
    previous->next =0;
  } else {
    previous->next = dummy->next;
  }
  free(dummy);
}