HELP, INFINIT LOOP... simple LINKED LIST

[na1paj]

here's a simple linked list program. the DeleteNode function is
producing an infinit loop i think, but i can't figure out where..


#include <stdio.h>
typedef struct
{
char *str; //str is a dynamic array of characters
int length; //number of characters
} String;

typedef struct node
{
String Data;
struct node* Link;
} ListNode;

typedef struct listStuct
{
ListNode *Head;
}List;

void AddNode(List *L, String item)
{
ListNode *currNode, *newNode;
currNode = L->Head;
if (L->Head == NULL)
{
L->Head = (ListNode*)malloc(sizeof(ListNode));
L->Head->Data = item;
L->Head->Link = NULL;
}
else
{
while(currNode->Link != NULL)
currNode = currNode->Link; //go to the last
newNode = (ListNode*)malloc(sizeof(ListNode));
newNode->Data = item;
newNode->Link = NULL;
currNode->Link = newNode;
}
}

void DeleteNode(List *L, String item)
{
ListNode *currNode, *prevNode;
currNode = L->Head;
prevNode = L->Head;
if (L->Head == NULL)
{
printf("empty list");
}
while(currNode != NULL) //check if first
{
if(strcmp(currNode->Data.str,item.str)==0) //found
{
if(currNode == L->Head) //first one
{
L->Head == currNode->Link;
currNode->Link = NULL;
free(currNode);
printf("Remove First");
}
else
{
prevNode->Link = currNode->Link;
currNode->Link = NULL;
free(currNode);
printf("removed ");
}
}
else
{
prevNode = currNode;
currNode = currNode->Link; //go to next one
}
}
}


int main()
{
FILE *fp = NULL;
List list;
String string;
int i;
int records = 0;
char filename[100];
//list.Head = (ListNode*)malloc(sizeof(ListNode));
list.Head = NULL;
string.str = (char *)malloc(sizeof(char) *20);
printf("Enter filename\n");
scanf("%s", filename);
fp = fopen(filename, "r");

if (fp == NULL)
{ printf ("Error\n");
exit(0);
}
else
{
fscanf(fp, "%d", &records);
for (i = 0; i<records; i++)
{
fscanf(fp, "%s", string.str);
AddNode(&list, string);
printf("%s ", string.str);
}
printf("\n\nEnter a Node to delete: ");
scanf("%s", string.str);
DeleteNode(&list, string);
}
return 1;
}

 

[Artie Gold]

here's a simple linked list program. the DeleteNode function is
producing an infinit loop i think, but i can't figure out where..


#include <stdio.h>
typedef struct
{
char *str; //str is a dynamic array of characters
int length; //number of characters
} String;

typedef struct node
{
String Data;
struct node* Link;
} ListNode;

typedef struct listStuct
{
ListNode *Head;
}List;

Why bother? (Yes, it's a style issue, but having just a pointer to a
`ListNode' would be sufficient. The extra struct definition doesn't
really add anything.

void AddNode(List *L, String item)
{
ListNode *currNode, *newNode;
currNode = L->Head;
if (L->Head == NULL)
{
L->Head = (ListNode*)malloc(sizeof(ListNode));

Better:
L->Head = malloc( sizeof * L->Head );

L->Head->Data = item;
L->Head->Link = NULL;
}
else
{
while(currNode->Link != NULL)
currNode = currNode->Link; //go to the last
newNode = (ListNode*)malloc(sizeof(ListNode));
Similarly.

newNode->Data = item;
newNode->Link = NULL;
currNode->Link = newNode;
}
}

void DeleteNode(List *L, String item)
{
ListNode *currNode, *prevNode;
currNode = L->Head;
prevNode = L->Head;
if (L->Head == NULL)
{
printf("empty list");
}
while(currNode != NULL) //check if first
{
if(strcmp(currNode->Data.str,item.str)==0) //found
{
if(currNode == L->Head) //first one
{
L->Head == currNode->Link;
currNode->Link = NULL;
free(currNode);

OK. Suppose you get here. What's the value of `currNode'?
[Hint: You're not even allowed to look at it. Doing so, as you will
when you get back to the top of the loop, invokes undefined
behavior. So all bets are off.]

printf("Remove First");
}
else
{
prevNode->Link = currNode->Link;
currNode->Link = NULL;
free(currNode);
printf("removed ");
}
}
else
{
prevNode = currNode;
currNode = currNode->Link; //go to next one
}
}
}


