Malloc problem in linked list

[Fr33t]

I don't normally post in these kinds of forums and prefer to just google my way through problems, but after 3 hours of trying to solve this, I am beat.

I am working on a program that uses a linked list and can add/delete/print out nodes (orbs in this program) but I am stuck on adding them. I can add the first node just fine, but when I go to add another, the program crashes when I try to malloc space to the new node.

I've posted where I'm at so far below. Note that I removed the other functions for the sake of saving space and I cannot even begin to work on them until I can actually get nodes into the list. Any help will be greatly appreciated, thank you.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

struct list
{
    struct orb* head;
};

struct orb
{
    char*       fname;
    char*       lname;
    int         mana;
    struct orb  *next;
};

struct list* create_list(void);
void insert(struct list* linked_list, int new_mana, char* new_fname,
                char* new_lname, int index);
void delete_list(struct list* linked_list);

// Creates a new empty linked list
struct list* create_list(void)
{
    struct list* new_list;
    
    new_list = malloc(sizeof(struct list));
    new_list->head = NULL;
    
    return new_list;
}

// Insert a new orb
void insert(struct list* linked_list, int new_mana, char* new_fname,
                char* new_lname, int index)
{
    struct orb* new_orb = malloc(sizeof *new_orb);
    struct orb* crnt;
    int i;    
      
    new_orb->mana = new_mana;
    strcpy(new_orb->fname, new_fname);
    strcpy(new_orb->lname, new_lname);

    if(linked_list->head == NULL || index == 0)
    {
        new_orb->next = linked_list->head;
        linked_list->head = new_orb;
        return;
    }
    
    crnt = linked_list->head;
    
    for(i = 0; crnt->next != NULL && i != index - 1; i++)
    {
        crnt = crnt->next;
    }
    
    new_orb->next = crnt->next;
    crnt->next = new_orb;
}

void delete_list(struct list* linked_list)
{
    struct orb* crnt;
    struct orb* temp;
    
    crnt = linked_list->head;
    
    while(crnt != NULL)
    {
        temp = crnt;
        crnt = crnt->next;
        free(temp);   
    }
    free(linked_list);
}

int main(void)
{
    struct list* linked_list;
    linked_list = create_list();
    
    int choice, index, tmana;
    char tfname[21], tlname[21];
    
    while(choice != 6)
    {
        printf("Here are your options\n");
        printf("1. Insert orb\n");
        printf("6. Quit\n");
        printf("Enter your choice:\n");
        
        scanf("%d", &choice);
        
        switch(choice)
        {
            case 1:
                printf("What is the first name?\n");
                scanf("%s", &tfname);
                printf("What is the last name?\n");
                scanf("%s", &tlname);
                printf("What is the mana of this orb?\n");
                scanf("%d", &tmana);
                printf("Where should this be inserted?\n");
                scanf("%d", &index);
                insert(linked_list, tmana, tfname, tlname, index);
                break;
        }
    }

    delete_list(linked_list);

    system("PAUSE");
    return 0; 
}

[Banfa]

Line 43 and 44, you copy to the char pointers fname and lname but you have not allocated any data for those pointers to point to. They are uninitialised.

[Fr33t]

Oh, I thought my one malloc line took care of the whole struct. How do I allocate data for those pointers as well?

[Banfa]

1 malloc does take care of the whole struct, however those pointers point to something that is not part of the struct they are separate things.

Anyway another call to malloc for each pointer should do the trick. Remember if using strlen to get the sizes of the strings to copy you need to add 1 for the null terminator.

If this is implemented on a POSIX compliant platform and you aren't too fussed about portability you could use strdup.

Which ever method you use remember that those strings will need to be explicitly deallocated again before freeing the structure when releasing the node memory.

[Fr33t]

Alrighty, just updated the code with the extra calls to malloc added in and its running just fine now. Thanks a million Banfa!

Here's the update if anyone else is having this problem (21 is the max size of the names):

new_orb->mana = malloc(sizeof(int));
new_orb->mana = new_mana;

new_orb->fname = malloc(sizeof(char[21]));
strcpy(new_orb->fname, new_fname);

new_orb->lname = malloc(sizeof(char[21]));
strcpy(new_orb->lname, new_lname);

[Banfa]

I hope that this new_orb->mana = malloc(sizeof(int)); is a typo because its wrong and its not what is in your original listing.

If all names are fixed to 21 then you should #define a symbol to that value (21) and use the symbol throughout your code. Then if for instance the value needs to change you have only to change a single location rather than change every place in the code. A issue that gets more serious the more code and code files you have.

Finally using scanf("%s", &tlname); where the size of tlname is limited is bad practice because it is easy for the user to input more than 20 characters and cause a buffer overflow. Instead either use something like fgets or use something like this scanf("%20s", &tlname); which limits the amount of data put into the target.