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

Improve this question
2
  • 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 (minimal reproducible example). Commented Mar 19, 2016 at 5:17
  • do not cast the return value of malloc. You're not using C++ compiler, since you're using new as a variable name. Commented Mar 19, 2016 at 6:40

3 Answers 3

Reset to default
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;
}
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

@iharob Reasonable. Answer improved and thanks for pointing that out!
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.
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);.

Improve this answer

Comments

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.

Improve this answer

1 Comment

@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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.