int main()
{
FILE *fp = NULL;
List list;
String string;
int i;
int records = 0;
char filename[100];
//list.Head = (ListNode*)malloc(sizeof(ListNode));
list.Head = NULL;
string.str = (char *)malloc(sizeof(char) *20);
printf("Enter filename\n");
scanf("%s", filename);
fp = fopen(filename, "r");

if (fp == NULL)
{ printf ("Error\n");
exit(0);
}
else
{
fscanf(fp, "%d", &records);
for (i = 0; i<records; i++)
{
fscanf(fp, "%s", string.str);
AddNode(&list, string);
printf("%s ", string.str);
}
printf("\n\nEnter a Node to delete: ");
scanf("%s", string.str);
DeleteNode(&list, string);
}
return 1;

Erm, that's a non-standard return value; worse, a non-zero return
value indicates *failure*.

}

There are some other things wrong with the code, but I won't go into
that here. In general, though, the fact that you have special cases
to deal with typically indicates that your design is more complex
than it needs to be. Think about that -- and if you still run into
trouble, by all means repost.

HTH,
--ag

 

[Richard Heathfield]

here's a simple linked list program.

It's not as simple as you think.

the DeleteNode function is
producing an infinit loop i think, but i can't figure out where..


#include <stdio.h>
typedef struct
{
char *str; //str is a dynamic array of characters

If you have a C99 compiler, okay, fine. Do you? If not, // is a syntax
error.

int length; //number of characters

Were you planning on having a negative number of characters for any reason?

In fact, your code seems not to use the length field at all.

} String;

typedef struct node
{
String Data;
struct node* Link;
} ListNode;

typedef struct listStuct
{
ListNode *Head;
}List;

void AddNode(List *L, String item)
{
ListNode *currNode, *newNode;
currNode = L->Head;
if (L->Head == NULL)
{
L->Head = (ListNode*)malloc(sizeof(ListNode));

Undefined behaviour. Your compiler would have warned you about it, too, if
only you hadn't put in that stupid cast.

Rewrite this as:

L->Head = malloc(sizeof *L->Head);

In general, if p is a pointer to some object type, then:

p = malloc(sizeof *p);


Don't forget to #include <stdlib.h> for the malloc prototype (the absence of
which is what gives you undefined behaviour in the above code).

L->Head->Data = item;

You forgot to check malloc's return value, which could easily have been NULL
(meaning "couldn't allocate the requested memory"). If it was, then
dereferencing that pointer (as you do here) invokes undefined behaviour.

L->Head->Link = NULL;
}
else
{
while(currNode->Link != NULL)
currNode = currNode->Link; //go to the last
newNode = (ListNode*)malloc(sizeof(ListNode));

newNode = malloc(sizeof *newNode);

Don't forget to check for NULL.

newNode->Data = item;
newNode->Link = NULL;
currNode->Link = newNode;
}
}

void DeleteNode(List *L, String item)
{
ListNode *currNode, *prevNode;
currNode = L->Head;
prevNode = L->Head;
if (L->Head == NULL)
{
printf("empty list");
}
while(currNode != NULL) //check if first
{
if(strcmp(currNode->Data.str,item.str)==0) //found
{
if(currNode == L->Head) //first one
{
L->Head == currNode->Link;
currNode->Link = NULL;
free(currNode);
printf("Remove First");
}
else
{
prevNode->Link = currNode->Link;
currNode->Link = NULL;
free(currNode);
printf("removed ");
}
}
else
{
prevNode = currNode;
currNode = currNode->Link; //go to next one
}
}
}

I had a quick look at this, but the complete absence of indentation made the
code very hard to follow, so I gave up.

