I am also concerned if there can be situation that I free a pointer twice in my case

... and ...

Yes just I am more interested in cases when/if there can be for example freeing unallocated memory or freeing already freed memory ...

After a quick inspection it doesn't appear that you free memory more than once:

• free statements are only in thefreeArray andmain methods
• Eachfree(a->array[0].name); is different because each name is allocated using its own malloc
• free(a->array) is only called once
• freeArray is only called once
• free(x.name); doesn't free the same memory asfree(a->array[0].name); becauseinsertArray allocates new memory for each name

and how to avoid that

Something which can help (though not guarantee) is to assign NULL to the pointer after you pass it to free.

• It can help, because calling free on a previously-nulled pointer will harmlessly do nothing
• It's not a guarantee, because you might have more than one pointer pointing to the same memory

dmcr_code's comment below points out a bug. You wrote,

for(int i=0; i<a->used; i++){    free(a->array[0].name);    a->array[0].name=NULL;}

This should be,

for(int i=0; i<a->used; i++){    free(a->array[i].name);    a->array[i].name=NULL;}

Because you seta->array[0].name=NULL; after freeing it, you don't free it twice.

But, you did fail to free the memory associated witha->array[i].name for values ofi larger than 0.

But then how do I protect against that - when array[i].name can contain random value and I try to free it?

To protect yourself:

• Either, don't let it contain a random value (e.g. ensure that it's either a valid pointer, or zero)
• Or, don't use it (e.g. ensure that youra->used logic is correct so that you don't touch elements which you haven't used/initialized).

is memset in the initArray method fine for that?

memset is good:

• You could use calloc instead of malloc to avoid having to use memset as well
• You could use memset on the whole array at once instead of using memset on each element of the array

memset in initArray isn't enough. It's enough to begin with, but there's a realloc in insertArray. So to be good enough, you'd also need to use memset after realloc (to memset the as-yet-unused end of the newly-reallocated array; without using memset on the beginning of the reallocated array, which already contains valid/initialized/used elements).

the only unclear part that remains from your response is how to memset realloced array

Your current code in initArray says,

// Initialize all values of the array to 0for(unsigned int i = 0; i<initialSize; i++){    memset(&a->array[i],0,sizeof(Student));}

Another way to do that would be:

// Initialize all elements of the array at once: they are contiguousmemset(&a->array[0], 0, sizeof(Student) * initialSize);

The memset statement to add to insertArray would be:

if (a->used == a->size){    a->size *= 2;    a->array = (Student *)realloc(a->array, a->size * sizeof(Student));    // Initialize the last/new elements of the reallocated array    for(unsigned int i = a->used; i<a->size; i++)    {        memset(&a->array[i],0,sizeof(Student));    }}

Or:

if (a->used == a->size){    a->size *= 2;    a->array = (Student *)realloc(a->array, a->size * sizeof(Student));    // Initialize the last/new elements of the reallocated array    memset(&a->array[a->used],0,sizeof(Student) * (a->size - a->used));}

and this comment: "It's not a guarantee, because you might have more than one pointer pointing to the same memory " would be nice if you can address that too

This is safe:

void* foo = malloc(10);free(foo);// protect against freeing twicefoo = NULL;// this is useless and strange, but harmlessfree(foo);

This is not safe:

void* foo = malloc(10);void* bar = foo;free(foo);// protect against freeing twicefoo = NULL;// this is useless and strange, but harmlessfree(foo);// but this is dangerous, illegal, undefined, etc.// because bar is now pointing to memory that has already been freedfree(bar);

• \$\begingroup\$There should befree(a->array[i].name);. Also tricky case is if saya->array[i].name contains some random garbage value and I try to free it right? (here checking for NULL won't help). ps. and I didn't I think fully understand your last bullet\$\endgroup\$

  user38434

  – user38434

  2014-03-18 15:12:04 +00:00

  CommentedMar 18, 2014 at 15:12
• 1
  \$\begingroup\$@dmcr_code Well done.\$\endgroup\$

  ChrisW

  – ChrisW

  2014-03-18 15:13:02 +00:00

  CommentedMar 18, 2014 at 15:13
• \$\begingroup\$Thanks for cheer up!. But then how do I protect against that - when array[i].name can contain random value and I try to free it? ismemset in theinitArray method fine for that? (probably not.. need to think a bit about it myself more)\$\endgroup\$

  user38434

  – user38434

  2014-03-18 15:15:10 +00:00

  CommentedMar 18, 2014 at 15:15
• \$\begingroup\$"But, you did fail to free the memory associated with a->array[i].name for values of i larger than 0" this I didn't get.. :(\$\endgroup\$

  user38434

  – user38434

  2014-03-18 15:22:01 +00:00

  CommentedMar 18, 2014 at 15:22
• \$\begingroup\$You calledfree(a->array[0].name); but never callfree(a->array[1].name);\$\endgroup\$

  ChrisW

  – ChrisW

  2014-03-18 15:24:58 +00:00

  CommentedMar 18, 2014 at 15:24

I have 3 suggestions.

• If you need to allocate memory and initialize it to zero usecalloc.
  Usingcalloc is better than usingmalloc + memset

  So change yourinitArray function like:

  void initArray(Array *a, size_t initialSize){   // Allocate initial space   a->array = (Student *)calloc(initialSize , sizeof(Student));   a->used = 0;           // no elements used   a->size = initialSize; // available nr of elements}

• Single character variable names are very bad. Use proper names for variables and follow naming conventions.

• In your code you are only creating and adding 3 objects. But you are trying to print the details of 4th object. (Array index is starting from zero, so index 3 means 4th object)

  printf("%d\n", a.array[3].ID);   printf("%s\n", a.array[3].name);

• \$\begingroup\$For your second point, you should add "unless it is a loop index/iterator".\$\endgroup\$

  Morwenn

  – Morwenn

  2014-03-18 09:32:06 +00:00

  CommentedMar 18, 2014 at 9:32
• \$\begingroup\$Yes, I was also interested more from memory management point of view etc..\$\endgroup\$

  user38434

  – user38434

  2014-03-18 10:51:25 +00:00

  CommentedMar 18, 2014 at 10:51

It looks like you tried to fix a bug inRev 4. (Editing the question after asking it is discouraged on Code Review.) However, it's still wrong. Hint:i is an unused variable in the for-loop.

malloc() followed bystrcpy() could be replaced bystrdup().

• \$\begingroup\$Yes you are right, should bei instead of 0\$\endgroup\$

  user38434

  – user38434

  2014-03-18 14:31:38 +00:00

  CommentedMar 18, 2014 at 14:31
• \$\begingroup\$Yes just I am more interested in cases when/if there can be for example freeing unallocated memory or freeing already freed memory ...\$\endgroup\$

  user38434

  – user38434

  2014-03-18 14:43:23 +00:00

  CommentedMar 18, 2014 at 14:43
• 2
  \$\begingroup\$strdup is no standard C. If I'm not mistaken, it's a POSIX function. Since there is a#include "stdafx.h", I bet that the system used is Windows, andstrdup may not exist.\$\endgroup\$

  Morwenn

  – Morwenn

  2014-03-18 14:44:45 +00:00

  CommentedMar 18, 2014 at 14:44

Don't forget that therealloc call ininsertArray() can fail, and if it does, your array will be corrupt.