Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Based on some code on internet, I implemented a dynamic array of structures in C.

I am really interested in some feedback on this. Maybe there are some places where it can cause memory leaks or other pitfalls? (I am also concerned if there can be situation that I free a pointer twice in my case, and how to avoid that)

#include "stdafx.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct
{
    int ID;
    char * name;
} Student;

// array of structs
typedef struct
{
    Student *array;
    size_t used;
    size_t size;
} Array;

void initArray(Array *a, size_t initialSize)
{
    // Allocate initial space
    a->array = (Student *)malloc(initialSize * sizeof(Student));

    a->used = 0;           // no elements used
    a->size = initialSize; // available nr of elements

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

// Add element to array
void insertArray(Array *a, Student element) 
{
    if (a->used == a->size)
    {
        a->size *= 2;
        a->array = (Student *)realloc(a->array, a->size * sizeof(Student));
    }

    // Copy name
    a->array[a->used].name = (char*)malloc(strlen(element.name) + 1);
    strcpy(a->array[a->used].name, element.name);

    // Copy ID
    a->array[a->used].ID=element.ID;

    a->used++;
}

void freeArray(Array *a) 
{
    // Free all name variables of each array element first
    for(int i=0; i<a->used; i++)
    {
        free(a->array[0].name);
        a->array[0].name=NULL;
    }

    // Now free the array 
    free(a->array);
    a->array = NULL;

    a->used = 0;
    a->size = 0;
}

int main(int argc, const char * argv[])
{

    Array a;
    Student x,y,z;

    x.ID = 20;
    x.name=(char*)malloc(strlen("stud1") + 1);
    strcpy(x.name,"stud1");

    y.ID = 30;
    y.name=(char*)malloc(strlen("student2") + 1);
    strcpy(y.name,"student2");

    z.ID = 40;
    z.name=(char*)malloc(strlen("student3") + 1);
    strcpy(z.name,"student3");

    // Init array
    initArray(&a, 5);   

    // Add elements 
    insertArray(&a, x);  
    insertArray(&a, y);   
    insertArray(&a, z);   

    // Print elements
    printf("%d\n", a.array[0].ID);        
    printf("%s\n", a.array[0].name);  
    printf("%d\n", a.array[1].ID);   
    printf("%s\n", a.array[1].name);   
    printf("%d\n", a.array[2].ID);   
    printf("%s\n", a.array[2].name);  
    printf("%d\n", a.array[3].ID);   
    printf("%s\n", a.array[3].name);   

    // Free array
    freeArray(&a);

    free(x.name);
    free(y.name);
    free(z.name);

    return 0;
}
share|improve this question

4 Answers 4

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 the freeArray and main methods
  • Each free(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 as free(a->array[0].name); because insertArray 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 set a->array[0].name=NULL; after freeing it, you don't free it twice.

But, you did fail to free the memory associated with a->array[i].name for values of i 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 your a->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 0
for(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 contiguous
memset(&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 twice
foo = NULL;
// this is useless and strange, but harmless
free(foo);

This is not safe:

void* foo = malloc(10);
void* bar = foo;
free(foo);
// protect against freeing twice
foo = NULL;
// this is useless and strange, but harmless
free(foo);
// but this is dangerous, illegal, undefined, etc.
// because bar is now pointing to memory that has already been freed
free(bar);
share|improve this answer
    
There should be free(a->array[i].name);. Also tricky case is if say a->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 –  user38434 Mar 18 at 15:12
1  
@dmcr_code Well done. –  ChrisW Mar 18 at 15:13
    
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? is memset in the initArray method fine for that? (probably not.. need to think a bit about it myself more) –  user38434 Mar 18 at 15:15
    
"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.. :( –  user38434 Mar 18 at 15:22
    
You called free(a->array[0].name); but never call free(a->array[1].name); –  ChrisW Mar 18 at 15:24

I have 3 suggestions.

  • If you need to allocate memory and initialize it to zero use calloc.
    Using calloc is better than using malloc + memset

    So change your initArray 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);   
    
share|improve this answer
    
For your second point, you should add "unless it is a loop index/iterator". –  Morwenn Mar 18 at 9:32
    
Yes, I was also interested more from memory management point of view etc.. –  user38434 Mar 18 at 10:51

It looks like you tried to fix a bug in Rev 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 by strcpy() could be replaced by strdup().

share|improve this answer
    
Yes you are right, should be i instead of 0 –  user38434 Mar 18 at 14:31
    
Yes just I am more interested in cases when/if there can be for example freeing unallocated memory or freeing already freed memory ... –  user38434 Mar 18 at 14:43
2  
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, and strdup may not exist. –  Morwenn Mar 18 at 14:44

Don't forget that the realloc call in insertArray() can fail, and if it does, your array will be corrupt.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.