# C and struct lists

This topic is 4831 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Well I was writing some word list code. It works OK, everything is perfect, but maybe I do miss some memory leaks, it is wery hard to find them .So il ask your help. Can anyone suggest is there a way to improve my code, or are there some mistakes ? i do have some concerns where i pass new struct to temp pointer. tnx in advance.
#include <stdio.h>

struct WSTRUCT;
//this is our words
struct WSTRUCT *words;

void FreeWords();

struct WSTRUCT
{

//word
char* word;
//pointer to next struct NULL if none
struct WSTRUCT *next;

};

//frees memory used
void FreeWords()
{

while(words != NULL)
{
if(words->word != NULL)
free(words->word);

words = words->next;
}
}

{

struct WSTRUCT *temp;
temp = words;

//this indicates that the are no
//words in a list
//so we add word and exit
if(words == NULL)
{
//now we create new structure
words = (struct WSTRUCT*)malloc(sizeof(struct WSTRUCT));

//we create a new word
words->word = (char*)malloc(sizeof(char) * length);
//and write a value to it
sprintf(words->word,"%s",word);

//and point the next pointer
//to NULL
words->next = NULL;

return;
}
else

while(temp->next != NULL)
{
temp = temp->next;
}
//now temp is the last structure we have
//so let's make it work

temp->next = (struct WSTRUCT*)malloc(sizeof(struct WSTRUCT));

//we create a new word
temp->next->word = (char*)malloc(sizeof(char) * length);
//and write a value to it
sprintf(temp->next->word,"%s",word);

//and point the next pointer
//to NULL
temp->next->next = NULL;

return;
}

int main()
{

printf("ALLOK\n");

//frees memory
FreeWords();
return 0;
}



##### Share on other sites
The code seems fine to me. Just a few pointers though,
• When creating a pointer always set it to NULL
• When freeing a pointer consider setting it to NULL, not really necessary, but can sometimes cause problems
• Does word really need to be a global

If your really worried about memory leaks have a look at a program called electric fence that can detect memory leaks, in a running program.

##### Share on other sites
I only glanced at your code, the only thing i'll say is you can only make one list of strings with your code.

##### Share on other sites
If you make the destructor of WSTRUCT: delete next, then you only have to delete the first. Remember to set every deleted pointer to NULL or 0.

##### Share on other sites
Quote:
 Original post by Pipo DeClownIf you make the destructor of WSTRUCT: delete next, then you only have to delete the first. Remember to set every deleted pointer to NULL or 0.

He's using C, no delete operator, no user-defined destructor just plain vanilla functions [smile]

##### Share on other sites
[attention] Thanks for pointing out. [grin] My bad.. XP

However, isn't the C-way to declare structs like "typedef struct {} sStruct;" ??

##### Share on other sites
Quote:
 Original post by Pipo DeClownHowever, isn't the C-way to declare structs like "typedef struct {} sStruct;" ??

You don't have to, its just syntactic sugar for a C programmer [smile]

##### Share on other sites
Great thank's to everyone

Quote:
 However, isn't the C-way to declare structs like "typedef struct {} sStruct;" ??

Nice i didn't use it before but, now i read in the book, and it saves me quite a few lines, instead of writing

struct WSTRUCT words = (struct WSTRUCT*)malloc(sizeof(struct WSTRUCT) * 10);

i can write ...

WSTRUCT words = (WSTRUCT*)malloc(sizeof(WSTRUCT) * 10);

it looks and reads far better.

Quote:
 The code seems fine to me. Just a few pointers though,When creating a pointer always set it to NULLWhen freeing a pointer consider setting it to NULL, not really necessary, but can sometimes cause problemsDoes word really need to be a globalIf your really worried about memory leaks have a look at a program called electric fence that can detect memory leaks, in a running program.

1. Where I am missing it ?
2. I am confused here, maybe an example ?
3. Well i only need to use only one variable , so there
is no need to pass it as argument

Quote:
 I only glanced at your code, the only thing i'll say is you can only make one list of strings with your code.

Yep that's true it's the only number of words lists i should need for now.

##### Share on other sites
Simple really, look at your free function
you release the string, but not the struct that you allocated to hold the string [grin]

[Edited by - Washu on November 25, 2004 2:52:27 PM]

##### Share on other sites
Quote:
 Original post by WashuSimple really, look at your free functionyou release the string, but the struct that you allocated to hold the string [grin]

Yeah, Washu found it! [lol] You need to release the strings too.. [wink]

