board = (char**)realloc(board, numbers[0] * sizeof (char*));
    for (int i = 0; i < numbers[0]; i  ) {
        board[i] = (char*)malloc(numbers[1] * sizeof (char));
    }

board is [y][x] but i want to expand it so it is [numbers[0]][numbers[1]], but this code gives me a memory leak (it's coming from this i checked it). Why and how to do it properly?

Later on it's free like this:

for(int i = 0; i < sizeY; i  ){
              free(board[i]);
        }
        free(board);

It's created like this:

char** board = (char**)malloc(boardY * sizeof * board); 
for (int i = 0; i < boardSizeY; i  ) { 
board[i] = (char*)malloc(boardX * sizeof * *board); 
}

CodePudding user response：

Here's some commentary on the existing code:

// assume at this point that board[0] is pointing to a malloc()'d
// buffer, as is board[1], board[2], and so on

// below here you call realloc(), which resizes the
// pointer-array either larger or smaller.  If it makes
// it smaller, then you've lost access to the pointers
// that were at the end of the old array, so the
// buffers they pointed to have been leaked.

board = (char**)realloc(board, numbers[0] * sizeof (char*));
for (int i = 0; i < numbers[0]; i  ) {
    // In any case, here you are overwriting all the pointers
    // in (board), so any buffers they previously pointed to
    // are now leaked (because no longer have access to them,
    // so you can never call free() on them after this)
    board[i] = (char*)malloc(numbers[1] * sizeof (char));
}

... so, what to do instead? How about something like this:

// before updating numbers[0] or numbers[1], we'll
// free any old allocations we had
for (int i = 0; i < numbers[0]; i  ) {
   free(board[i]);
}

// update numbers[0] and numbers[1] to their new values...
numbers[0] = [...]
numbers[1] = [...];

// now we can resize board and allocate new sub-buffers
board = (char**)realloc(board, numbers[0] * sizeof (char*));
for (int i = 0; i < numbers[0]; i  ) {
   board[i] = (char*)malloc(numbers[1] * sizeof (char));
}

Of course, storing a 2D array as an array-of-pointers-to-arrays is kind of overly-complicated and inefficient anyway, when you could just store all your data inside a single allocation:

size_t arraySize = numbers[0] * numbers[1];
char * board = (char *) malloc(numbers[0] * numbers[1]);
...
free(board);

char getValueAt(const char * board, int sizeX, int x, int y)
{
   return board[(x*sizeX) y];
}

void setValueAt(char * board, int sizeX, int x, int y, char newValue)
{
   board[(x*sizeX) y] = newValue;
}

CodePudding user response：

board = (char**)realloc(board, numbers[0] * sizeof (char*));

This is not good because each pointers in board are lost, and it causes memory leaks as you say.

I think that there are two approaches.

1. free() all pointers in board before realloc.

for(int i = 0; i < sizeY; i  ){
   free(board[i]);
}
board = (char**)realloc(board, numbers[0] * sizeof (char*));
for (int i = 0; i < numbers[0]; i  ) {
    board[i] = (char*)malloc(numbers[1] * sizeof (char));
}

2. Save all pointers to another array before realloc() and restore them after realloc()

char **temp = (char **)malloc(numbers[0] * sizeof(char *));
for (int i = 0; i < numbers[0]; i  ) {
  temp[i] = board[i];
}
board = (char**)realloc(board, numbers[0] * sizeof (char*));
for (int i = 0; i < numbers[0]; i  ) {
  board[i] = temp[i];
}
free(temp);

CodePudding user response：

There are a few options to your approach. If you want to keep the arrangement you created with a matrix of pointers, you need to take care of all edge cases that arise when you resize it.

In your code, when you are resizing, you are forgetting to free the rows that are in the leftover. You should do something like this:

class Board1 {
public:
    Board1() : rows(0), cols(0), board(nullptr) {}
    Board1( uint32_t nr, uint32_t nc ) : rows(0), cols(0), board(nullptr) { resize(nr,nc); }
    ~Board1() {
        for ( uint32_t j=0; j<rows;   j ) {
            ::free( board[j] );
        }
        ::free( board );
    }
    void resize( uint32_t nr, uint32_t nc ) {
        if ( nr<rows ) {
            for ( uint32_t j=0; j<rows-nr;   j ) {
                ::free( board[rows-1-j] );
            }
        }
        board = (char**)::realloc( board, nr*sizeof(char*) );
        if ( nr>rows ) {
            for ( uint32_t j=0; j<nr-rows;   j ) {
                board[rows j] = (char*)::malloc(nc*sizeof(char));
            }
        }
        if ( nc!=cols ) {
            for ( uint32_t j=0; j<nr;   j ) {
                board[j] = (char*)::realloc( board[j], nc*sizeof(char) );
            }
        }
        rows = nr;
        cols = nc;
    }
    char& operator()( uint32_t nr, uint32_t nc ) {
        return board[nr][nc];
    }
    char operator() ( uint32_t nr, uint32_t nc ) const {
        return board[nr][nc];
    }
private:
    uint32_t rows, cols;
    char** board;
};

However if the matrix is small (<1 GiB) then you should consider having it in a single slab of memory like this:

class Board2 {
public:
    Board2() : rows(0), cols(0), board(nullptr) {}
    Board2( uint32_t nr, uint32_t nc ) : rows(0), cols(0), board(nullptr) { resize( nr, nc ); }
    void resize( uint32_t nr, uint32_t nc ) {
        board = (char*)::realloc( board, nr*nc*sizeof(char) );
        rows = nr;
        cols = nc;
    }
    char& operator()( uint32_t nr, uint32_t nc ) {
        return board[nr*rows nc];
    }
    char operator() ( uint32_t nr, uint32_t nc ) const {
        return board[nr*rows nc];
    }
private:
    uint32_t rows, cols;
    char* board;
};

But if you want something that is much safer, you can use std::vector as the backend for your class:

class Board3 {
public:
    Board3() {}
    Board3( uint32_t nr, uint32_t nc ) { resize(nr,nc); }
    void resize( uint32_t nr, uint32_t nc ) {
        board.resize( nr );
        for ( Row& row : board ) {
            row.resize( nc );
        }
    }
private:
    using Row = std::vector<char>;
    using Matrix = std::vector<Row>;
    Matrix board;
};