Dynamic array of structs in C

Question

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;
}

Answer 1

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);

Answer 2

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);   
  

Answer 3

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

Answer 4

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