C Linked List - Linking next Linked List Node

Question

I have an application which takes multiple command line arguments, amongst the arguments it accepts a file of target hosts which certain actions will be performed against.

173.194.40.225
157.55.152.112
200.49.185.230
195.95.178.226
98.158.27.203

On each line is an IP address of a target, I traverse through the file using the following function;

// Open file read line by line to Linked List
ListNode* file(char *file, char *option) 
{

    FILE *ptr_file;
    char buffer[1000];
    char *destination;
    ListNode *linelist;

    // Create an empty line list
    linelist = NULL; 

    // Open a file
    if(strcmp(option, "r") == 0)
    {
        ptr_file = fopen(file, option);

        if(!ptr_file)
        {
            printf("Can\'t open file\n\n");
            exit(0);
        }
    }
    else
    {
        printf("File operation not implemented\n\n");
        exit(0);
    }

    // Traverse through the file 
    while (fgets(buffer, LINE_MAX, ptr_file) != NULL)
    {
        printf("Line: %s\n", buffer);

        // Add line to Linked List
        linelist = addtoList(linelist, buffer);

        printf("---------------------------------------\n\n");
    }

    printList(linelist);

    // Close the file
    fclose(ptr_file);

    // Return a pointer to linelist
    return linelist;
}

The function is supposed to pass back a pointer to a Linked List of target IP addresses that can be used in a separate socket function. The Linked List has been designed as follows;

// Define node structure
struct listnode 
{
    char *data;
    struct listnode *next;
};

// Define node as a type
typedef struct listnode ListNode;

// Add to List
ListNode* addtoList( ListNode *List, char *data );

After the file() function traverses the passed file using a while loop and fgets() to load each line into a buffer, the buffer is passed to a addtoList() function which accepts two variables, a pointer to a Linked List and a pointer to a char which in reality is simply each line of the passed file. The function is meant to add to the end of the passed list.

The function works as follows;

1) Creates a temporary pointer of type ListNode & allocates memory using malloc()

2) Check malloc() doesn't return NULL, if it does then inform user, else fill the temporary ListNode with passed data

3) If the ListNode passed to the function is NULL, simply assign the temporary ListNode to the passed NULL ListNode and return the list

4) If the ListNode passed to the function is not NULL, and the passed ListNode next value is NULL, assign the temporary ListNode to the ListNode next value and return the list

5) If the ListNode passed to the function is not NULL, and the passed ListNode next value is not NULL, create a copy of the pointer to the beginning of the list. Traverse the list using a while loop until we get to the last ListNode which is NULL. Assign the temporary ListNode to the NULL ListNode

The function is as follows;

// Add to List
ListNode* addtoList(ListNode *List, char *data)
{

    printf("Adding: %s\n", data);

    // Create pointer to allocated memory the size of a ListNode
    ListNode* temp = (ListNode*)malloc( sizeof( ListNode ) );

    // Check malloc didn't return NULL
    if(temp == NULL) 
    {
        printf( "Can\'t allocate memory for temp, failed to allocate the requested block of memory, a null pointer is returned\n\n" ); 
        exit(0);
    }
    else
    {
        printf("Memory allocated for temp, filling allocated memory\n\n");

        // Fill the allocated memory where data is a pointer to data
        temp->data = data;
        temp->next = NULL;

        printf("Allocated memory for temp filled with data\n\n");

    }

    printf("temp->data = %s\n\n", temp->data);

    int size = countList(List);
    printf("List size: %i\n\n", size);

    if(List == NULL)
    {
        // If computer can't allocate memory let us know
        printf( "List is empty\n\n" ); 
        List = temp;
        printf( "List is now temp\n\n" ); 
        return List;
    }
    else
    if(List != NULL && List->next == NULL) 
    {
        printf("List isn\'t empty and List->next is NULL\n\n");

        List->next = temp;
        return List;
    }
    else
    if(List != NULL && List->next != NULL) 
    {
        printf("List isn\'t empty and List->next is not NULL\n\n");

        ListNode* Head = List;

        while(Head != NULL)
        {
            printf("List->next data: %s List->next pointer: %p\n\n", Head->data, Head->next);
            Head = Head->next;
        }

        if(Head == NULL)
        {

            printf("Head equals null\n");
            //problem here
            Head->next = temp;

        }

        return List;
    }

}

My problem sits near the very end of the addtoList() function in the if(Head == NULL) conditional statement, the assignment of the temporary ListNode to the Head->next;

