What's wrong with this memory allocation

Question

Hi guys, I tried to make a dynamic 2D array of char as follow:

char** ppMapData = (char**)malloc(sizeof(char*)*iMapHeight);
for (int i=0; i< iMapHeight; i++)
{
    ppMapData[i] = (char*)malloc(sizeof(char)*iMapWidth);
    //do something
}

// do something

for (int i=0; i<iMapHeight; i++)
    free(ppMapData[i]);
free(ppMapData);

It looks fine to me; however, when it comes to run time, my program crash at the line which calls free(ppMapData[i]). Any ideas what is the problem here. Thank you very much.

Answer 1

At a quick glance, the frees look fine. The next thing you should test is to make sure you aren't overrunning any of these arrays past the end. This can cause problems with memory allocation systems.

If you are using visual studio, you can call _CrtCheckMemory to help verify that you aren't trashing things. That call only works in a DEBUG build.

Answer 2

Did you check for NULL values after malloc?

Answer 3

You've tagged your question “C++”. Why not use appropriate C++ constructs instead? E.g. a vector<vector<char> > or even (might I suggest) a vector<string>?

EDIT Oh well, a vector<string> is probably not what you want. Still, I strongly suggest a C++-ier solution.

Answer 4

I don't see the problem at first glance, but it doesn't mean it's not there. Please make sure you aren't doing a double free by freeing the memory somewhere in the "do something" section.

Answer 5

If it's a 2D array with fix size in both dimensions, I always prefer:

char* array = (char*)malloc(iMapHeight*iMapWidth);

And accessing it with:

char val = array[height*iMapWidth + width];

Wrap that in a macro if you prefer:

#define GET_VAL(h, w) array[h*iMapWidth + w]

The multiplication will be faster than de-referencing two pointer to find the value.

Answer 6

A few possible answers:

1. Are you certain you are not overrunning the ends of your allocated arrays? malloc will store important information immediately after your allocated memory and if you overwrite it bad things will happen.

2. Are you certain you are not modifying the values in ppMapData? Attempting to free a pointer that was never allocated is generally a bad thing as well.

3. What is the value of i when your program crashed? Is it consistent? knowing this would be helpful and possibly instructive

Some stylistic comments:

• Check the return value of malloc (it will be NULL if it fails).

• Do not cast the return value of malloc (if you are indeed writing C and not C++).

Answer 7

Is iMapHeight / iMapWidth constant? If not, they could be changed in between, causing problems. Also, as suggested by others, check for null returns by malloc.

Answer 8

Assuming // do something is more than just the comment, take out the

// do something

code and see if it still crashes. Add stuff back one at a time till it crashes again

Answer 9

From the code in the question, it doesn't seem like you are checking if the malloc has succeeded. Check that malloc is not returning NULL as a first step, It might not be your problem as the

//do something

is probrably using the new allocated memory, but if it isn't, malloc could be failing with out you knowing untill it is derefrenced with the free. (I know it's unlikely for malloc to fail unless you are developing for a low memory enviroment but it is still worth checking)

Secondly what is the i value when it crashes, is it almost the same?

Finally can you get us the core dump the problem would be easier to identify :)

Answer 10

Does it still crash if you remove the "do something" code? If it doesn't, it probably means you're breaking memory in the "do something" code, and that's where your bug lies, not in the code above.

If you're in a platform that supports it, run it through Valgrind, it's priceless to find subtle errors.

Answer 11

As an alternative to joemucchiello's,

char **ppMapData = malloc(iMapHeight * sizeof(char*) +
                          iMapHeight * iMapWidth * sizeof(char)));

for (int i = 0; i < iMapHeight; i++) {
    ppMapData[i] = (char *)(ppMapData + iMapHeight) + i * iMapWidth;
    /* do something */
}

/* do something */

free(ppMapData);

This makes one large allocation, containing both the array-of-pointers and the array-of-array-of-characters.

However, as many others have stated, the code you have posted runs fine. There must be something in the "// do something" segment that you haven't posted which is causing this error.

For example, if you've already freed ppMapData[i], you can't free it again. Some people would avoid this by setting ppMapData[i] = NULL immediately following any free(ppMapData[i]): this works because a free(NULL) is legal and does nothing. My preference is not to do this, and to fix any design that would lead you to a double-free situation.