CODEWITHSUNDEEP
cpointersmemory-managementmallocdynamic-memory-allocation
m4design
m4design

Reputation: 2180

Simple dynamic memory allocation bug

I'm sure you (pros) can identify the bug's' in my code, I also would appreciate any other comments on my code.

BTW, the code crashes after I run it.

#include <stdlib.h>
#include <stdio.h>
#include <stdbool.h>

typedef struct
{
    int x;
    int y;
}  Location;

typedef struct
{
    bool walkable;
    unsigned char walked; // number of times walked upon
} Cell;

typedef struct
{
    char name[40];  // Name of maze
    Cell **grid;    // 2D array of cells
    int rows;       // Number of rows
    int cols;       // Number of columns
    Location entrance;
} Maze;


Maze *maz_new()
{
    int i = 0;

    Maze *mazPtr = (Maze *)malloc(sizeof (Maze));

    if(!mazPtr)
    {
        puts("The memory couldn't be initilised, Press ENTER to exit");
        getchar();
        exit(-1);
    }
    else
    {
        // allocating memory for the grid
    mazPtr->grid = (Cell **) malloc((sizeof (Cell)) * (mazPtr->rows));

    for(i = 0; i < mazPtr->rows; i++)
        mazPtr->grid[i] = (Cell *) malloc((sizeof (Cell)) * (mazPtr->cols));
    }

    return mazPtr;
}


void maz_delete(Maze *maz)
{
    int i = 0;

    if (maz != NULL)
        {
            for(i = 0; i < maz->rows; i++)
                free(maz->grid[i]);

            free(maz->grid);
        }
}


int main()
{
    Maze *ptr = maz_new();
    maz_delete(ptr);

    getchar();
    return 0;
}

Thanks in advance.

Upvotes: 0

Views: 438

Answers (3)

David Woo
David Woo

Reputation: 791

in maz_delete you need another call to free for the maz structure itself

void maz_delete(Maze *maz) { int i = 0;

 if (maz != NULL)
 {
     for(i = 0; i < maz->rows; i++)
         free(maz->grid[i]);

     free(maz->grid);

     free(maz);
 }

}

Upvotes: 0

Marcelo Cantos
Marcelo Cantos

Reputation: 185852

How big is the maze supposed to be? You never initialise rows and cols.

Your big problem, though, is that you use sizeof(Cell) when initialising grid, but it should be sizeof(Cell*).

On my architecture, Cell is only two bytes, whereas Cell * is eight bytes, which means that you haven't allocated enough space. As you fill in this array, you are writing past the end and into some other chunk of allocated memory, at which point all bets are off. The memory allocator corrupts the contents of the array at some point, and you are left trying to free rubbish.

Upvotes: 0

wds
wds

Reputation: 32283

In addition to the problem Marcelo pointed out, I spotted this:

mazPtr->grid = (Cell **) malloc((sizeof (Cell)) * (mazPtr->rows));

You're allocating 10 Cells, this returns a pointer to the first Cell, which would be of type Cell *. A Cell struct is a bool and an unsigned char, which, depending on compiler and target architecture, might not be allocated big enough to hold a Cell * (which could be a 64 bit pointer). When later initialising your grid array, you probably end up writing past the ends of the array.

So, try allocating 10 sizeof (Cell *) in your grid. And fix the initialisation problem there of course.

Upvotes: 1

Related Questions