Tip: The number of free()'s called should be the same as the number of malloc()'s called.

Lesson to be learned: When you create the string, which you THINK is inside the struct, it is actually somewhere else in the System Memory (RAM). As it is actually a pointer, pointing to a chunk of data. And the struct contains the POINTER and not the chunk of data.

##### Share on other sites
He does release the string, but not the struct that holds the string.

##### Share on other sites
Heh tnx :) You just in time, a few minutes ago I used memory debug functions _CrtDumpMemoryLeaks, and I did actually found a memory leak :). Well after a few moments of code debugging I realised that 8 bytes * 3 wasn't freed, so after a quick search i noticed, that i didn't free the structure I pointed to, instead of this it pointed to other. So this is what should be done :

//frees memory usedvoid FreeWords(){    //temp value used for freeing memory	struct WSTRUCT *temp;	while(words != NULL)   {      if(words->word != NULL)	  free(words->word);      	  temp = words;      	  //we select next item in the list	  words = words->next;	  //but we need to free current item	  free(temp);   }	//kill it	temp = NULL;}

I actually use temp value to store the current word pointer, then i update word to next value and finally free the temp structure i stored moments ago. It is absolutely clear now :)

Thank's again :)

##### Share on other sites
Oops, my bad. I guess I have my imagination back! Well, I'm gonna play with him, later y'all!

##### Share on other sites
If you're going to be fiddling with the list a lot, I'd advise not freeing the cells when you remove them from the list. Instead maintain a second list of free cells. Typically, that will greatly reduces the time spent allocating and deleting cells.

Also, don't ever use sprintf. Either use strndup or snprintf, if that's available.

  // Don't ever do this:  words->word = (char *) malloc(length);  sprintf(words->word, "%s", words);  // Use snprintf:  words->word = (char *) malloc(length);  snprintf(words->word, length, "%s", words);  // Or strndup:  words->word = strndup(words, length);

Note that the length of a string is the number of characters, plus 1 for the terminating NUL character. You may have already dealt with that outside the function, but it's probably better to ensure that you have enough room in this one place than to have to remember to add 1 everywhere the function is called.

In addition, (sizeof char) is guaranteed to be 1. So there's no need to check it.

##### Share on other sites
Quote:
 If you're going to be fiddling with the list a lot, I'd advise not freeing the cells when you remove them from the list. Instead maintain a second list of free cells. Typically, that will greatly reduces the time spent allocating and deleting cells.

That's really interesting,but when dealing with string i still have to allocate new string because i don't know what size of next string would be.If I would have a fixed size of struct then it would be really efficient :). Nice advice.

Quote:
 Also, don't ever use sprintf. Either use strndup or snprintf, if that's available.

Maybe you can explain why sprintf is so bad ?

Quote:
 Note that the length of a string is the number of characters, plus 1 for the terminating NUL character. You may have already dealt with that outside the function, but it's probably better to ensure that you have enough room in this one place than to have to remember to add 1 everywhere the function is called.

Yep i already had a bug once with size of array less than required to store string. But I would think of something , strlen() is not really the thing to play with :)

Quote:
 In addition, (sizeof char) is guaranteed to be 1. So there's no need to check it.

Yep i know that too, just working out the style, still :)

Thank's again

##### Share on other sites
Quote:
Original post by DMINATOR
Quote:
 Also, don't ever use sprintf. Either use strndup or snprintf, if that's available.

Maybe you can explain why sprintf is so bad ?

AddWord("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", 1);

Although the memory you've allocated is only one byte long, sprintf doesn't know that. It'll write the entire word into memory, overwriting whatever's stored after the allocated memory. That will cause your program to misbehave or crash.

If you used strlen() to find the length of the word, rather than relying upon the parameter, then sprintf would be perfectly safe as you can be sure another thread wouldn't modify the string. But if you have some reason why strlen() wouldn't do, don't use sprintf.

##### Share on other sites
Quote:
Original post by Mayrel
Quote:
Original post by DMINATOR
Quote:
 Also, don't ever use sprintf. Either use strndup or snprintf, if that's available.

Maybe you can explain why sprintf is so bad ?

*** Source Snippet Removed ***
Although the memory you've allocated is only one byte long, sprintf doesn't know that. It'll write the entire word into memory, overwriting whatever's stored after the allocated memory. That will cause your program to misbehave or crash.

If you used strlen() to find the length of the word, rather than relying upon the parameter, then sprintf would be perfectly safe as you can be sure another thread wouldn't modify the string. But if you have some reason why strlen() wouldn't do, don't use sprintf.

