1

I was reading the "c primer plus" by Stephen Prata. There is sample program for linked list. The program uses malloc to allocate the memory space for a structure array, the code for the sample program is as below.

#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(void)
{
  struct film * head =NULL;
  struct film * prev, * current;
  char input[TSIZE];

puts("Enter first movie title:");
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);
    puts("Enter your rating <0-10>:");
    scanf("%d",&current->rating);
    while(getchar()!='\n')
        continue;
    puts("Enter next movie title (empty line to stop):");
    prev=current;
}
if(head==NULL)
    printf("No data entered.\n");
else
    printf("Here is the movie list:\n");
current=head;
while(current!=NULL)
{
    printf("Movie: %s Rating: %d\n",current->title,current->rating);

    current=current->next;
}
current=head;
while(current!=NULL)
{
    free(current);
    current=current->next;
}
printf("Bye!\n");

return 0;
}

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

My confusion is from the memory free code. The current is freed by free(current); why the following line can get effect? current=current->next; since current is freed, this line should have no way to access the current memeber "next".

Looking forward to your help on this.

Thanks so much.

Marvin Zhang
  • 169
  • 1
  • 8
  • 1
    This is a bad code. It cannot work. If this is coming from a book - trash it and get a better one. – Eugene Sh. Aug 24 '18 at 13:48
  • 1
    That is a dead loop, the previous loop makes sure `current == NULL` already!! – Sourav Ghosh Aug 24 '18 at 13:50
  • 3
    @SouravGhosh Looks like the OP have messed with the original code. I found the code online and it has `current=head;` before the second loop. But the `free` bug is still there. – Eugene Sh. Aug 24 '18 at 13:55
  • You just got yourself a bad book (one of he many!). You can browse the good books list here: https://web.archive.org/web/20171226004830/https://stackoverflow.com/questions/562303/the-definitive-c-book-guide-and-list (this was a Stack Overflow post, which recently was deleted for unclear reasons). – SergeyA Aug 24 '18 at 14:00
  • It is really bothering me that this (and similar) book is getting 5 star rating in online stores. – Eugene Sh. Aug 24 '18 at 14:02
  • @SergeyA https://meta.stackoverflow.com/questions/355588/the-c-book-list-has-gone-haywire-what-to-do-with-it – Spikatrix Aug 24 '18 at 14:10
  • Sorry, i missed the current=head before the loop to free memory. A type error. – Marvin Zhang Aug 24 '18 at 14:10
  • actually this book is a very famous book for c. I am very close to the end of the book, this is the only one bug i found in the book. Thanks so much for your reply, it really help me a lot. – Marvin Zhang Aug 24 '18 at 14:16

2 Answers2

6

When you do this 

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

You make current pointer dangling and you try to access it current=current->next; which will result in undefined behavior.

I would suggest you to free as below. Also Your current pointer will be pointing to NULL since you have looped till end of the list before to the free while loop.

current=head;
while(current!=NULL)
{
    struct film * temp = current;
    current=current->next;
    free(temp);
}
kiran Biradar
  • 12,700
  • 3
  • 19
  • 44
-2

The free(current); does not clean any of the memory which is at the @ of current, just give it back to the memory pool, so it can be reuse, no cleanning of the memory is done, so it will continue to contain the same data better practive would be to add current=NULL; just after

SLI
  • 1
  • 1
  • Ok, i got your point. It means, without free operation, if you assign any other value to the location, there will be a fatal error and program interrupt. With a free operation, the memory can used to store other things. Thanks so much for your kindly reply. – Marvin Zhang Aug 27 '18 at 02:12