int main()
{
FILE *fp = NULL;
List list;
String string;
int i;
int records = 0;
char filename[100];
//list.Head = (ListNode*)malloc(sizeof(ListNode));
list.Head = NULL;
string.str = (char *)malloc(sizeof(char) *20);

string.str = malloc(20 * sizeof *string.str);

Note: 20 may not be enough. If not, you risk a buffer overrun attack.

Test for NULL.

printf("Enter filename\n");
scanf("%s", filename);

Danger of buffer overrun attack by careless or malicious user.

fp = fopen(filename, "r");

if (fp == NULL)
{ printf ("Error\n");
exit(0);

0 indicates success. On error, I suggest using EXIT_FAILURE (which is

}
else
{
fscanf(fp, "%d", &records);
for (i = 0; i<records; i++)
{
fscanf(fp, "%s", string.str);

Buffer overrun may occur.

AddNode(&list, string);
printf("%s ", string.str);
}
printf("\n\nEnter a Node to delete: ");

Either print a newline here or fflush(stdout);

scanf("%s", string.str);

Buffer overrun may occur.

DeleteNode(&list, string);
}
return 1;

return EXIT_SUCCESS or return 0 would be a much better idea. Google the
archives for an explanation.

 

[T.M. Sommers]

here's a simple linked list program. the DeleteNode function is
producing an infinit loop i think, but i can't figure out where..

Other posters have already pointed out some errors. Here are more.

#include <stdio.h>

#include <string.h>

typedef struct
{
char *str; //str is a dynamic array of characters
int length; //number of characters
} String;

typedef struct node
{
String Data;
struct node* Link;
} ListNode;

typedef struct listStuct
{
ListNode *Head;
}List;

void AddNode(List *L, String item)
{
ListNode *currNode, *newNode;
currNode = L->Head;
if (L->Head == NULL)
{
L->Head = (ListNode*)malloc(sizeof(ListNode));
L->Head->Data = item;
L->Head->Link = NULL;
}
else
{
while(currNode->Link != NULL)
currNode = currNode->Link; //go to the last
newNode = (ListNode*)malloc(sizeof(ListNode));
newNode->Data = item;
newNode->Link = NULL;
currNode->Link = newNode;
}
}

void DeleteNode(List *L, String item)
{
ListNode *currNode, *prevNode;
currNode = L->Head;
prevNode = L->Head;
if (L->Head == NULL)
{
printf("empty list");
}
while(currNode != NULL) //check if first
{
if(strcmp(currNode->Data.str,item.str)==0) //found
{
if(currNode == L->Head) //first one
{
L->Head == currNode->Link;

L->Head = currNode->Link;

currNode->Link = NULL;
free(currNode);
printf("Remove First");
}
else
{
prevNode->Link = currNode->Link;
currNode->Link = NULL;
free(currNode);
printf("removed ");
}

break;

}
else
{
prevNode = currNode;
currNode = currNode->Link; //go to next one
}
}
}


int main()
{
FILE *fp = NULL;
List list;
String string;
int i;
int records = 0;
char filename[100];
//list.Head = (ListNode*)malloc(sizeof(ListNode));
list.Head = NULL;
string.str = (char *)malloc(sizeof(char) *20);
printf("Enter filename\n");
scanf("%s", filename);
fp = fopen(filename, "r");

if (fp == NULL)
{ printf ("Error\n");
exit(0);
}
else
{
fscanf(fp, "%d", &records);
for (i = 0; i<records; i++)
{
fscanf(fp, "%s", string.str);
AddNode(&list, string);
printf("%s ", string.str);
}
printf("\n\nEnter a Node to delete: ");
scanf("%s", string.str);

Here is the real problem. Your String structure contains a pointer to
an array of chars. When adding a node, you copy the structure, and
hence the pointer, so every node is pointing to the buffer you
allocated above in main. Including the String structure you are using
as the key to delete.

 

[goose]

here's a simple linked list program. the DeleteNode function is
producing an infinit loop i think, but i can't figure out where..

a really good idea would be to turn up the warning/diagnostics
level of your compiler. let your compiler catch the obvious
bugs. I've numbered my "fixes", this lets me only explain
each "fix" once, and refer back to it when I come across it again.

It would be a good idea if you wrote a function that could
loop though a list and print out each element. that way you
can verify that what you added to the list is in the list and
what you deleted from the list has in fact been deleted.

#include <stdio.h>

1. you also need to #include <stdlib.h> for the malloc/free
function calls and string.h (for strncpy):

#include <stdlib.h>

typedef struct
{
char *str; //str is a dynamic array of characters
int length; //number of characters
} String;

typedef struct node
{ f struct node
String Data;
struct node* Link;
} ListNode;

typedef struct listStuct
{ f struct listStuct
ListNode *Head;
}List;

void AddNode(List *L, String item)
{ ddNode(List *L, String item)
ListNode *currNode, *newNode; ers
currNode = L->Head; Node; ers
if (L->Head == NULL) Node; ers
{ ULL) Node; ers
L->Head = (ListNode*)malloc(sizeof(ListNode)); (e-mail address removed)>...

2. there is no need to cast the return of malloc. yuor compiler
probably complained about this line without the cast because
you didn't include stdlib.h.
3. a better idea is to use sizeof on the variable itself,
not on the type:

L->Head = malloc(sizeof L->Head); e itself,

4. you must *always* check the return of malloc. what in case
there was no memory ? do something like this:

if (L->Head==NULL) {
printf ("no memory for item %s\n", item.str);
return;
}

L->Head->Data = item; .str);

5. there is a fundamental misconception here with regard to the
way you /think/ the String struct works. when you /assign/
item to L->Head->Data, the field "length" gets set correctly.
but the /other/ field ("str") does not get copied into. I
suspect that that is what you would want. you should do this
instead:

L->Head->Data.length = item.length;
L->Head->Data.str = malloc (item.length);
if (L->Head->Data.str==NULL) {
printf ("no memory for str%s\n", item.str);
free (L->Head);
return;
}
strncpy (L->Head->Data.str, item.str, item.length);
L->Head->Data.str[item.length] = '\0';

L->Head->Link = NULL; ;
} L->Head->Link = NULL; ;
else L->Head->Link = NULL; ;
{ >Link = NULL; ;
while(currNode->Link != NULL)
currNode = currNode->Link; //go to the last (e-mail address removed)>...
newNode = (ListNode*)malloc(sizeof(ListNode)); (e-mail address removed)>...

see fix #2, #3 and #4:
newNode = malloc(sizeof *newNode); ode));
(e-mail address removed)>...
if (newNode==NULL) {
printf ("no memory for item %s\n", item.str);
return;
}

newNode->Data = item; .str);

see fix #5:

newNode->Data.length = item.length;
newNode->Data.str = malloc (item.length);
if (newNode->Data.str==NULL) {
printf ("no memory for str%s\n", item.str);
free (newNode);
return;
}
strncpy (L->Head->Data.str, item.str, item.length);
L->Head->Data.str[item.length] = '\0';

newNode->Link = NULL; ngth);
currNode->Link = newNode; ngth);
}
}

