Segmentation fault after a free()

Question

I'm trying to dynamically allocate one matrix N*N and put random numbers (between 0 and h-1) in it. I also have to create a function that deallocates it. The problem is that I have to use structures and I'm not very used to them. Structure "game_t" is defined in another file and included.

game_t * newgame( int n, int m, int t){
    game_t x, *p;
    int i,j;
    p=&x;
    x.t=t;
    x.n=n;    /* structure game has n,t,h,board as keys*/
    x.h=h;
    srand(time(NULL)); 
    x.board=malloc(n*sizeof(int*));    /*Allocate*/
    if (x.board==NULL) return NULL;
    for (i=0;i<n;i++){
              x.board[i]=malloc(n*sizeof(int));}
    for (i=0;i<n;i++){   /*put random numbers*/
             for (j=0;i<n;j++){
                     x.board[i][j]=rand()%h;}}
    return p;
}

void destroy(game_t *p){
    int i;
    for (i=0;i<p->n;i++){
        free(p->board[i]);}
    free(p->board);
}

The main problem is `p=&x;` Here you make `p` point to the *local* variable `x`, a variable whose life-time ends when the function `newgame` returns. That makes the pointer invalid. — Some programmer dude, Nov 04 '21 at 13:26
You return the address of a local variable here: `return p;` because `p` points to `x`. — Jabberwocky, Nov 04 '21 at 13:27
All the bugs aside, you probably want to use a 2D array instead. See [Correctly allocating multi-dimensional arrays](https://stackoverflow.com/questions/42094465/correctly-allocating-multi-dimensional-arrays). — Lundin, Nov 04 '21 at 13:46

Konrad Rudolph · Answer 1 · 2021-11-04T13:32:32.920

p is a pointer to a local variable, x. Once you leave the function newgame, that variable ceases to exist, and p is now a dangling pointer. You are not allowed to access it.

The solution is straightforward: return a game_t from newgame, rather than a newgame_t *, and don’t use p:

game_t newgame(int n, int m, int t) {
    game_t x;
    int i, j;
    x.t = t;
    x.n = n;    /* structure game has n,t,h,board as keys*/
    x.h = h;
    srand(time(NULL)); 
    x.board = malloc(n * sizeof (int*));    /*Allocate*/
    if (x.board == NULL) {
        perror("newgame");
        exit(1);
    }
    for (i = 0; i < n; i++) {
        x.board[i] = malloc(n * sizeof(int));
        if (x.board[i] == NULL) {
            perror("newgame");
            exit(1);
        }
    }
    for (i = 0; i < n; i++) {   /*put random numbers*/
        for (j = 0; i < n; j++) {
            x.board[i][j] = rand() % h;
        }
    }
    return x;
}

Note that this means that you can no longer return NULL if an allocation fails. I’ve amended the code to exit instead, but a different strategy might be desirable (though probably not: allocation failures are often tough to handle, and quitting — with an error message! — is an OK response).

Problem is i've been asked to create a function newgame that returns a * game_t ! Thank you anyways! — PhysicsStudent, Nov 04 '21 at 14:05
@PhysicsStudent In that case you’ll want the answer by Jabberwocky. — Konrad Rudolph, Nov 04 '21 at 14:07

Jabberwocky · Accepted Answer · 2021-11-04T14:37:41.833

1

You return the pointer to the local variable x. Local variables disappear once you leave their scope.

game_t * newgame( int n, int m, int t){
    game_t x, *p;
    int i,j;
    p=&x;    // <<<< p points to the local variable x
    x.t=t;
    x.n=n;
    x.h=h;
    srand(time(NULL)); 
    x.board=malloc(n*sizeof(int*));
    if (x.board==NULL) return NULL;
    for (i=0;i<n;i++){
              x.board[i]=malloc(n*sizeof(int));}
    for (i=0;i<n;i++){
             for (j=0;i<n;j++){
                     x.board[i][j]=rand()%h;}}
    return p;
}

You probably want this:

game_t * newgame( int n, int m, int t){
    game_t *p = malloc(sizeof (*p));   // allocate memory for a new game_t
    int i,j;
    p->t=t;
    p->n=n;
    p->h=h;
    srand(time(NULL)); 
    p->board=malloc(n*sizeof(int*));
    if (p->board==NULL) return NULL;
    for (i=0;i<n;i++){
              p->board[i]=malloc(n*sizeof(int));}
    for (i=0;i<n;i++){
             for (j=0;i<n;j++){
                     p->board[i][j]=rand()%h;}}
    return p;
}

BTW: you should use meaningful variable names. For example p should be named new etc.

Bonus: call srand(time(NULL)) only once at the start of the program.

edited Nov 04 '21 at 14:37

answered Nov 04 '21 at 13:30

Jabberwocky

48,281
17
65
115

It makes sense! However now i get an error because *p is impatible with malloc. The structure game_t defined outside ` typedef struct game { int n, int h, int ** board} game_t` – PhysicsStudent Nov 04 '21 at 14:30
@PhysicsStudent which error do you get _exactly_? Never write "I get an error", always write the _verbatim_ error message. – Jabberwocky Nov 04 '21 at 14:34
I get "incompatible type for argument 1 of malloc", it expects size_t – PhysicsStudent Nov 04 '21 at 14:36
Sorry, my bad, it should be `malloc(sizeof (*p))`. I just edited the question – Jabberwocky Nov 04 '21 at 14:37
Wops it was easier than i thought! Thanks a lot it works perfectly now! – PhysicsStudent Nov 04 '21 at 14:39
@PhysicsStudent also consider the other answer, which has a slightly different approach. I can't tell which one is better, it depends on your use case. – Jabberwocky Nov 04 '21 at 14:40

Segmentation fault after a free()

2 Answers2