Freeing array of struct

Question

I've done some research and couldn't find any answer to my problem.

I'm having problems with freeing my struct.

This is how i create my struct:

struct Structure * newStructure(int N) 
{
    struct Structure * structure;
    int i;
    structure = (struct Structure * ) malloc(N * sizeof(struct Structure));
    for (i = 0; i < N; i++) 
    {
        structure[i].i_Number = (int * ) malloc(sizeof(int));
        structure[i].c_Char = (char * ) malloc(sizeof(char));
        structure[i].c_Char[0] = '\0';
        structure[i].d_Float = (double * ) malloc(sizeof(double));
    }
    return structure;
}

Everything works to this point. Later I fill every variable with random values so that they are not empty.

I call my freeMemory function like this freeMemory(structure, amountOfStructures); And here is freeMemory function itself:

void freeMemory (struct Structure* structure, int N)
{
    int i;

    for( i=0 ; i<N ; i++ )
    {
        if (structure[i].i_Number!=NULL) free(structure[i].i_Number);
        if (structure[i].c_Char!=NULL) free(structure[i].c_Char);
        if (structure[i].d_Float!=NULL) free(structure[i].d_Float);
    }

    free(structure);
}

The free(structure) part works fine. But there are problems with the for loop and I have no idea what I'm doing wrong here.

@EDIT I'm adding my struct declaration:

struct Structure{
    int *i_Number;
    char *c_Char;
    double *d_Float;
};

@EDIT2 That's the function that initializes struct:

    struct Structure* randomizing (int N)
{
    struct Structure* structure = newStructure(N); int i;
    srand(time(NULL));

    for (i = 0; i < N; i++)
    {
        int _i; char _c; double _d;

         _i = rand()%1000000;
         _c = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" [rand () % 26];
         _d = 0;

        setStructureNumber(structure, i,(int*) _i);
        setStructureChar(structure, i, (char*) _c);
        setStructureDouble(structure, i, &_d);

// I'VE COMMENTED OUT THE MUTATORS ABOVE AND THE ERROR DOES NOT SHOW ANYMORE, SO THERES SOMETHING WRONG WITH THEM

    }

    return structure;
    }

And im calling it like this:

    struct Structure* structure;
    structure = randomizing(amountOfStructures);

The mutators used:

// Mutators
void setStructureNumber (struct Structure* structure, int p, int* num)
{
    if (structure[p].i_Number != NULL) free(structure[p].i_Number);

    structure[p].i_Number = (int*) malloc (sizeof(int));
    structure[p].i_Number = num;
}

void setStructureChar (struct Structure* structure, int p, char* str)
{
    if (structure[p].c_Char != NULL) free(structure[p].c_Char);

    structure[p].c_Char = (char*) malloc (sizeof(char));
    structure[p].c_Char = str;
}

void setStructureDouble (struct Structure* structure, int p, double* dou)
{
    if (structure[p].d_Float != NULL) free(structure[p].d_Float);

    structure[p].d_Float = (double*) malloc (sizeof(double));
    structure[p].d_Float = dou;
}

Answer 1

The most likely reason is that somewhere in your code you go out of bounds of the memory you allocated and thus destroy the integrity of the heap. A frequently encountered practical manifestation of such undefined behavior is a failure at free, when the library detects the problem with the heap.

Inside you allocation cycle you allocate just one object of each respective type for each field of your struct object. For example, you allocate only one character for c_Char field and initialize it with \0. This might suggest that c_Char is intended to hold a string (is it?). If so, then the memory you allocated is sufficient for an empty string only. If you do not reallocate that memory later, any attempts to place a longer string into that memory will break the integrity of the heap and trigger undefined behavior.

The same applies to other fields as well. However, without extra explanations from you it is not possible to say whether it is right or wrong. At least, you have to provide the definition of struct Structure. And you have to explain your intent. Why are you dynamically allocating single-object memory for struct fields instead of just making these objects immediate members of the struct?

---

The additional code you posted is completely and utterly broken.

Firstly you are calling your mutators as