if(Head == NULL)
{     
    printf("Head equals null\n");
    //problem here
    Head->next = temp;      
}

The output of the application is as follows;

######################################

Line: 173.194.40.225

Adding: 173.194.40.225

Memory allocated for temp, filling allocated memory

Allocated memory for temp filled with data

temp->data = 173.194.40.225


List size: 1

List is empty

List is now temp

---------------------------------------

Line: 157.55.152.112

Adding: 157.55.152.112

Memory allocated for temp, filling allocated memory

Allocated memory for temp filled with data

temp->data = 157.55.152.112


List size: 2

List isn't empty and List->next is NULL

---------------------------------------

Line: 200.49.185.230

Adding: 200.49.185.230

Memory allocated for temp, filling allocated memory

Allocated memory for temp filled with data

temp->data = 200.49.185.230


List size: 3

List isn't empty and List->next is not NULL

List->next data: 200.49.185.230
 List->next pointer: 0x8592180

List->next data: 200.49.185.230
 List->next pointer: (nil)

Head equals null
Segmentation fault

The problem I'm having is with the assignment of the temporary ListNode into the NULL ListNode of the listnode structure.

Any help is greatly appreciated...

Edit

So my addtoList() function is now as follows after molbdnilo's & Diego's recommendations;

// Add to end of List
ListNode* addtoList(ListNode *List, char *data)
{

    char* data_ptr = data;    // Create a pointer to passed data
    char* bkup_copy = NULL;   // Create a null pointer for backing up passed data

    bkup_copy = copyString(data_ptr); // Create a backup of data

    // Create pointer to allocated memory the size of a ListNode
    ListNode* temp = (ListNode*)malloc( sizeof( ListNode ) );

    // Check malloc didn't return NULL
    if(temp == NULL) 
    {
        printf("Can\'t allocate memory for temp, failed to allocate the requested block of memory, a null pointer returned\n\n" ); 
        exit(0);
    }
    else
    {
        // Fill the allocated memory where data is a pointer to data
        temp->data = bkup_copy;
        temp->next = NULL;       
    }

    if(List == NULL)
    {
        List = temp;
        return List;
    }
    else
    if(List != NULL && List->next == NULL) 
    {
        List->next = temp;
        return List;
    }
    else
    if (List != NULL && List->next != NULL) 
    {

        // Create a copy of pointer to passed list
        ListNode* Head = List;

        // Traverse through the list until last item
        while(Head->next != NULL)
        {
            Head = Head->next;
        }

        // Assign temp to the last list item next
        Head->next = temp;

        return List;
    }
    else
    {
        printf("Unknown state of List\n\n");
        exit(0);
    }

}

With a new copyString() function which simply returns a copy of a string as follows;

char* copyString(char* data)
{

    char* data_ptr = data;    // Create a pointer to passed data
    int orig_str_size = 0;    // Create an int to hold passed data size
    char* bkup_copy = NULL;   // Create a null pointer to backup passed data
    int bkup_index = 0;       // Create a index variable for traversal of passed data
    int length;

    // Count the number of characters in data_ptr
    while (*data_ptr++ != '\0')
    { 
        orig_str_size++; 
    }

    // Dynamically allocate space for a backup copy of data
    bkup_copy = (char*)malloc((orig_str_size+1) * sizeof(char));

    // Check malloc didn't return NULL
    if(bkup_copy == NULL) 
    {
        printf("Can\'t allocate memory for bkup_copy, failed to allocate the requested block of memory, a null pointer returned\n\n" ); 
        exit(0);
    }
    else // Copy data to separate allocated memory
    {
        // Place the '\0' character at the end of the backup string.
        bkup_copy[orig_str_size] = '\0';

        // Assign the pointer to data to the first pointer position in data
        data_ptr = &data[0]; 

        // The backup string is not the '\0' character copy data to bkup_copy
        while (*data_ptr != '\0'){ bkup_copy[bkup_index++] = *data_ptr++; }
    }

    return bkup_copy;

}

It works as expected now...if anyone can see any problems with that code please let me know so I can fix it now...

Answer 1

The problem is that you can't dereference NULL, and you have made very sure that Head is NULL.

You need to stop the traversal earlier, so you can link the new node to the last node in the list.

    // Find the last node - the node with NULL as its 'next'
    while (Head->next != NULL)
    {
        Head = Head->next;
    }
    // 'Head' is now the last node.
    // Replace the NULL at the end of the list with the new node.
    Head->next = temp;