Segmentation fault when accessing a 2D array in a structure whose pointer is returned from a function

Question

I made a structure who has two members (int and int**), and I return the pointer to this structure from one function to main(). It is fine to access the int value in the structure. However, in main() I got Segmentation fault : 11 when I tried to access the element of the 2D array.

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

typedef struct Square {
    int value;
    int **array;
} Square;

Square * generate();

int main(int argc, char *argv[]){
    Square *sqrptr = generate();

    printf("%d\n", sqrptr -> value);
    /* It prints 1 */

    /* Print out the 2D array */
    for (int i = 0; i < 3; i++){
        for (int j = 0; j < 3 ; j++){
            printf("%d ", *(*((sqrptr -> array) + i) + j));
        }
        printf("\n");
    }
    /* It gives segmentation fault */

    return 0;
}

Square * generate(){
    Square mySquare;
    mySquare.value = 1;
    mySquare.array = malloc(sizeof(int*) * 3);

    /* Initialize the 2D array */
    for (int i = 0; i < 3; i++){
        *(mySquare.array + i) = malloc(sizeof(int) * 3);
        for (int j = 0; j < 3; j++){
            *(*(mySquare.array + i) + j) = 0;
        }
    }

    /* Print out the 2D array */
    for (int i = 0; i < 3; i++){
        for (int j = 0; j < 3l ; j++){
            printf("%d ", *(*(mySquare.array + i) + j));
        }
        printf("\n");
    }
    /* I can see the complete 2D array here */

    Square *sqrptr = &mySquare;

    return sqrptr;    
}

I have tried to generate the Square in main(), and use one pointer of the structure to access my 2D array. It works fine, so I guess I have missed something when I use a pointer returned from other functions. On the other hand, I can access the int value successfully, so I have no clues now.

Could someone please explain the underlying reason for this segmentation fault? Thanks!

Answer 1

You're returning a pointer to a local variable (&mySquare). Stack memory (where local variables reside) is when the function returns, so the resulting pointer is pointing to invalid memory. Allocate the struct, and return the pointer to heap memory:

Square *my_square = malloc(sizeof *my_square);
//do stuff
return my_square;

Or pass a pointer to a stack variable as argument:

Square * generate(Square *my_square)
{
    //in case pointer wasn't provided, allocate
    if (my_square == NULL) {
        my_square = malloc(sizeof *my_square);
        if (!my_square)
            return NULL; // or exit or whatever
    }
    //initialize members. To initialize array to 3x3 zero matrix, you can use:
    for (int i=0;i<3;++i)
        my_square.array[i] = calloc(3, sizeof *my_square->array[i]);
    //or even, if you change array member to type int*:
    my_square.array = calloc(3*3, sizeof *my_square->array);
    //at the end:
    return my_square;
}

The latter is arguably the most flexible solution: if you want to work on stack, you call the function like so:

Square my_stack_square;
generate(&my_stack_square);

If you need to use heap memory, you can use:

Square *my_heap_square = generate(NULL);

As Jonathan Leffler pointed out, for a small struct such as this, returning by value isn't too much of a cost. Getting a struct on heap can be achieved in the same way as returning any other type:

Square generate( void )
{
    Square my_square;
    //initialize
    return my_square;
}
//call like so:
Square sq = generate();

The idea here is that you'll use a local variable in the generate function to create a new square, initialize the fields, and then return it. Because in C everything is passed by value, this essentially means the function will assign the value of the local variable from the generate function to the caller's scoped sq variable. For small structs such as this, that's perfectly fine.

What's more, a common thing for compilers to do is to optimise these kinds of functions to the equivalent of the second example I posted: Essentially your function will be creating a new Sqaure object on the stack memory of the caller. This can happen, that's not to say it will. It depends on the optimization levels used when compiling, and on the size of the struct you're returning.

Basically, if you want to keep the code as close to what you have now, it's probably easiest to stick to the first version (returning a heap pointer).

The more flexible approach is the second one (as it allows you to use stack and heap, depending on how you call the function).

For now, using the third approach is perfectly fine: the compiler will most likely optimize the code to whatever makes most sense anyway.

Answer 2

Try this:

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

typedef struct Square {
    int value;
    int **array;
} Square;

Square * generate();

int main(int argc, char *argv[]){
    Square *sqrptr = generate();

    printf("%d\n", sqrptr -> value);
    /* It prints 1 */

    /* Print out the 2D array */
    int i,j;
    for (i = 0; i < 3; i++){
        for (j = 0; j < 3 ; j++){
            printf("%d ", *(*((sqrptr -> array) + i) + j));
        }
        printf("\n");
    }
    /* It gives segmentation fault */

    return 0;
}

Square * generate(){
    Square* mySquare = (Square*) malloc(sizeof(Square)); //c++ compiler
    //Square* mySquare = (void*) malloc(sizeof(Square)); //c compiler
    mySquare->value = 1;
    mySquare->array = malloc(sizeof(int*) * 3);

    /* Initialize the 2D array */
    int i,j;
    for (i = 0; i < 3; i++){
        *(mySquare->array + i) = malloc(sizeof(int) * 3);
        for (j = 0; j < 3; j++){
            *(*(mySquare->array + i) + j) = 0;
        }
    }

    /* Print out the 2D array */
    for (i = 0; i < 3; i++){
        for (j = 0; j < 3l ; j++){
            printf("%d ", *(*(mySquare->array + i) + j));
        }
        printf("\n");
    }
    /* I can see the complete 2D array here */
    return mySquare;
}