void DeleteNode(List *L, String item)
{ eleteNode(List *L, String item)
ListNode *currNode, *prevNode; ; ngth);
currNode = L->Head;
prevNode = L->Head; vNode; ; ngth);
if (L->Head == NULL)
{ ULL)
printf("empty list"); ; ngth);

6. you have correctly identified this as an empty list.
dont you think it might be better to merely return
immediately ? there is no need to actually check,
because the loop below will never execute if the
list is empty.

} .
while(currNode != NULL) //check if first ode)); (e-mail address removed)>...
{ //check if first ode)); (e-mail address removed)>...
if(strcmp(currNode->Data.str,item.str)==0) //found sting.google.com>...
{
if(currNode == L->Head) //first one
{
L->Head == currNode->Link;

7. this line does nothing. your compiler might have emitted
a diagnostic (did your compiler complain?). I replaced it
withs:
L->Head = currNode->Link;

currNode->Link = NULL;
free(currNode);

8. before you can do that, we need to free the memory in item
that I allocated earlier. do this instead:

free (currNode->Data.str);
free (currNode);

printf("Remove First");
}
else
{ item
prevNode->Link = currNode->Link; >...
currNode->Link = NULL;
free(currNode); >...

see #8 above:

free (currNode->Data.str);
free (currNode);

printf("removed ");
} removed ");
}
else
{
prevNode = currNode;
currNode = currNode->Link; //go to next one >...
}
}
}


int main()
{ in()
FILE *fp = NULL; = currNode->Link; //go to next one >...
List list; = currNode->Link; //go to next one >...
String string; = currNode->Link; //go to next one >...
int i; tring; = currNode->Link; //go to next one >...
int records = 0; = currNode->Link; //go to next one >...
char filename[100];
//list.Head = (ListNode*)malloc(sizeof(ListNode)); >...
list.Head = NULL; istNode*)malloc(sizeof(ListNode)); >...
string.str = (char *)malloc(sizeof(char) *20); istNode)); >...

