Free memory from linked list in C

Question

I have been trying to free the allocated memory of a file loaded into a linked list, I have managed to free the nodes, but I can't figure out how to free the allocated memory of the file's values copies.

I have tried something like that:

void FreeString(struct node * newNode)
{
    for (int i = 0; i < 5; i++)
    {   
        free(newNode->string);
    }
}

but the compiler would crash with a segmentation fault, and valgrind would still point out to memory leaks.

it would be appreciated if anyone can tell me what am I doing wrong, and point me to the right direction.

Full code:

The struct:

typedef struct node
{
    char *string;
    struct node *next;
}node;

// main function here...

void Push(struct node **RefHead, char *word)
{
    struct node *newNode = NULL;

    newNode = (struct node *)malloc(sizeof(node));

    newNode->string = (char*)malloc(strlen(word) + 1); // can't free this part here
    strcpy(newNode->string, word);
    newNode->next = *RefHead;
    *RefHead = newNode;

}

Loading the file into memory:

void FileToNode()
{
    struct node *head = NULL, *current = NULL;

    infile = fopen("file.txt", "r");
    if (infile == NULL)
    {
        printf("Could not open file\n");
        exit(1);
    }

    while (fgets(word, sizeof(word), infile))
    {
        Push(&head, word);
    }

    fclose(infile);

    current = head;

    while(current)
    {
        printf("%s", current->string);
        current = current->next;
    }


    freeAll(head);

}

The Free function:

void freeAll(struct node *head)
{
    struct node *current = NULL;

    while ((current = head) != NULL)
    {
        head = head->next;
        free(current);
    }
}

What's with the 5 in `FreeString`? Once you've freed an object, you can't free it again! — David Schwartz, Oct 13 '15 at 22:16
I have tried the function without the for loop, but it didn't for, and because there are 5 items in the list, I thought I might iterate over them and free each one. — Yamen Tawk, Oct 13 '15 at 22:19
@Erebus Except that doesn't iterate over anything. It just keeps incrementing `i`. — David Schwartz, Oct 13 '15 at 22:19

score 2 · Accepted Answer · answered Oct 13 '15 at 22:15

2

Am I missing something? What's wrong with:

void freeAll(struct node *head)
{
    struct node *current = NULL;

    while ((current = head) != NULL)
    {
        head = head->next;
        free(current->string);
        free(current);
    }
}

answered Oct 13 '15 at 22:15

melpomene

84,125
8
85
148

This function is working properly for freeing the nodes, but I cant seem to find a way to write a function to free the newNode->string malloc – Yamen Tawk Oct 13 '15 at 22:18
The freeAll function is freeing the nodes, but the newNode->string mallocs are still there, valgrind is giving me the following after running: in use at exit: 28 bytes in 5 blocks total heap usage: 11 allocs, 6 frees, 420 bytes allocated – Yamen Tawk Oct 13 '15 at 22:30
@Erebus With your `freeAll` function or with mine? – melpomene Oct 13 '15 at 22:30

score 0 · Answer 2 · answered Oct 13 '15 at 22:18

0

It's not the problem, but you should probably replace:

newNode->string = (char*)malloc(strlen(word) + 1); // can't free this part here
strcpy(newNode->string, word);

with:

newNode->string = strdup (word);

The problem is this:

void FreeString(struct node * newNode)
{
    for (int i = 0; i < 5; i++)
    {   
        free(newNode->string);
    }
}

Once you call free, newNode->string no longer points to an allocated object (because you just freed it). So you can't pass it to free again.

answered Oct 13 '15 at 22:18

David Schwartz

179,497
17
214
278

I tried it and the compiler would give an error: listtonode.c:46:20: error: implicit declaration of function 'strdup' is invalid in C99 [-Werror,-Wimplicit-function-declaration] newNode->string = strdup (word); ^ listtonode.c:46:20: note: did you mean 'strcmp'? /usr/include/string.h:144:12: note: 'strcmp' declared here extern int strcmp (const char *__s1, const char *__s2) ^ listtonode.c:46:18: error: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Werror,-Wint-conversion] newNode->string = strdup (word); – Yamen Tawk Oct 13 '15 at 22:23
`strdup` is not a standard function and thus not available in `-std=c99` mode. – melpomene Oct 13 '15 at 22:26
@Erebus I wouldn't suggest advising your platform to neuter itself. But if for some reason I had absolutely no choice, I'd just make my own implementation of `strdup` rather than writing out the logic to duplicate a string everywhere. – David Schwartz Oct 13 '15 at 22:27

Free memory from linked list in C

2 Answers2

Linked