cerror-handling

Why does this C program free up memory that doesn't exist and doesn't output Bye as written?


The code is as follows: It is from the book "C Primer Plus" 6th edition _ Chinese version

// This program creates a linked list of films and displays them.
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define TSIZE 45

struct film{
    char title[TSIZE];
    int rating;
    struct film *next;
};

char *s_gets(char *st, int n);

int main(int argc, char const *argv[])
{
    struct film *head = NULL;
    struct film *prev, *current;
    char input[TSIZE];

    printf("Enter first movie title (or enter to quit): ");
    while(s_gets(input, TSIZE)!= NULL && input[0]!= '\0')
    {
        current = (struct film*) malloc(sizeof(struct film));
        if (head == NULL)
            head = current;
        else
            prev->next = current;
        current->next = NULL;
        strcpy(current->title, input);
        printf("Enter your rating <0-10>: ");
        scanf("%d", &current->rating);
        while (getchar()!='\n')
            continue;
        printf("Enter next movie title (or enter to quit): ");
        prev=current;
    }
    if (head == NULL)
        printf("No data entered. ");
    else
        printf("Here is the list of films: \n");
    current = head;
    while (current!= NULL)
    {
        printf("Movie: %s, Rating: %d\n",
               current->title, current->rating);
        current = current->next;
    }
    current = head;
    while (current!= NULL)
    {
        printf("freeing memory for %s\n", current->title);
        current=head;
        head=head->next;
        free(current);
    }
    fflush(stdout);
    printf("Bye!\n");
    fflush(stdout);
    return 0;
}

char * s_gets(char *st, int n)
{
    char *ret_val;
    char *find;
    ret_val = fgets(st, n, stdin);
    if (ret_val) {
        find = strchr(st, '\n');
        if (find) {
            *find = '\0';
        } else {
            while (getchar()!= '\n')
                continue;
        }
    }
    return ret_val;
}

Here is the output from running the program:

PS C:\Develop\c\c> gcc .\film2.c -o .\film2     
PS C:\Develop\c\c> .\film2     
PS C:\Develop\c\c> gcc .\film2.c -o film2  
Enter first movie title (or enter to quit): ashah
Enter your rating <0-10>: 4
Enter next movie title (or enter to quit): dhaah
Enter your rating <0-10>: 2
Enter next movie title (or enter to quit): adhazb
Enter your rating <0-10>: 1
Enter next movie title (or enter to quit):
Here is the list of films:
Movie: ashah, Rating: 4
Movie: dhaah, Rating: 2
Movie: adhazb, Rating: 1
PS C:\Develop\c\c> .\film2.exe
Enter first movie title (or enter to quit): SG
Enter your rating <0-10>: 32
Enter next movie title (or enter to quit): aghah
Enter your rating <0-10>: 4
Enter next movie title (or enter to quit):
Here is the list of films:
Movie: SG, Rating: 4
Movie: sahhd, Rating: 32
Movie: aghah, Rating: 4
PS C:\Develop\c\c> ^C
PS C:\Develop\c\c> ^C
PS C:\Develop\c\c> gcc .\film2.c -o film2
PS C:\Develop\c\c> .\film2.exe
Enter first movie title (or enter to quit): asg
Enter your rating <0-10>: 4
Enter next movie title (or enter to quit): adeha
Enter your rating <0-10>: 3
Enter next movie title (or enter to quit):
Here is the list of films:
Movie: asg, Rating: 4
Movie: adeha, Rating: 3
Movie: aash, Rating: 3
freeing memory for asg
PS C:\Develop\c\c> .\film2.exe
Enter first movie title (or enter to quit): gag
Enter your rating <0-10>: 3
Enter next movie title (or enter to quit):
Here is the list of films:
Movie: gag, Rating: 3
freeing memory for gag
freeing memory for eD
PS C:\Develop\c\c>
PS C:\Develop\c\c>
PS C:\Develop\c\c> gcc .\film2.c -o film2
Enter next movie title (or enter to quit): adehg
Enter your rating <0-10>: 32
Enter next movie title (or enter to quit):
Here is the list of films:
Movie: asg, Rating: 326
Movie: adehg, Rating: 32
freeing memory for asg
freeing memory for }5P
freeing memory for adehg
PS C:\Develop\c\c>
PS C:\Develop\c\c>
PS C:\Develop\c\c>
PS C:\Develop\c\c>


PS C:\KuGou\mp4> C:\Develop\c\c\film2.exe
Enter first movie title (or enter to quit): sgag
Enter your rating <0-10>: 3
Enter your rating <0-10>: 4
Enter next movie title (or enter to quit): jpsn
Enter next movie title (or enter to quit):
Here is the list of films:
Movie: sgag, Rating: 3
Movie: ahjkjoa, Rating: 4
Movie: jpsn, Rating: 56
freeing memory for sgag
freeing memory for 锢z
freeing memory for ahjkjoa
freeing memory for jpsn
PS C:\KuGou\mp4> C:\Develop\c\c\film2.exe
Enter first movie title (or enter to quit): dfsj
Enter your rating <0-10>: 3
Enter next movie title (or enter to quit): jopiap
Enter your rating <0-10>: 4
Enter next movie title (or enter to quit): irwal
Enter your rating <0-10>: 6
Enter next movie title (or enter to quit):
Here is the list of films:
Movie: dfsj, Rating: 3
Movie: jopiap, Rating: 4
Movie: irwal, Rating: 6
freeing memory for dfsj
freeing memory for 谟I
freeing memory for jopiap
freeing memory for irwal


And the program has to wait tens of seconds after printing the last sentence "freeing memory for irwal" before exiting

At first I thought the code was broken when free() was running, or the buffer wasn't refreshing, so I added these two lines: printf("freeing memory for %s\n", current->title); fflush(stdout); But a new problem arose: Why is memory freed for a nonexistent structure, and the program still frees memory normally? Why??????


Solution

  • The problem is in this loop:

    while (current!= NULL)
    {
        printf("freeing memory for %s\n", current->title);
        current=head;
        head=head->next;
        free(current);
    }
    

    After you call free(current), the memory that current is pointing to is no longer valid. So the next iteration when you use current->title you will access that invalid memory and have undefined behavior.

    Also note that this loop will try to use head twice, in the first and second iteration.

    You need to change the order in which you do things:

    while (current!= NULL)
    {
        printf("freeing memory for %s\n", current->title);
        head=head->next;  // Get the next node in the list
        free(current);    // Free the current node
        current=head;     // Make the next node the current node
    }