9. sizeof (char) is always equal to one, also see fixes
#2 and #4 above:

string.str = malloc(20); to one, also see fixes
if (string.str==NULL) {
printf ("no memory\n");
return EXIT_FAILURE;
}

printf("Enter filename\n"); also see fixes
scanf("%s", filename); "); also see fixes
fp = fopen(filename, "r"); also see fixes
fp = fopen(filename, "r"); also see fixes
if (fp == NULL)
{ printf ("Error\n"); >...
exit(0); printf ("Error\n"); >...
}
else
{
fscanf(fp, "%d", &records);
for (i = 0; i<records; i++) "); >...
{
fscanf(fp, "%s", string.str);

10.what in case the string you are trying
to read in is bigger than the space you allocated ?
try this instead:

fscanf(fp, "%19s", string.str);

11.you forget to set the length field of the struct
"string", try this:

string.length = strlen (string.str) +1;

AddNode(&list, string);
printf("%s ", string.str);
}
printf("\n\nEnter a Node to delete: ");
scanf("%s", string.str);
DeleteNode(&list, string);
}
return 1;
}

hth

goose,
hand

 

[goose]

oops!!! the previous post got messed over somehow, i hope
this gets though without any problems:

here's a simple linked list program. the DeleteNode function is
producing an infinit loop i think, but i can't figure out where..

a really good idea would be to turn up the warning/diagnostics
level of your compiler. let your compiler catch the obvious
bugs. I've numbered my "fixes", this lets me only explain
each "fix" once, and refer back to it when I come across it again.

It would be a good idea if you wrote a function that could
loop though a list and print out each element. that way you
can verify that what you added to the list is in the list and
what you deleted from the list has in fact been deleted.

#include <stdio.h>

1. you also need to #include <stdlib.h> for the malloc/free
function calls and string.h (for strncpy):

#include <stdlib.h>

typedef struct
{
char *str; //str is a dynamic array of characters
int length; //number of characters
} String;

typedef struct node
{
String Data;
struct node* Link;
} ListNode;

typedef struct listStuct
{
ListNode *Head;
}List;

void AddNode(List *L, String item)
{
ListNode *currNode, *newNode;
currNode = L->Head;
if (L->Head == NULL)
{
L->Head = (ListNode*)malloc(sizeof(ListNode));

2. there is no need to cast the return of malloc. yuor compiler
probably complained about this line without the cast because
you didn't include stdlib.h.
3. a better idea is to use sizeof on the variable itself,
not on the type:

L->Head = malloc(sizeof L->Head);

4. you must *always* check the return of malloc. what in case
there was no memory ? do something like this:

if (L->Head==NULL) {
printf ("no memory for item %s\n", item.str);
return;
}

L->Head->Data = item;

5. there is a fundamental misconception here with regard to the
way you /think/ the String struct works. when you /assign/
item to L->Head->Data, the field "length" gets set correctly.
but the /other/ field ("str") does not get copied into. I
suspect that that is what you would want. you should do this
instead:

L->Head->Data.length = item.length;
L->Head->Data.str = malloc (item.length);
if (L->Head->Data.str==NULL) {
printf ("no memory for str%s\n", item.str);
free (L->Head);
return;
}
strncpy (L->Head->Data.str, item.str, item.length);
L->Head->Data.str[item.length] = '\0';

L->Head->Link = NULL;
}
else
{
while(currNode->Link != NULL)
currNode = currNode->Link; //go to the last
newNode = (ListNode*)malloc(sizeof(ListNode));

see fix #2, #3 and #4:
newNode = malloc(sizeof *newNode);
if (newNode==NULL) {
printf ("no memory for item %s\n", item.str);
return;
}

newNode->Data = item;

see fix #5:

newNode->Data.length = item.length;
newNode->Data.str = malloc (item.length);
if (newNode->Data.str==NULL) {
printf ("no memory for str%s\n", item.str);
free (newNode);
return;
}
strncpy (L->Head->Data.str, item.str, item.length);
L->Head->Data.str[item.length] = '\0';

newNode->Link = NULL;
currNode->Link = newNode;
}
}

