Illegal operation when setting array values in struct

Question

I have the following 3 structs for a card game program

// Linked list of cards, used for draw & discard pile, players' hands
typedef struct cardStack {
    struct cardStack *next;
    Card *card;
} CardStack;

// A player and their hand
typedef struct player {
    int playerNumber;
    CardStack *hand;
} Player;

typedef struct _game {
    CardStack *discardPile;
    CardStack *drawPile;    
    Player *players[4];
    int currentPlayer;
    int currentTurn;
} *Game;

and this function to initialise the game struct

Game newGame(int deckSize, value values[], color colors[], suit suits[]) {

    Game game = (Game)malloc(sizeof(Game));

    game->players[0] = (Player*)malloc(sizeof(Player));
    game->players[0] = &(Player){0, NULL};
    game->players[1] = (Player*)malloc(sizeof(Player));
    game->players[1] = &(Player){1, NULL};
    game->players[2] = (Player*)malloc(sizeof(Player));
    game->players[2] = &(Player){2, NULL};
    game->players[3] = (Player*)malloc(sizeof(Player));
    game->players[3] = &(Player){3, NULL};

    for (int i = 1; i <= 7; i++) {
        for (int j = 1; i <= NUM_PLAYERS; i++) {
            Card card = newCard(values[i * j - 1], colors[i * j - 1], suits[i * j - 1]);
            addToStack(card, game->players[j-1]->hand);
        }
    }

    CardStack *discardPile = (CardStack*)malloc(sizeof(CardStack));
    Card firstDiscard = newCard(values[28], colors[28], suits[28]);
    addToStack(firstDiscard, discardPile);
    game->discardPile = discardPile;

    CardStack *drawPile = (CardStack*)malloc(sizeof(CardStack));
    for (int i = 29; i < deckSize; i++) {
        Card card = newCard(values[i], colors[i], suits[i]);
        addToStack(card, drawPile);
    }
    game->drawPile = drawPile;

    game->currentPlayer = 0;
    game->currentTurn = 1;

    return game;
}

It compiles fine, but when I try to run it, this line

game->players[0] = (Player*)malloc(sizeof(Player));

and similar, give an error "illegal array, pointer or other operation" I'm not sure whats wrong as I am just setting one pointer (in the array of pointers in the struct) to another

EDIT: unfortunately this is an assignment where the header file was given so I have no choice but to use the pointer typedef

You are getting confused with malloc when you use *Game If Game is really a pointer than sizeof(Game) isn't correct allocation! — abc
– abc, Commented Oct 10, 2018 at 2:03
ok... this time I bet I have it... Game is a pointer. So, this line doesn't allocate enough space: Game game = (Game)malloc(sizeof(Game)); — bruceg
– bruceg, Commented Oct 10, 2018 at 2:03
Probably worth noting that the line game->players[0] = &(Player){0, NULL}; not only causes a memory leak, but also takes the address of an object allocated on the stack. That object will cease to exist when the function returns. §6.5.2.5/5 — user3386109
– user3386109, Commented Oct 10, 2018 at 2:26
Note that you should not create function, variable, tag or macro names that start with an underscore, in general. C11 §7.1.3 Reserved identifiers says (in part): — All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use. — All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces. See also What does double underscore (__const) mean in C? — Jonathan Leffler
– Jonathan Leffler, Commented Oct 10, 2018 at 5:50
Bad luck on being given a header to work with that illustrates several ways in which you should not write C code. Presumably, you were also given a structure for the type Card — Jonathan Leffler
– Jonathan Leffler, Commented Oct 10, 2018 at 6:00

John Kugelman · Accepted Answer · 2018-10-10 02:14:57Z

3

typedef struct _game {
    ...
} *Game;

Game is defined as an alias for struct _game *, a pointer.

Game game = (Game)malloc(sizeof(Game));

That means that sizeof(Game) is the size of a pointer and not the entire struct. A pointer is smaller than the entire struct, so it's not enough memory. Writing to ->players accesses memory outside of the malloc'ed area which causes the illegal operation error.

A correct allocation would be:

Game game = malloc(sizeof *game);

Lesson learned: use p = malloc(sizeof *p) rather than p = malloc(sizeof(Type)) to avoid this kind of mistake. The compiler won't catch a size mismatch. sizeof *p will always be the right size, even if p changes type.

And if possible, get rid of the * in the definition of Game! It's quite out of place.

edited Oct 10, 2018 at 2:14

answered Oct 10, 2018 at 2:05

John Kugelman

365k70 gold badges555 silver badges600 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

yxting Over a year ago

Thanks, that fixed it. Unfortunately I have to follow a given header file with that definition. I understand the malloc issue, but could you explain why the error was an illegal operation?

John Kugelman Over a year ago

sizeof(Game) does not allocate enough memory. Writing to ->players accesses memory outside of the malloc'ed area.

Jonathan Leffler · Accepted Answer · 2018-10-10 05:04:23Z

Quite apart from the memory allocation issue identified by John Kugelman, you have at least one more major problem…

Another problem

Note that the lines like these two:

game->players[0] = (Player*)malloc(sizeof(Player));
game->players[0] = &(Player){0, NULL};

carefully leak the allocated memory. You replace the just allocated pointer with a pointer to the compound literal, which leaves you no way to free the allocated memory. It is perfectly legitimate to modify the compound literal as long as it doesn't go out of scope — but it does go out of scope when the function returns, so you not only leak memory but you also modify data you no longer own if you ever change the player information.

You probably want this instead, which copies the compound literal to the allocated memory:

game->players[0] = (Player*)malloc(sizeof(Player));
*game->players[0] = (Player){0, NULL};

I wouldn't be surprised to find there are other issues lurking, but the code is not an MCVE (Minimal, Complete, Verifiable Example) so it is hard to be sure.

abc · Accepted Answer · 2018-10-10 02:08:29Z

0

Don't use pointers at the end of struct definitions it's ugly.

Change it to:

typedef struct _game {
   CardStack *discardPile;
   CardStack *drawPile;    
   Player *players[4];
   int currentPlayer;
   int currentTurn;
} Game;

And in your malloc replace that statement with the following:

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

Bonn-appetite!

answered Oct 10, 2018 at 2:08

abc

2,0871 gold badge19 silver badges32 bronze badges

2 Comments

Jonathan Leffler Over a year ago

Curious — I don't think I've ever seen the advice "don't use pointers at the end of struct definitions; it's ugly" before. Would you care to elaborate on why it is ugly, and can you perhaps provide a URL where the reasoning is explained?

Jonathan Leffler Over a year ago

Oh — do you mean "Don't typedef pointers"? If so, then the linked question is the canonical reference on the topic. Note that when you say "don't use pointers at the end of struct definitions", people are likely to assume that you mean "don't use struct Whatever { …; SomeType *ptr; } because that is at the end of the structure definition. In the question, there is typedef struct _game { … } *Game; which is defining the name Game as a pointer to struct _game, but it's not part of the structure definition per se. English is weird sometimes!

Collectives™ on Stack Overflow

Illegal operation when setting array values in struct

3 Answers 3

2 Comments

Another problem

Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

Another problem

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related