Trying to reallocate a struct, c

Question

I've been struggling for a while now to create a ‫‪Gomoku‬‬ board. Here's the sturct of the board:

typedef struct Board
{
    int width;
    int height;
    char **board;
} Board;

Here's the constructor, that return a pointer to Board (BoardP):

BoardP createNewBoard(int width, int high)
{

    BoardP board = (BoardP) malloc(sizeof(Board));

    if (board == NULL)
    {
        reportError(MEM_OUT);
        return NULL;
    }
    board->height = high;
    board->width = width;
    board->board = (char**) malloc(high * sizeof(char*));
    int i;
    for (i=0; i<high; i++)
    {
        board->board[i] = (char*) malloc(width * sizeof(char));
    }

    if (board->board == NULL)
    {
        //SHOULD I FREE EACH LINE INDIVIDUALLY??!!??!
        free(board);
        reportError(MEM_OUT);
        return NULL;
    }

    return board;
}

and here's my attempt to expand the board:

static BoardP expandBoard(BoardP theBoard, int X, int Y)
{
    int newWidth = theBoard->width;
    int newHeight = theBoard->height;
    if (X>theBoard->height)
    {
        newHeight = (newHeight+1) * 2;
    }

    if (Y>theBoard->width)
    {
        newWidth = (newWidth+1) * 2;
    }

    BoardP result = createNewBoard(newWidth,newHeight);

    int i;
    for (i=0; i<newHeight; i++)
    {
        result->board[i] = realloc(theBoard->board[i],newWidth);
    }

    freeBoard(theBoard);
    return result;
}

but I keep getting a segmentation fault, and I don't know why. Is my basic idea right? Any idea what am I doing wrong? thanks

Answer 1

There are a lot of possible gotchas here. Make sure when you create an opaque data structure like you have that you write helper functions to make your life easier, and then use those helpers.

I've adjusted your code into a little working example program with a stub driver. Let me break it down:

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

#DEFINE BOARD_BLANK ' '

typedef struct Board 
{
    int width;
    int height;
    char **board;
} Board;

typedef Board *BoardP;

#define MEM_OUT 109

void reportError(int msg)
{
    // Stub
}

This part's just some boilerplate. Now let's create a freeBoard function that outside of createNewBoard, we'll use exclusively for freeing boards:

void freeBoard(BoardP board)
{
    int i;
    for (i=0; i<board->height; i++) // Free each row
        free(board->board[i]);
    free(board->board); // Free the row pointers
    free(board); // Free the structure
}

Now we'll write a board constructor. Notice I've changed the error handling code to reduce repetition and add clarity. This is also the only common use of goto in C:

BoardP createNewBoard(int width, int height)
{
    BoardP board = (BoardP) malloc(sizeof(Board));

    if (board == NULL) 
        goto err1; // Error, jump to alternative error handling exit

    board->height = height;
    board->width = width;
    board->board = (char**) malloc(height* sizeof(char*));

    if (board->board == NULL) 
        goto err2; // Error, jump to alternative error handling exit

    int i;
    for (i=0; i<height; i++)
    {
        // Allocate each row one at a time
        board->board[i] = (char*) malloc(width * sizeof(char));
        if (board == NULL)
            goto err3; 
        for (j=0; j<height; j++)
            board->board[i][j] = BOARD_BLANK; // Give all the data points an initial value
    }

    // Successfully allocated board -- return it.
    return board;

    // Error handling code
err3:
    while (i-- > 0) // Start freeing rows from where we left off
        free(board->board[i]);
    free(board->board); // Free the allocated board->board too

err2:
    free(board);
err1:
    reportError(MEM_OUT);
    return NULL;
}

Straightforward enough. Not a lot of difference there, but notice that I initialized each point on the board to a "blank" value I defined in a macro. This is because the malloc'd memory could contain just about anything in it. Now on to the expandBoard function:

BoardP expandBoard(BoardP board, int X, int Y)
{
    int newWidth = board->width;
    int newHeight = board->height;
    if (X > board->height) // This seems backwards. Height is usually in terms of Y
        newHeight = (newHeight+1)*2; // If we're trying to ensure that the new board
                                     // can contain X as a row index, then it should be:
                                     // newHeight = (board->height > X) ? board->height : X;

    if (Y > board->width) // Same here; width is usually in terms of X
        newWidth = (newWidth+1)*2; // Likewise, should this be:
                                   // newWidth = (board->width > Y) ? board->width : Y; 

    // Create a new board using the constructor we already wrote
    BoardP newBoard = createNewBoard(newWidth, newHeight);

    int i, j;
    for (i=0; i<newHeight; i++) 
        for (j=0; j<board->width; j++)
            newBoard->board[i][j] = board->board[i][j]; // Simply copy data from old to new

    freeBoard(board); // Free the old board
    return newBoard;
}

Read the comments. The approach I took was simply to copy the old board data onto the new board. That's because we've already allocated a completely new board anyway. You can speed this process up a lot by using memcpy instead of doubly nested loops, but either way it should be more than fast enough.

The problem you were having before is that you were trying to realloc the old rows pointed to by the previous board, but then you freed that board and lost all the pointers you realloc'd. This means your "expandBoard" that you originally posted simply discarded all the old board data, leaving you with a completely new board.

In general, try to stay away from realloc until you feel more comfortable with pointers and you've developed a knack for keeping track of them.

Now for the usage:

int main()
{
    BoardP board = createNewBoard(50, 50);
    board = expandBoard(board, 3, 2); // Make sure you assign the board pointer to
                                      // the new expanded board!
    freeBoard(board);

    return 0;
}

Notice that comment! When you call a function that modifies and returns a pointer, make sure you do an assignment. Otherwise, you'll still be pointing to the old, free'd object, which does you no good.

Alternatively, you can always pass a pointer to a pointer to functions that modify pointers.

Anyway, I hope that helps. Take care!

Answer 2

Looks like you are getting a segmentation fault because you point your new board's rows at re-alloced memory of your old board, then you free your old board. I'm not sure why you are doing a realloc when you already allocate memory for your new board in the createNewBoard function.

Answer 3

First, you need to check that each line (when it is getting allocated by malloc in the for loop) doesn't fail. You have checked that for board->board, but not each line, which is also malloc'd and must be checked to see if they were properly allocated. otherwise, one of the lines of the board might be NULL and when it the attempt is made with realloc, it will receive a null pointer.