setStructureNumber(structure, i,(int*) _i);
setStructureChar(structure, i, (char*) _c);
setStructureDouble(structure, i, &_d);

This does not make any sense. Why are you trying to convert integer value _i to pointer type??? If you want to obtain a pointer to _i, it is done as &_i. You already do it correctly in the very last call, where you pass &_d. Why are the first two calls different from the last one? What was your logic behind this?

Secondly, inside your mutator functions

void setStructureNumber (struct Structure* structure, int p, int* num)
{
    if (structure[p].i_Number != NULL) free(structure[p].i_Number);

    structure[p].i_Number = (int*) malloc (sizeof(int));
    structure[p].i_Number = num;
}

you are freeing old memory and allocating new memory. Why? Why don't just reuse the old memory to store the new value? (BTW, there's no need to check the pointer for null before calling free, because free will check it internally anyway.)

Thirdly, after allocating the new memory you immediately leak it by overriding the pointer value returned by malloc with the pointer value passed from the outside

structure[p].i_Number = num;

Again, this does not make any sense. This is actually what causes the crash on free - the pointers you pass from the outside are either meaningless random values (like your (int *) _i or (char *) _c)) or point to a local variable (like your &_d).

---

There's no way to "correct" your code without knowing what it is you are trying to do in the first place. There are just too many completely unnecessary memory allocations and reallocations and other illogical things. I would simply rewrite the mutator functions as

void setStructureNumber (struct Structure* structure, int p, int num)
{
    *structure[p].i_Number = num;
}

Note - no memory reallocations and the argument is passed by value.

The functions would be called as

setStructureNumber(structure, i, _i);
setStructureChar(structure, i, _c);
setStructureDouble(structure, i, _d);

But again, this is so vastly different from what you have that I don't know whether this is what you need.

Answer 2

Technically, there is nothing wrong with what you are doing (except the missing error checks on allocations, unnecessary casts of malloc results, and unnecessary NULL checking before calling free).

This should work fine, assuming that you pass the correct value of N, and that you do not free things more than once:

struct Structure * newStructure(int N) {
    struct Structure * structure = malloc(N * sizeof(struct Structure));
    for (int i = 0; i < N; i++) {
        structure[i].i_Number = malloc(sizeof(int));
        structure[i].c_Char = malloc(sizeof(char));
        structure[i].c_Char[0] = '\0';
        structure[i].d_Float = malloc(sizeof(double));
    }
    return structure;
}

void freeMemory (struct Structure* structure, int N)
{
    for(int i=0 ; i<N ; i++ )
    {
        free(structure[i].i_Number);
        free(structure[i].c_Char);
        free(structure[i].d_Float);
    }
    free(structure);
}

You can use a memory diagnostic tool such as valgrind to ensure that you do not freeing things more than once.

Answer 3

In your mutators you leak memory and then point to local variables (comments mine)

void setStructureChar (struct Structure* structure, int p, char* str)
{
    if (structure[p].c_Char != NULL) free(structure[p].c_Char);

// allocates new memory and points c_Char at it.
    structure[p].c_Char = (char*) malloc (sizeof(char));

// makes c_Char point to where `str` is pointing; now the allocated memory is leaked
    structure[p].c_Char = str;
}

When you later do free on structure[p].c_Char, it causes undefined behaviour because you called this function with a pointer to a local variable. You probably have undefined behaviour elsewhere too if you try to access c_Char anywhere before freeing it.

The other mutators have the same problem.

To "fix" this change structure[p].c_Char = str; to *structure[p].c_Char = *str;.

You also have blunders here:

setStructureNumber(structure, i,(int*) _i);
setStructureChar(structure, i, (char*) _c);

You meant &_i and &_c respectively. I would advise to remove all casts from your code. At best they are redundant; at worst (e.g. in these two lines) they hide an error which the compiler would diagnose.

Also remove all the NULL checks before free, they are redundant and make your code hard to read. Instead, do the NULL checks after calling malloc, and abort the program if malloc returned NULL.

However this whole setup seems like a ghastly design. You could pass the things by value to the mutators. And you could change your struct to not contain pointers, and therefore not need all this extra allocation.