Freeing linked list crashes when on last node

Question

This is a continuation of a problem I posted yesterday, which I thought was solved, but turns out another problem has been encountered with the way I iterate through the loop and its exit condition. I felt a new question thread might be more appropriate.

The following freeAllListMembers() function seems to be working, up until the last iteration of the loop because it is trying to free "temp" but temp has already been freed, what method can I use to exit this loop and keep it from running once more, everything I have tried doesn't seem to be working.

thanks for any insight

int main() {

    struct node *head = NULL;
    createList(&head);

    //do stuff with list

    freeAllListMembers(&head);

    return 0;
}

int createList(struct node **head) {
    struct node *newNode= NULL;

    for(int I = 0; I < 100; I++) 
    {
         struct node *node = (struct node*)malloc(sizeof(struct node));

         node->data = someData;

         node->next = NULL;

         //if we havent created an initial start node, create it
         if (*head == NULL)
         {
             *head = node;
         }
         //otherwise, navigate to the end of the list to add a new node
         else
         {
             newNode = *head;
             while (newNode->next != NULL){
                 newNode = newNode->next;
             }
             newNode->next = node;
         }
    }
    return 0;
}

void freeAllListMembers(struct node **head){ 
    struct node *temp;
    while (*head != NULL) {
        temp = *head;
        *head = (*head)->next;
        free(temp);
    }
    return;  
}

I think your loop should look like while(head != NULL) instead of while(*head != NULL) — merl, Oct 13 '16 at 16:57
@merl Note that the OP is passing a *pointer* to the actual head. — Some programmer dude, Oct 13 '16 at 16:59
Fill in `createList` some. Your chain may not terminate right — infixed, Oct 13 '16 at 16:59
@infixed , this is interesting, it would be too much for me to abstract out createList, but I will look there, if you think the loop should be working. thanks — skevthedev, Oct 13 '16 at 17:04
Mostly I could see a problem in how you inserted a node. if your first inserted one didn't get it's `next` field initialized right. — infixed, Oct 13 '16 at 17:09
Show how you create the list. The problem is most likely there. `freeAllListMembers` looks more or less OK to me. — Jabberwocky, Oct 13 '16 at 17:17
@MichaelWalz, I abstracted my code out a little bit and edited my original post, I believe at one point this did work, but in an attempt for me to try and optimize something went wrong. thanks for the help — skevthedev, Oct 13 '16 at 17:44
@infixed edited original post with elaborated createList. thanks for any help — skevthedev, Oct 13 '16 at 18:30
I cut and pasted it, cleaned up some minor things, like `#include ` and where SO changed a i to an I. and it compiled and ran OK in gdb. — infixed, Oct 13 '16 at 19:15

score 0 · Answer 1 · answered Oct 13 '16 at 19:19

Cleaning up the example given to remove some small human errors, the program compiles OK, and I can step through OK in GDB. It basically ran after it compiled without errors

#include <malloc.h>

#define someData  12345

struct node
{
        int data;
        struct node* next;
};

int createList(struct node **head )
{
    struct node *newNode= NULL;

    int i;
    for( i = 0; i < 100; i++)
    {
         struct node *node = (struct node*)malloc(sizeof(struct node));

         node->data = someData;

         node->next = NULL;

         //if we havent created an initial start node, create it
         if (*head == NULL)
         {
             *head = node;
         }
         //otherwise, navigate to the end of the list to add a new node
         else
         {
             newNode = *head;
             while (newNode->next != NULL){
                 newNode = newNode->next;
             }
             newNode->next = node;
         }
    }
    return 0;
}

void freeAllListMembers(struct node **head)
{
    struct node *temp;
    while (*head != NULL) {
        temp = *head;
        *head = (*head)->next;
        free(temp);
    }
    return;
}


int main()
 {

    struct node *head = NULL;
    createList(&head);

    //do stuff with list

    freeAllListMembers(&head);

    return 0;
}

I can only guess that you lost whatever your bug was in the 'abstraction' process

thanks for looking into this. yeah, when copying things over some things got lost, but I was still experiencing the issue. I did find a resolution, but to me it seems wildly inefficient, and not something I really like. In order for me to get this to work I need to move struct node *newNode= NULL; from being initialized at the top of createList(); to being initialized in the for loop, right before the assignment conditions...any suggestions on improving this would be great. thanks! — skevthedev, Oct 13 '16 at 19:49
in the given example `newNode` is really only used as a list pointer to find then end of the linked list. I would have declared it inside that else clause like `struct node *newNode= *head;` and dropped the declaration at the start of `createList` — infixed, Oct 13 '16 at 20:05
I wouldn't even call it `newNode` because it is more a pointer to find the end of the linked list. The actual new stuff is pointed to by `node`. Perhaps there is similar ambiguity in your un-abstracted program and you mixed up things — infixed, Oct 13 '16 at 20:14

Freeing linked list crashes when on last node

1 Answers1