Linked List Infinite Loop

Question

Trying to create a program to count the number of comparisons made with strcmp while moving the current node to head. Having some problems with the algorithm though .. it "works" sometimes, and at other times gives me an infiniteloop. Tried lldb for hours now, and put printf messages literally every 2 lines of code, but I have no idea how to pinpoint the problem. I presume it's somewhere in the algorithm but I can't see anything wrong with it.

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

typedef struct Node2
{
    char* word;
    struct Node2 *next;
} Node2;

Node2* head = NULL;
Node2* now = NULL;
int uniqueWords = 0;
int totalMTFRComparisons = 0;


Node2* iNode(char* word)
{   
    Node2* ptr = malloc(sizeof(Node2));
    Node2* tmp = head;
    Node2* prev = head;

    while (tmp)
    {
        totalMTFRComparisons++;
        printf("Current word: %s", tmp->word);
        if (strcmp(tmp->word, word) == 0)
        {
            prev->next = tmp->next;
            tmp->next = head;
            return tmp;
        }
        prev = tmp;
        tmp = tmp->next;
        printf("tmp incremented.\n");
    }

    ptr->word = malloc(strlen(word) + 1);
    strcpy(ptr->word, word);
    ptr->next = NULL;
    if (head == NULL)
    {
        head = now = ptr;
        return ptr;
    }
    else
    {
        ptr->next = head;
        head = ptr;
    }
    return ptr;
}

char* getString()
{
    static char buffer[128];
    while (fgets(buffer, 128, stdin) != NULL)
    {
        iNode(buffer);
    }
    return buffer;
}

int main()
{
    getString();
    printf("Total words: %d, total MTFR comparisons: %d\n", uniqueWords, totalMTFRComparisons);

    Node2* ptr2 = head;
    Node2* tmp2 = head;
    while (ptr2 != NULL)
    {
        tmp2 = ptr2->next;
        free(ptr2->word);
        free(ptr2);
        ptr2 = tmp2;
    }
}

My console is just spammed with: tmp incremented. But this doesn't always happen - it only happens sometimes, so I have no idea what's causing it.

Example input & output: http://pastebin.com/QfuCm7gt

Answer 1

You're allocating too little memory for your strings:

ptr->word = malloc(strlen(word));
strcpy(ptr->word, word);

You need to allocate strlen(word) + 1 bytes to allow for the null terminator.

When you write beyond the allocated space, you invoke undefined behaviour. Unfortunately for you, that can mean it sometimes appears to work OK — it is a valid response to undefined behaviour to seem to work as expected except when it suits the system to change its mind and behave aberrantly.

Consider whether you can use the strdup() function. If not, consider whether you should write and use your own version of it. Don't forget to check whether the memory allocation was successful.

(Until you fix the undefined behaviour, there's really no point in wondering what else is wrong, if there is anything else that's wrong.)

---

I wrote this code to see if I could simulate your problem. I can't:

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

typedef struct Node
{
    char *word;
    struct Node *next;
} Node;

int totalMTFRComparisons = 0;

Node* head = NULL;
static Node* iNode(char* word)
{   
    Node* ptr = malloc(sizeof(Node));
    Node* tmp = head;
    Node* prev = head;

    while (tmp)
    {
        totalMTFRComparisons++;
        if (strcmp(tmp->word, word) == 0)
        {
            prev->next = tmp->next;
            tmp->next = head;
            //tmp->next = prev;

/* JL: Still a memory leak here. */
/* Either free(ptr) or don't allocate until after the loop */

            return tmp;
        }
        prev = tmp;
        tmp = tmp->next;
        printf("tmp incremented.\n");
    }
    ptr->word = malloc(strlen(word) + 1);
    strcpy(ptr->word, word);
    ptr->next = NULL;
    if (head == NULL)
    {
        head = ptr;
        return ptr;
    }
    else
    {
        ptr->next = head;
        head = ptr;
    }
    return ptr;
}

static void dump_list(Node *node)
{
    while (node != 0)
    {
        printf("Node word: [[%s]]\n", node->word);
        node = node->next;
    }
}

static void free_list(Node *node)
{
    printf("Freeing list\n");
    while (node != 0)
    {
        Node *next = node->next;
        printf("Freeing: [[%s]]\n", node->word);
        free(node->word);
        free(node);
        node = next;
    }
}

int main(void)
{
    char line[4096];

    while (fgets(line, sizeof(line), stdin) != 0)
    {
        printf("Line: [[%s]]\n", line);
        char *ptr = line;
        char *tok;
        while ((tok = strtok(ptr, "\n\t ")) != 0)
        {
            printf("Word: [[%s]]\n", tok);
            iNode(tok);
            ptr = NULL;
        }
        dump_list(head);
    }
    free_list(head);
    return 0;
}

That's more or less an MCVE. The dump and free functions simply allow me to ensure I can see what's in the list and release all the memory.