void DeleteNode(List *L, String item)
{
ListNode *currNode, *prevNode;
currNode = L->Head;
prevNode = L->Head;
if (L->Head == NULL)
{
printf("empty list");

6. you have correctly identified this as an empty list.
dont you think it might be better to merely return
immediately ?

}
while(currNode != NULL) //check if first
{
if(strcmp(currNode->Data.str,item.str)==0) //found
{
if(currNode == L->Head) //first one
{
L->Head == currNode->Link;

7. this line does nothing. your compiler might have emitted
a diagnostic (did your compiler complain?). I replaced it
withs:
L->Head = currNode->Link;

currNode->Link = NULL;
free(currNode);

8. before you can do that, we need to free the memory in item
that I allocated earlier. do this instead:

free (currNode->Data.str);
free (currNode);

printf("Remove First");
}
else
{
prevNode->Link = currNode->Link;
currNode->Link = NULL;
free(currNode);

see #8 above:

free (currNode->Data.str);
free (currNode);

printf("removed ");
}
}
else
{
prevNode = currNode;
currNode = currNode->Link; //go to next one
}
}
}


int main()
{
FILE *fp = NULL;
List list;
String string;
int i;
int records = 0;
char filename[100];
//list.Head = (ListNode*)malloc(sizeof(ListNode));
list.Head = NULL;
string.str = (char *)malloc(sizeof(char) *20);

9. sizeof (char) is always equal to one, also see fixes
#2 and #4 above:

string.str = malloc(20);
if (string.str==NULL) {
printf ("no memory\n");
return EXIT_FAILURE;
}

printf("Enter filename\n");
scanf("%s", filename);
fp = fopen(filename, "r");

if (fp == NULL)
{ printf ("Error\n");
exit(0);
}
else
{
fscanf(fp, "%d", &records);
for (i = 0; i<records; i++)
{
fscanf(fp, "%s", string.str);

10.what in case the string you are trying
to read in is bigger than the space you allocated ?
try this instead:

fscanf(fp, "%19s", string.str);

11.you forget to set the length field of the struct
"string", try this:

string.length = strlen (string.str) +1;

AddNode(&list, string);
printf("%s ", string.str);
}
printf("\n\nEnter a Node to delete: ");
scanf("%s", string.str);
DeleteNode(&list, string);
}
return 1;
}

hth

goose,
hand

 

[Rahul Agarkar]

(e-mail address removed) (na1paj) wrote in message Hi,

The problem is because of currNode in the function DeleteNode. When
you say free(currNode), it just frees the memory allocated to that
variable but doesn't clear the contents of the memory.

In short, that memory area is marked as free that can be reallocated
to some other variable but the contents still remain the same.

This is a very common problem related with the common programming
practice.

For clearing the memory, just don't rely on free(). The good
programming practice is to assign NULL to the variable after calling
free().

Please make some changes in the code. The changes are given below:

here's a simple linked list program. the DeleteNode function is
producing an infinit loop i think, but i can't figure out where..


#include <stdio.h>
typedef struct
{
char *str; //str is a dynamic array of characters
int length; //number of characters
} String;

typedef struct node
{
String Data;
struct node* Link;
} ListNode;

typedef struct listStuct
{
ListNode *Head;
}List;

void AddNode(List *L, String item)
{
ListNode *currNode, *newNode;
currNode = L->Head;
if (L->Head == NULL)
{
L->Head = (ListNode*)malloc(sizeof(ListNode));
L->Head->Data = item;
L->Head->Link = NULL;
}
else
{
while(currNode->Link != NULL)
currNode = currNode->Link; //go to the last
newNode = (ListNode*)malloc(sizeof(ListNode));
newNode->Data = item;
newNode->Link = NULL;
currNode->Link = newNode;
}
}

