Seg Fault upon accessing malloc'd 2d array in class

Question

This is meant to be a class that contains a 2d array of chars called "map". First, I allocate [mapDimension] char*s to a char**. Then, I allocate [mapDimension] chars to each char*. I do it this way rather than the static: char map[mapDimension][mapDimension] because while that works for small numbers, once mapDimension gets above around 2500, it also segfaults. I wanted to allocate the memory on the heap rather than the stack so that I could hold more.

The last two lines in the constructor are to initialize every char in the 2d array to 0 and then set the center one to 5.

#define mapDimension 1000    

class Field 
{
private:
    int i;
public:
    char** map = (char**)malloc(mapDimension * sizeof(char*));
    Field()
    {
        for(i = 0; i < mapDimension; i++)
            map[i] = (char*)malloc(mapDimension * sizeof(char));
        memset(map, 0, mapDimension*mapDimension*sizeof(char));
        map[mapDimension/2][mapDimension/2] = 5;
    }
};

int main()
{
    Field field;
    return 0;
}

This dynamic approach also segfaults. What am I doing wrong?

This is really really really bad C++. Go read a [good book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). — Baum mit Augen, Jul 15 '15 at 18:01
It may be real bad C++, but it is a valid and fairly well formed question. Why all the neg votes? — Michael Dorgan, Jul 15 '15 at 18:05

score 4 · Accepted Answer · edited Jul 16 '15 at 04:07

4

    memset(map, 0, mapDimension*mapDimension*sizeof(char));

You are clearing map array (array of pointers), so map[x] now contains NULL. You probably should use map[i] in loop to clear allocated memory.

BUT

When you allocate memory you should deallocate it (there is malloc, no free visible)
In C++ use new[] and delete[] instead of malloc
Even better - use std::vector<std::vector<char>>

edited Jul 16 '15 at 04:07

Drew Dormann

59,987
13
123
180

answered Jul 15 '15 at 17:59

Hcorg

11,598
3
31
36

1

I can't decide if this is an answer to the question or a comment. I suppose it depends on the interpretation of *"What am I doing wrong?"* – Drew Dormann Jul 15 '15 at 18:01
vector of vector of char? not a string? – Michael Dorgan Jul 15 '15 at 18:04
@MichaelDorgan depending what author wants to achieve. I wanted to show how to create multidim in C++ :) – Hcorg Jul 15 '15 at 18:06
@DrewDormann - true, it would be just less readable as comment... also question is the one of "please do my debugging for me" – Hcorg Jul 15 '15 at 18:07
Was the second half a resonse for me? – Michael Dorgan Jul 15 '15 at 18:09
@Hcorg based on your comment, I would claim **that this answer should be a comment**. It is a very helpful comment, nonetheless. Please don't take it personally. – Drew Dormann Jul 15 '15 at 18:11
@DrewDormann - don't worry, I won't. As I said - I was wondering what it should be (ans/comm), but I'm still learning proper 'feels' and 'smells' of SO. – Hcorg Jul 15 '15 at 18:27

Vlad from Moscow · Answer 2 · 2015-07-15T18:12:44.203

This statement is invalid

memset(map, 0, mapDimension*mapDimension*sizeof(char));

you allocated several extents of memory not one contiguous storage area.

You have to use a loop. For example

    for ( i = 0; i < mapDimension; i++ ) memset( map[i], 0, mapDimension );

And your class has to have a destructor that to free the allocated memory.

For example

~Field()
{
    for( i = 0; i < mapDimension; i++ ) free( map[i] );
    free( map );
}

It would be better to make mapDimension like a static constant variable of type size_t instead of using a macro definition.

Take into account that in C++ you should use operator new instead of C function malloc.

Also you could simply use container std::vector<std::string> instead of the manually allocated arrays.

AnT stands with Russia · Answer 3 · 2015-07-15T18:12:58.137

Firstly, it is possible to allocate your array so that you'll be able to memset the entire array in one shot. But for that you have to allocate the second-level memory as one continuous memory block and then distribute it between the rows
```
Field()
{
  char *data_memory = (char *) malloc(mapDimension * mapDimension * sizeof(char));

  for (i = 0; i < mapDimension; i++)
    map[i] = data_memory + i * mapDimension;
  ...
}
```
Secondly, if you changed your allocation approach to a "single block allocation" (as described above) then in order to memset the whole thing, you have to do it as follows
```
memset(map[0], 0, mapDimension * mapDimension * sizeof(char));
```
or
```
memset(&map[0][0], 0, mapDimension * mapDimension * sizeof(char));
```
Note the fist argument of memset.

But if you kept to your current allocation approach, then you can forget about using memset on the entire array. Your array is not continuous in memory. You can't memset the whole thing. All you can do is memset each row individually
```
for (i = 0; i < mapDimension; i++)
  memset(map[i], 0, mapDimension * sizeof(char));
```
Thirdly, declaring i as a member fo the class, instead of using a local variable i is bad idea. Also, using in-class initializer for the frist malloc is a bad idea too. Also, in C++ you should prefer to use new/new[] over malloc, not even mentioning that you should prefer to use std::vector over manual array memory management.

Seg Fault upon accessing malloc'd 2d array in class

3 Answers3