Heap corruption when freeing 2D dynamic array

Question

I have a simple programa that populates a 2D dynamic char array. Allocation works as usual. My problem relies on freeing arrays. I'm using pure C on VC++ 2008.

This is the piece of code when I allocate and initializes my arrays:

char** messsages = (char**)malloc(5*sizeof(char*));
initValorArrayMsgs(messsages, 5);

insertMsgToArray(5, messsages , "Test message.");

void insertMsgToArray(int totalLines, char** msgsArray, const char* msgToInsert)
{
    int line = 0;
    int size= strlen(msgToInsert);

    for(; line < totalLines; line ++)
    {
        if(strlen(msgsArray[line ]) == 0)
        {
            msgsArray[line ] = (char*)malloc(sizeof(char) * size);
            strcpy(msgsArray[line], msgToInsert);
            break;
        }
    }
}

And this is the code where I free my arrays

void freeArrayMsgs(char** arry, int lines)
{
    int i = 0;
    for(; i < lines; i++)
    {
        if(strlen(arry[i]) == 0){
            break;
        }
        free(arry[i]);
    }
    free(arry);
}

When program try to free the first array, it raises a Heap Corruption Exception.

Reading some posts on SO, I'm corretly freeing my array. So, why I'm getting the hep corruption exception?

`msgsArray[line ] = (char*)malloc(sizeof(char) * size);` needs to have `(size+1)` instead of `size` to accommodate the null terminator of the string. As a side note, why don't you put your loop initializers right in the `for` loop? For example, `for ( i = 0; i < lines; i++ )` rather than doing the `i = 0;` separately and doing `for ( ; i < lines; i++ )`? — lurker, Aug 29 '13 at 15:14
As well as the above bug, note that you should not cast the result of malloc in C. — Paul R, Aug 29 '13 at 15:15
To answer the separate `for` loop question, he is probably not using `-std=c99`. As he needs to declare the value separately, he might as well set initialize it at the same time. — Matt Bryant, Aug 29 '13 at 15:20
@learner: see http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc — Paul R, Aug 29 '13 at 17:01

score 3 · Accepted Answer · answered Aug 29 '13 at 15:18

A couple things:

You need to malloc(size + 1) to fit the null-terminator.
There is no need to do sizeof(char)- sizeof is defined in terms of char, so that will always be 1.
You shouldn't cast the result of malloc - it is unnecessary and may hide problems.
As msgsArray is not initialized, there is no guarantee strlen(msgsArray[line]) == 0. In fact, it probably won't. Use calloc instead of malloc if you want this to be true.
In general, use strncpy instead of strcpy - it's safer.
Rather than using strlen, it would be a better idea to initialize the char*s to null and check for null. What if the first msgToInsert is an empty string, but the rest are real strings? Your code will exit the loop because strlen(msgs[0]) == 0, which checking for null instead would fix.

Solved, the heap corruption was occurring because I was missing to fit the null-terminator. Right to the point Matt. — learner, Aug 29 '13 at 17:02

score 0 · Answer 2 · edited Aug 29 '13 at 15:46

0

Two things jump out at me:

The inner logic of freeArrayMsgs seems suspect: On the first NULL pointer, it stops looping.
```
 for ( ;  i < lines;  i++)  
 {  
     if (strlen (arry[i]) == 0)  
         break;  
     free (arry[i]);  
 }
```
Maybe that is okay, but if lines is the maximum ever used, it could be important to get them all. Note that it is perfectly fine to free even NULL pointers—doing so is a no-op.
If the code reuses the pointers, it really ought to NULL them out after free(). If the same philosophy implied by the bit of code I see persists throughout the code, you are probably reusing a freed pointer because the stored pointer is not NULL. That is easily fixed:
```
for ( ;  i < lines;  i++)  
{  
    free (arry[i]);  
    arry[i] = NULL;  
}
```

edited Aug 29 '13 at 15:46

Mark Hurd

10,665
10
68
101

answered Aug 29 '13 at 15:34

wallyk

56,922
16
83
148

From a quick play it looks like SO doesn't like code formatting within list elements - I deleted your 1. and 2. and code formatting started working. – Nigel Harper Aug 29 '13 at 15:42
@wallyk You just needed to add more indentation. – Mark Hurd Aug 29 '13 at 15:47
@MarkHurd I've inserted break because I want to not delete anything but allocated memory. An ok, I forgot to put null after that. But the problem still there. – learner Aug 29 '13 at 16:55
I only corrected the indentation to get the formatting to look right. You may want to @comment to others here. – Mark Hurd Aug 30 '13 at 07:49

Heap corruption when freeing 2D dynamic array

2 Answers2