void DeleteNode(List *L, String item)
{
ListNode *currNode, *prevNode;
currNode = L->Head;
prevNode = L->Head;
if (L->Head == NULL)
{
printf("empty list");
}
while(currNode != NULL) //check if first
{
if(strcmp(currNode->Data.str,item.str)==0) //found
{
if(currNode == L->Head) //first one
{
L->Head == currNode->Link;

-----> This code is of no use as '==' is a comparison operator whereas
here you have to use a assignment operator '='. Hence change the above
line to :
L->Head = currNode->Link;

currNode->Link = NULL;
free(currNode);

-----> Add the following code to stop your programme to go into the
infinite loop.
currNode = NULL;

printf("Remove First");
}
else
{
prevNode->Link = currNode->Link;
currNode->Link = NULL;
free(currNode);

-----> Add the following code to stop your programme to go into the
infinite loop.
currNode = NULL;

printf("removed ");
}
}
else
{
prevNode = currNode;
currNode = currNode->Link; //go to next one
}
}
}


int main()
{
FILE *fp = NULL;
List list;
String string;
int i;
int records = 0;
char filename[100];
//list.Head = (ListNode*)malloc(sizeof(ListNode));
list.Head = NULL;
string.str = (char *)malloc(sizeof(char) *20);
printf("Enter filename\n");
scanf("%s", filename);
fp = fopen(filename, "r");

if (fp == NULL)
{ printf ("Error\n");
exit(0);
}
else
{
fscanf(fp, "%d", &records);
for (i = 0; i<records; i++)
{
fscanf(fp, "%s", string.str);
AddNode(&list, string);
printf("%s ", string.str);
}
printf("\n\nEnter a Node to delete: ");
scanf("%s", string.str);
DeleteNode(&list, string);
}
return 1;
}

The above changes will interrupt the infinite loop. When some memory
is freed using free() call, assigning NULL to that variable will clear
up that memory area.

These things are never explained in any of the books. One can only
learn these things with experience.

 

[nobody]

(e-mail address removed) (na1paj) wrote in message

Hi,

The problem is because of currNode in the function DeleteNode. When
you say free(currNode), it just frees the memory allocated to that
variable but doesn't clear the contents of the memory.

This is not a problem., as far as I can see it.

In short, that memory area is marked as free that can be reallocated
to some other variable but the contents still remain the same.

So what? Contents was random even after initial call to malloc().

This is a very common problem related with the common programming
practice.

Maybe it's common, but yet again, it's not a problem, generally speaking.

For clearing the memory, just don't rely on free(). The good
programming practice is to assign NULL to the variable after calling
free().

This doesn't "clear" memory, whatever you mean by "clear". It just
prevents UB(?) if an attempt is made to free same block more than once.

[snip]

The above changes will interrupt the infinite loop. When some memory
is freed using free() call, assigning NULL to that variable will clear
up that memory area.

It will not (regardless of definition of "clear"), see above.

These things are never explained in any of the books. One can only
learn these things with experience.

Obviously, there is good reason not to put nonsense in the books.
Hope you are not going to write any book on C any time soon.

 

[Kevin Goodsell]

This doesn't "clear" memory, whatever you mean by "clear". It just
prevents UB(?) if an attempt is made to free same block more than once.

It doesn't even do that. All it does is prevent UB if an attempt is made
to free the same block more than once *through the same pointer*. I
think errors due to freeing the same block more than once are usually
the result of freeing it through multiple pointers to the same location.
Setting your pointers to NULL after freeing them doesn't help matters.
In fact, it may make matters worse since people tend to assume that if
they assign NULL to freed pointers, non-NULL pointers are safe to free.
Clearly this is not true in general.

-Kevin

 

[nobody]

It doesn't even do that. All it does is prevent UB if an attempt is made
to free the same block more than once *through the same pointer*. I

Sorry for insufficient clarity on my part. This is what meant, in the
context
of (snipped) code.

think errors due to freeing the same block more than once are usually
the result of freeing it through multiple pointers to the same location.
Setting your pointers to NULL after freeing them doesn't help matters.
In fact, it may make matters worse since people tend to assume that if
they assign NULL to freed pointers, non-NULL pointers are safe to free.
Clearly this is not true in general.

Right. Again, I was referring (perhaps insufficiently) to code like OP's,
where
it's always "same" pointer, but it's difficult to trace multiple free()
calls on same
pointer. (Personally, I prefer *not* to assign NULL /I even prefer more not
to assign 0/, so problem is not hidden but is exhibited via UB and fixed at
the first opportunity.)
BTW, If I didn't read the reply (to which I had replied) to OP, I would
polemize
with your statement "people tend to assume that if ...". Obviously, some
people do (tend to assume illogical things).

 

