seg fault from 2d array allocation

Question

i have a struct "cell" defined as

typedef struct{
int id;
terrainType terrain;
} cell;

i then make a 2d array of cells with

cell** makeCellGrid(int sizeX, int sizeY)
{  
    cell** theArray;  
    int i;

    theArray = (cell**) malloc(sizeX*sizeof(cell*));  

    for ( i = 0; i < sizeX; i++)
    {
        theArray[i] = (cell*) malloc(sizeY*sizeof(cell));  
    }

   return theArray;  
}

at first i thought this was working fine but a few seg faults later i discovered that with some values (e.g. makeCellGrid(32, 87) ) it breaks. im fairly fresh with C pointers and memory junk and was hoping some one could point me in the right direction here.

with lower number bounds i had no issue accessing it with

map[i][j].id = x;

and so on

EDIT: forgot to add, from testing, the seg fault originate from

theArray[i] = (cell*) malloc(sizeY*sizeof(cell));

This code seems fine. Are you sure you don't swap the X and Y indices by accident? — , Feb 12 '13 at 06:47
Are you on a system such as Linux where you can use [`valgrind`](http://valgrind.org/)? If so, use it. The code you show is not self-evidently the problem. It is therefore likely related to how you are using the data that your code returns. — Jonathan Leffler, Feb 12 '13 at 06:50
(1) **[Take out the casts on your malloc() calls.](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc)** (2) Ensure you're including ``. If the first `malloc()`'ed address dereference you experience dies like this appears to from your edit-comment, you may well not be storing a full pointer, which is more common than you think when you hide an implied `malloc()` function declaration with a cast such as this. — WhozCraig, Feb 12 '13 at 07:29
Why don't you just malloc x*y instead of allocating all structs separately? Malloc (and free) is very costly... — Dariusz, Feb 12 '13 at 07:44
@DariuszWawer: Perhaps the y-vectors shall be used seperatly? — alk, Feb 12 '13 at 07:54
@alk possibly, but the function name suggests otherwise. It'll just be some kind of map used in a game. Moreover, using single memory block may improve code speed, because a large part of the map will always be loaded into cache, which wouldn't be possible for fragmented memory — Dariusz, Feb 12 '13 at 08:04
@DariuszWawer: I do agree that the way to allocate memory should best follow its use. — alk, Feb 12 '13 at 16:49

alk · Accepted Answer · 2013-02-12T17:02:27.970

The code lacks error checking for the malloc() system call.

So if the first call to malloc() failed the second one (in the loop) tries to assign memory to NULL which indeed leads to the segmentation violation your are witnessing.

You might consider modifing you code like so:

#include <stdlib.h>

typedef struct {
  int id;
  TerrainType terrain;
} CellType;

void freeCellGrid(CellType ** ppCells, size_t sizeX)
{
  size_t i = 0;
  for (; i < sizeX; ++i)
  {
    free(ppCells[i]);
  }

  free(ppCells);
}

CellType ** makeCellGrid(size_t sizeX, size_t sizeY)
{  
    CellType ** ppCells = malloc(sizeX * sizeof(*ppCells));  

    if (ppCells)
    {
      size_t i = 0;

      for (; i < sizeX; ++i)
      {
          ppCells[i] = malloc(sizeY * sizeof(**ppCells));  
          if (NULL == ppCells[i])
          {
            freeCellGrid(ppCells, i);
            ppCells = NULL;

            break;
          }
      }
   }

   return ppCells;  
}

Notes on my modifications:

Always check system calls for errors (in the case of malloc() on error NULL is returned)
Better use an unsigned type to access memory/array indicies; size_t is meant for this
In C there is no need to cast the value returned by a void * function like malloc()
Always try to initialise variables as soon as possible; un-initilaised variables very easily lead to "irrational" behaviour of the application
If working with pointers, it might be helpfull to 'code' the level of indirection into their names (I did this here by using the prefix pp to indicated that it's a 2-level indirection)
types are different from variables: One way to distinguish this is by starting type names using capitals (CellType) and variables using small letters (ppCells).
If allocating memory to a pointer and it matters that the size of the allocated memory some suits the pointer's type it's always more secure to use the (dereferenced) pointer itself as argument to the sizeof operator then some type. As the declaration of the pointer the memory is allocated to might be changed during develpment and the adjustment of the argument to malloc() will be forgotten. To cut it short: doing as I did is less error prone.
If encapsulating the dynamical creation of structures (including arrays) it is a could idea to also implement a method which de-allocates it (here: freeCellGrid()). Even better start of with coding this deallocator first, as then you have it by hand when coding the allocator's error handling (as shown for the second call to malloc()).

Thanks. really great post with lots of usable info. – TrewTzu Feb 12 '13 at 14:50 — TrewTzu, Feb 12 '13 at 14:50

seg fault from 2d array allocation

1 Answers1