1

This is my function to add a new node using double pointer 

void insertBefore(node_t **first)
{  
    node_t *new = NULL;  
    new = (node_t *) malloc(sizeof(node_t));
    new->next = *first;
    *first = new;
    free(new);

}

If I use it once, everything seems to work fine. However, if I use it again, my linked-list is messed up. I have the output image below (same thing happened to my function to insert node to any position). I tried to put some very specific part of the code, so if you suspect that I must have been doing something wrong at other parts, please tell me. Any idea what did I do wrong?

The output image

Iharob Al Asimi
  • 52,653
  • 6
  • 59
  • 97
CSDD
  • 339
  • 2
  • 14
  • 1
    Usually, your `node_t` structure will have more than one element; your code would normally have one or more other function parameters which would be used to initialize the other fields in the structure. At the least, you should normally make sure all the other fields are initialized to a known value (`""` or `0` or `NULL` are often good choices in the absence of any better value, but it depends on what else the structure contains and what it describes). I'll assume you just did a thorough job of creating an MCVE ([MCVE]). – Jonathan Leffler Mar 19 '16 at 05:17
  • [do not cast the return value of malloc](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). You're not using C++ compiler, since you're using `new` as a variable name. – Antti Haapala -- Слава Україні Mar 19 '16 at 06:40

3 Answers3

4

Just remove free(new);. Since new and *first have same value, freeing new also frees *first.

Refined code:

void insertBefore(node_t **first)
{  
    node_t *new = malloc(sizeof(node_t)); //don't cast
    if(!new)
    {
        fputs("Don't have enough memory", stderr);
        return;
    }
    new -> next = *first;
    *first = new;
}
nalzok
  • 14,965
  • 21
  • 72
  • 139
  • @iharob Reasonable. Answer improved and thanks for pointing that out! – nalzok Mar 19 '16 at 05:09
  • Now I upvoted, because you actually helped the OP to fix the issue and make the function usable, as in his OP the funcion would not do what it seems to be designed for. – Iharob Al Asimi Mar 19 '16 at 05:13
2

You free the node you had just allocated; whenever you attempt to use it, undefined behavior will occur because *first becomes a Dangling Pointer. The address stored in the pointer is no longer valid after the function free() returns.

Your code needs many improvements

  1. You don't need to initialize new to NULL; probably no problem as this would be optimized.

  2. You don't need to cast malloc() or in general void * to any other pointer type.

  3. You don't check that malloc() was successful; you should check that new is not being assigned NULL which would indicate an error.

  4. You MUST NOT free() the newly allocated pointer; you should free() it when you don't need it anymore and not immediately after allocating it.

  5. You really should improve your code formatting. It was completely unreadable in the original post.

Iharob Al Asimi
  • 52,653
  • 6
  • 59
  • 97
  • 1
    @CSDD You pick an answer and click the check mark below the score if you think it is the one that helped you more or gives to your criterion the best explanation. – Iharob Al Asimi Mar 19 '16 at 05:03
2

It because you are freeing the memory.

free(new);

You are just assigning the value to first.

*first=new;

Now first and new will point to same memory position. Then you are freeing that memory. Remove the line free(new);.

Karthikeyan.R.S
  • 3,991
  • 1
  • 19
  • 31