[Rahul Agarkar]

This is not a problem., as far as I can see it.

Did you try the changes suggested by me ? If not, then please try
them.

So what? Contents was random even after initial call to malloc().

You are absolutely right. I am not denying the fact that even after
calling malloc, you have some value. But BEWARE, this is not a valid
value, this is just a garbage and any condition of type
if (x)
would return true in such a case. Although there is some value in
the pointer, but this is not a valid value. So, its better to reset
the memory contents by assigning NULL so that any references to this
pointer can be avoided. Then what is the way of testing the value of a
pointer if it is a valid pointer.

I am talking in the context of the original problem posted here
dealing with the infinite loop.

To mark the end of the list, you have to assign some value to
currNode. Assigning NULL is the safest way to do so.

Also, assigning NULL to a pointer after calling free, makes sure
that this pointer doesn't have any garbage value also.

Maybe it's common, but yet again, it's not a problem, generally speaking.

This doesn't "clear" memory, whatever you mean by "clear". It just
prevents UB(?) if an attempt is made to free same block more than once.

I am not trying to say that the pointer going to be modified here. I
am just checking the value assigned to the pointer which should be
NULL to mark the end of the list.

[snip]

The above changes will interrupt the infinite loop. When some memory
is freed using free() call, assigning NULL to that variable will clear
up that memory area.

It will not (regardless of definition of "clear"), see above.

Assigning NULL to the pointer will reset the memory pointed by the
pointer.

Obviously, there is good reason not to put nonsense in the books.
Hope you are not going to write any book on C any time soon.

I am not. But, i would definitely like to write a book on practical
approaches of writing C programmes.

 

[goose]

(e-mail address removed) (Rahul Agarkar) wrote in message

[snip]

The above changes will interrupt the infinite loop. When some memory
is freed using free() call, assigning NULL to that variable will clear
up that memory area.

It will not (regardless of definition of "clear"), see above.

Assigning NULL to the pointer will reset the memory pointed by the
pointer.

it will not, it will set the pointer variable to NULL.

<snipped>

hth
goose,

 

[goose]

(e-mail address removed) (goose) wrote in message
<snipped>

oops (again!), I've given incorrect information below,
see below:

2. there is no need to cast the return of malloc. yuor compiler
probably complained about this line without the cast because
you didn't include stdlib.h.
3. a better idea is to use sizeof on the variable itself,
not on the type:

L->Head = malloc(sizeof L->Head);

this should have been:
L->Head = malloc (sizeof *L->Head);

sorry for any inconvenience and/or strange crashes
and resultant hair-pulling caused by this

<snipped>

goose,
one always gets through.

 

[Sheldon Simms]

its better to reset the memory contents by assigning NULL... ....
Assigning NULL to the pointer will reset the memory pointed by the
pointer.

I'm not sure if your problem is a misunderstanding of C or a misuse
of the english language, so maybe you already know what I'm about
to say:

Consider the following code:

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

int main (void)
{
unsigned char * p;
unsigned char * q;

p = malloc(1024);
if (p == NULL) exit(EXIT_FAILURE);
q = malloc(1024);
if (q == NULL) exit(EXIT_FAILURE);

/* Point A */

memset(p, 0, 1024);
/* Point B */

q = NULL;
/* Point C */

free(p);
/* Point D */

p = NULL;
/* Point E */

return 0;
}

* At Point A, both p and q point to 1024 byte memory objects. The
values of p and q are well defined, but the contents of the objects
to which they point are indeterminate.

* At Point B, the values of p and q are the same as they were at
Point A. The contents of the object pointed to by q are still
indeterminate. The contents of the object pointed to by p are
no longer indeterminate. Every bit in each of the 1024 bytes in
that object is now 0.

* At Point C, the value of p and the contents of the object p points
to are the same as they were at Point B. q is now a null pointer
and no longer points to any object. The object that q used to
point to *still exists* and it's contents are *still indeterminate*
The memory that was pointed to by q is has *not* been "reset" or
"cleared".

* At Point D, the object that had been pointed to by p has now been
deallocated. It no longer exists as an object. The value of p
has not changed at all; it is the same. However, the value of p
is now an "invalid value" and p may not be dereferenced.

* At Point E, p is a null pointer and no longer points to any
object.

-Sheldon