Using malloc and free in C/C++ and getting error HEAP CORRUPTION DETECTED

Question

I am having a problem when free(position) is run.

void printTree(nodeT node, int hieght)
{
    int *position;
    position = (int*)malloc(hieght * sizeof(int*));
    for (int i = 0; i <= hieght; i++)
    {
        position[i] = 0;
    }
    BOOLEAN DONE = FALSE;
    while (DONE == FALSE)
    {
        printMoveDown(&node, position);
        printLEAFNode(&node, position);
        DONE = printMoveUp(&node, position);
        printSingleKey(&node, position);
    } 
    free(position);
    position = NULL;
}

The full error message I get from VS2010 is HEAP CORRUPTION DETECTED: after normal block (#64) at 0x00031390. CRT detected that the application wrote to memory after end of heap.

The debugger says the problem occurs while at in: dbgheap.c

extern "C" void __cdecl _free_dbg_nolock
line 1376:  if (!CheckBytes(pbData(pHead) + pHead->nDataSize, _bNoMansLandFill, nNoMansLandSize))
                if (pHead->szFileName) {..}
                else { this is where the program stops }

I tried setting up the same situation with less stuff going on to see if I could narrow the problem down.

void function (int y)
{
    int *x;
    x = (int*)malloc(y * sizeof(int*));
    free(x);
    x = NULL;
}

This is the same thing as above without the for loop and while loop. This works. Removing the for loop is what made it work. I don't know why. I looked up what the CRT was but it was all pretty new concepts to me and I assume that I can solve this problem without knowing about these CRTs.

The for loop assigns values to the memory allocated for position, beyond that I can't think of why this causes a problem.... actually now that I think about it. I changed the loop to be height + 1 which fixed the problem.

Answer 1

It should be:

position = malloc(hieght * sizeof(int));

or:

position = malloc(hieght * sizeof *position);

It's undefined behavior the way you have it. You're probably just getting lucky because int and int* are the same size.

And the typical correct way to write the loop is:

for (int i = 0; i < hieght; i++)
{
    position[i] = 0;
}

You can also use calloc here:

position = calloc(hieght, sizeof *position);

and the memory will be zeroed for you, so you don't have to loop.

Also, if this is really C, the cast is superfluous.

Answer 2

I think the problem in the loop is the <= which really should be <. Consequently the loop goes round one time too many and corrupts the next item on the heap!