Freeing elements of a void pointer array

Question

I am debugging some code written by someone else which has several memory leaks detected by Valgrind. Part of the code which I believe is causing the problems involves void pointers:

int noElements = 100;
void** ptr = malloc( noElements * sizeof(void*));
for(int i = 0; i < noElements; i++)
{
    ptr[i] = malloc(sizeof(void*));
}

//Operation filling the array and doing calculation

free(ptr); //This will not clear the underlying array causing the memory leak?

I created a small test program to check the error is where I thought it was (not freeing each ptr[i] before calling free(ptr);), but I am getting errors when trying to free elements. My test program looks like this:

int main()
{
    int num = 10000000; //Large array so I can clearly see memory usage
    void** ptr = (void**) malloc(sizeof(void*) * num);

    for (int i = 0; i < num; i++)
    {
        ptr[i] = malloc(sizeof(void*));
        ptr[i] = &i;    //If I never point elements at anything, this code works
    }

    for (int i = 0; i < num; i++)
    {       
        free(ptr[i]);       //Crashes here if the elements have been pointed at a variable
    }
    free(ptr);  

    return 0;
}

Why would calling free on each array element cause the program to crash if the pointers have actually been assigned?

[Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). — Sourav Ghosh, Apr 10 '17 at 09:31
If you replace `ptr[i] = &i;` with `*(ptr[i]) = &i;`, then your test program will work (assuming the `malloc` calls do not return `NULL`). However, the pointer stored in `*(ptr[i])` will be pointing to an invalid location `&i` after the `for` loop. — Ian Abbott, Apr 10 '17 at 09:34
@IanAbbott type of `ptr[i]` is `void*`. type of `*(ptr[i])` is `void`. this can't use. — BLUEPIXY, Apr 10 '17 at 09:39
@BLUXPIXY Yes, you are correct. It should be `*(void **)(ptr[i]) = &i;`. — Ian Abbott, Apr 10 '17 at 09:44

Sourav Ghosh · Answer 1 · 2017-04-10T09:58:36.947

In your code

  ptr[i] = &i;

creates three issues.

It makes the actual returned pointer by malloc() to be lost (you're overwriting it), so you have no shot at free()-ing it later, leading to memory leak.
i is a local scoped variable (the scope of for loop body), and you're storing the address of the variable to be used outside the scope (i.e., after the lifetime is over). Outside the scope attempt to access the address of it will invoke undefined behavior.
(Ignoring point 2) Attempt to free() the ptr[i] will cause undefined behavior again, as the pointer is not returned by memory allocator function.

score 1 · Accepted Answer · answered Apr 10 '17 at 09:33

Your pointers have been assigned a value which is not what you want.

for (int i = 0; i < num; i++)
{
    ptr[i] = malloc(sizeof(void*));
    ptr[i] = &i;    //If I never point elements at anything, this code works
}

First you assign a valid memory address that is returned by malloc(). This address could be free'd using free(). But then you assign another address. The address of your local variable i. This memory location was not allocated using malloc() and hence cannot be free'd. By assigning this address you've lost the dynamically allocated address causing a memory leak.

Your comment is wrong. You already assigned a value that ptr[i] points to some valid address. You don't need to assign again. And you should not.

That makes a lot of sense. I was assigning a new address to `ptr[i]` to do something with the pointer, when actually to use the pointer I just cast it and use it as an int pointer. Thanks! — MikeS159, Apr 10 '17 at 09:36

Freeing elements of a void pointer array

2 Answers2