##### Share on other sites
Quote:
 Original post by Mayrel(sizeof char) is guaranteed to be 1. So there's no need to check it.
this maybe completly irrelivant to the OP but not on some systems (i think unicode char == 2 bytes, ala java/C#) for the sake of portability (and argument) sizeof(type) is desirable over any hard-coded values.

a perfect example is sizeof(int) == sizeof(long) brecause long == int on MS compilers, even though it goes beyond reason that long should be larger than int.

with C/C++, you can hardly guarantee things ;)

##### Share on other sites
Yeah maybe you are right, and btw,

I can use

sprintf(words->word,"%.*s",length,word);

And it does write only the number of bytes specified in length, so I think there is no problems afterall to stick with sprintf() ?

##### Share on other sites
Quote:
Original post by silvermace
Quote:
 Original post by Mayrel(sizeof char) is guaranteed to be 1. So there's no need to check it.
this maybe completly irrelivant to the OP but not on some systems (i think unicode char == 2 bytes, ala java/C#) for the sake of portability (and argument) sizeof(type) is desirable over any hard-coded values.

a perfect example is sizeof(int) == sizeof(long) brecause long == int on MS compilers, even though it goes beyond reason that long should be larger than int.

with C/C++, you can hardly guarantee things ;)

nitpick [smile]

sizeof(char) is always 1 byte

You might be thinking of wchar_t which designates a wide-character or multibyte character.

##### Share on other sites
Quote:
 Original post by DMINATORYeah maybe you are right, and btw, I can usesprintf(words->word,"%.*s",length,word);And it does write only the number of bytes specified in length, so I think there is no problems afterall to stick with sprintf() ?

My tests show that sprintf behaves safely when using '*' with a string.

However, you should be aware of two things.

Firstly, the standard specifies that the width specifies the minimum width, not the maximum width. It explicitly states the value is never truncated even if the result is larger. So this use of sprintf is, although safe, contrary to the requirements of the standard: a later version of the library may correct the behaviour, and the program might then crash.

Secondly, when using '*' with a number, my tests show that sprintf behaves unsafely but correctly -- i.e. as the standard dictates. This means that if you get into the habit of expecting "%.*s" to truncate a string, you may later expect "%.*i" to truncate an integer. But it will not, and the program might then crash.

In short, it is best to avoid sprintf. It is one of those devious functions that may appear to be perfectly safe until it makes your program fall over go boom.

##### Share on other sites
Quote:
Original post by snk_kid
Quote:
Original post by silvermace
Quote:
 Original post by Mayrel(sizeof char) is guaranteed to be 1. So there's no need to check it.
this maybe completly irrelivant to the OP but not on some systems (i think unicode char == 2 bytes, ala java/C#) for the sake of portability (and argument) sizeof(type) is desirable over any hard-coded values.

a perfect example is sizeof(int) == sizeof(long) brecause long == int on MS compilers, even though it goes beyond reason that long should be larger than int.

with C/C++, you can hardly guarantee things ;)

nitpick [smile]

sizeof(char) is always 1 byte

You might be thinking of wchar_t which designates a wide-character or multibyte character.
i mentioned this only because java and C# do not define wchar_t, in those languages, the char type is unicode and thus 16bit so: C++ sizeof(char) != C# sizeof(char) literaly.

##### Share on other sites
Quote:
Original post by Mayrel
Quote:
 Original post by DMINATORYeah maybe you are right, and btw, I can usesprintf(words->word,"%.*s",length,word);And it does write only the number of bytes specified in length, so I think there is no problems afterall to stick with sprintf() ?

My tests show that sprintf behaves safely when using '*' with a string.

However, you should be aware of two things.

Firstly, the standard specifies that the width specifies the minimum width, not the maximum width. It explicitly states the value is never truncated even if the result is larger. So this use of sprintf is, although safe, contrary to the requirements of the standard: a later version of the library may correct the behaviour, and the program might then crash.

Secondly, when using '*' with a number, my tests show that sprintf behaves unsafely but correctly -- i.e. as the standard dictates. This means that if you get into the habit of expecting "%.*s" to truncate a string, you may later expect "%.*i" to truncate an integer. But it will not, and the program might then crash.

In short, it is best to avoid sprintf. It is one of those devious functions that may appear to be perfectly safe until it makes your program fall over go boom.

Thank's i didn't know that. Il be carefull with sprintf() :)