Assigning values to a 2d array initiated in a struct causes an infinite loop

Question

typedef struct game_t {
    char gameBoard[ROWS][COLUMNS];
} Game;


Game* Create(){
    Game *thegame = (Game *)malloc(sizeof(Game*));
    if (thegame==NULL || historySize<=0){
        return NULL;
    }
    int row=0, col=0;
    for (row = 0; row<ROWS; row++){
        for (col = 0; col<COLUMNS; col++){
            thegame->gameBoard[row][col] = EMPTY_ENTRY;
        }
    }
    return thegame;
}

I'm trying to run the code above and to assign values to the 2d array initialized in the struct (capital letters are all #define values). for some reason, I keep entering into an infinite loop in eclipse. No flag is raised when I build the project, just when I run it. Could anyone please explain to me what am I doing wrong?

thanks!

How do you know its the loop? Confirm it by using a printf inside the loop — Pushan Gupta, Jun 09 '17 at 21:57
Fyi, the `historySize` check should be done *before* you bother allocating at all, as this is recipe for a memory leak if you successfully allocate, but `historySize <=0` is true. — WhozCraig, Jun 09 '17 at 21:58
There is NO need to cast the return of `malloc`, it is unnecessary. See: [**Do I cast the result of malloc?**](http://stackoverflow.com/q/605845/995714) — David C. Rankin, Jun 09 '17 at 22:33

score 1 · Answer 1 · edited Jun 09 '17 at 23:08

1

Statement Game *thegame = (Game *)malloc(sizeof(Game*)) is wrong for sure; together with thegame->gameBoard[row][col] = EMPTY_ENTRY, it introduces undefined behaviour, as you access memory that is not allocated. So you should write Game *thegame = malloc(sizeof(Game)) instead. Note that in C, it is better not to cast the result of malloc as argued here.

Anyway, if EMPTY_ENTRY is defined as value 0, you could also use calloc, which initialises the memory with 0. So you could omit your for-loops then (cf. cppreference/calloc):

void* calloc( size_t num, size_t size ); Allocates memory for an array of num objects of size size and initializes all bytes in the allocated storage to zero.

edited Jun 09 '17 at 23:08

Jonathan Leffler

730,956
141
904
1,278

answered Jun 09 '17 at 22:03

Stephan Lechner

34,891
4
35
58

It is always worth noting that the cast of the return of `malloc` (or `calloc` or `realloc`) is unnecessary. See link in comment to original question. (I suspect you already know that, and that your inclusion is most likely due to simple copy/paste, but when answering, it is always better to clean that up.) – David C. Rankin Jun 09 '17 at 22:36
@David C. Rankin: right, thanks! Added your reference. – Stephan Lechner Jun 09 '17 at 22:44

Vlad from Moscow · Answer 2 · 2017-06-09T23:20:20.390

There are two problems with the function.

First of all this statement

Game *thegame = (Game *)malloc(sizeof(Game*));

allocates memory with the size equal to the size of the pointer Game* instead of allocating memory with the size equal to the size of the structure Game or struct game_t that is the same.

You have to write

Game *thegame = (Game *)malloc(sizeof(Game));

or

Game *thegame = (Game *)malloc(sizeof(struct game_t));

The second problem is that there can be a memory leak in case when the variable historySize is less than or equal to 0.

Take into account that it is better to specify the function parameter as void.

The function definition can look the following way

Game * Create( void )
{
    Game *thegame = NULL;

    if ( historySize > 0 && ( thegame = malloc( sizeof( Game ) ) ) != NULL )
    { 
        for ( int row = 0; row < ROWS; row++ )
        {
            for ( int col = 0; col < COLUMNS; col++ )
            {
                thegame->gameBoard[row][col] = EMPTY_ENTRY;
            }
        }
    }

    return thegame;
}

Also it is not a good idea when a function deals with global variables. You could pass the variable historySize as an argument to the function.

In this case the function will look like (I suppose that the type of the variable historySize is int)

Game * Create( int  historySize )
{
    Game *thegame = NULL;

    if ( historySize > 0 && ( thegame = malloc( sizeof( Game ) ) ) != NULL )
    { 
        for ( int row = 0; row < ROWS; row++ )
        {
            for ( int col = 0; col < COLUMNS; col++ )
            {
                thegame->gameBoard[row][col] = EMPTY_ENTRY;
            }
        }
    }

    return thegame;
}

In this case the function can be called like

Create( historySize );

There is a possibility to substitute the loops for a call of the standard function memset.

For example

#include <string.h>

//...

Game * Create( int  historySize )
{
    Game *thegame = NULL;

    if ( historySize > 0 && ( thegame = malloc( sizeof( Game ) ) ) != NULL )
    {
        memset( thegame->gameBoard, EMPTY_ENTRY, ROWS * COLUMNS ); 
    }

    return thegame;
}

Good spotting on the second issue. – Jonathan Leffler Jun 09 '17 at 23:08 — Jonathan Leffler, Jun 09 '17 at 23:08
you've helped me a lot, really great advices. thanks. – RBS Jun 10 '17 at 06:51 — RBS, Jun 10 '17 at 06:51

Assigning values to a 2d array initiated in a struct causes an infinite loop

2 Answers2