Apart from fixing the memory overwriting problem, I only provided a definition for the Node type, put static in front of the function (to avoid one of the compilation warnings I'd otherwise get), and added the two support functions and main(); I didn't change your code otherwise. (Incidentally, the only complaint on the first compilation was about cur2 which I'd noticed but forgot to delete — that's very good; few programs, even ones that I write, get through quite that cleanly.)

When run, I typed:

abc def ghi
mno pqr stuvwxyz humongously-and-tempestuously-but-neither-abstemiously-nor-facetiously-long word!

and I got the output:

abc def ghi
Line: [[abc def ghi
]]
Word: [[abc]]
Word: [[def]]
tmp incremented.
Word: [[ghi]]
tmp incremented.
tmp incremented.
Node word: [[ghi]]
Node word: [[def]]
Node word: [[abc]]
mno pqr stuvwxyz humongously-and-tempestuously-but-neither-abstemiously-nor-facetiously-long word!
Line: [[mno pqr stuvwxyz humongously-and-tempestuously-but-neither-abstemiously-nor-facetiously-long word!
]]
Word: [[mno]]
tmp incremented.
tmp incremented.
tmp incremented.
Word: [[pqr]]
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
Word: [[stuvwxyz]]
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
Word: [[humongously-and-tempestuously-but-neither-abstemiously-nor-facetiously-long]]
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
Word: [[word!]]
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
tmp incremented.
Node word: [[word!]]
Node word: [[humongously-and-tempestuously-but-neither-abstemiously-nor-facetiously-long]]
Node word: [[stuvwxyz]]
Node word: [[pqr]]
Node word: [[mno]]
Node word: [[ghi]]
Node word: [[def]]
Node word: [[abc]]
Freeing list
Freeing: [[word!]]
Freeing: [[humongously-and-tempestuously-but-neither-abstemiously-nor-facetiously-long]]
Freeing: [[stuvwxyz]]
Freeing: [[pqr]]
Freeing: [[mno]]
Freeing: [[ghi]]
Freeing: [[def]]
Freeing: [[abc]]

When run under Valgrind, I got some curious outputs, but that's because I've upgraded the o/s (Mac OS X Yosemite to El Capitan) without updating the suppressions. The leaks were all in the system code, not in this code, AFAICT.

---

Post-Chat

One of the features of the code is that if the word is entered twice, the word should be moved to the front of the list. My testing was on unique sets of words. The problem was in the handling of the duplicate of the first word. A fix that seems to be robust is in this code:

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


typedef struct Node2
{
    char* word;
    struct Node2 *next;
} Node2;

Node2* head = NULL;
Node2* now = NULL;
int uniqueWords = 0;
int totalMTFRComparisons = 0;

bool DEBUG = true;

static
Node2* iNode(char* word)
{   
    if (DEBUG)
        printf("addNode2 called [[%s]].\n", word);
    Node2* tmp = head;
    Node2* prev = 0;

    while (tmp)
    {
        totalMTFRComparisons++;
        printf("Current word: %s\n", tmp->word);
        if (strcmp(tmp->word, word) == 0)
        {
            printf("Already entered: [[%s]]\n", tmp->word);
            if (prev != 0)
            {
                prev->next = tmp->next;
                tmp->next = head;
                head = tmp;
            }
            //tmp->next = prev;
            return tmp;
        }
        prev = tmp;
        tmp = tmp->next;
        printf("tmp incremented.\n");
    }

    Node2* ptr = malloc(sizeof(Node2));
    printf("New word: [[%s]]\n", word);
    uniqueWords++;
    ptr->word = malloc(strlen(word) + 1);
    strcpy(ptr->word, word);
    ptr->next = NULL;
    if (head == NULL)
    {
        head = now = ptr;
        return ptr;
    }
    else
    {
        ptr->next = head;
        head = ptr;
    }
    return ptr;
}

static
char* getString(void)
{
    static char buffer[128];
    while (fgets(buffer, 128, stdin) != NULL)
    {
        char *nl = strchr(buffer, '\n');
        if (nl != 0)
            *nl = '\0';
        printf("Line: [[%s]]\n", buffer);
        iNode(buffer);
    }
    return buffer;
}

int main(void)
{
    getString();
    printf("Total words: %d, total MTFR comparisons: %d\n", uniqueWords, totalMTFRComparisons);

    Node2* ptr2 = head;
    Node2* tmp2 = head;
    while (ptr2 != NULL)
    {
        printf("Freeing: [[%s]]\n", ptr2->word);
        tmp2 = ptr2->next;
        free(ptr2->word);
        free(ptr2);
        ptr2 = tmp2;
    }
}

It has some diagnostic printing. The newline stripping means the diagnostic output is shorter than you'll see in the chat, if you bother to look at the chat (where the strings had a newline at the end — lines were being treated as words, including the newline).

Answer 2

ptr->word = malloc(strlen(word));

Above Line : incorrect , little memory Ex : strlen("hello") = 5 while sizeof("hello") will give you 6

Changed the line to

ptr->word = malloc(sizeof(word));

Then after strcpy will work correctly