Sign in to follow this  
DMINATOR

C and struct lists

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 AddWord(char* word,int length);
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;
   }
}


void AddWord(char* word,int length)
{

	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()
{

  //add new word
  AddWord("word1",6);

  AddWord("word2",6);

  printf("ALLOK\n");

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

Share this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by Pipo DeClown
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.


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

Share this post


Link to post
Share on other sites
Quote:
Original post by Pipo DeClown
However, 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 this post


Link to post
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 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.


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

Thank's for the program advise.


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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by Washu
Simple really, look at your free function
you 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 this post


Link to post
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 used
void 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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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.


Yep i got your point, tnx for advice.

Share this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by DMINATOR
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() ?

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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by Mayrel
Quote:
Original post by DMINATOR
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() ?

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() :)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this