Passing 2d array to function with pointer

Question

Can anyone explain what is the wrong of print() function ?

printf("Control") never works, output is segmentation fault-11

int main(int argc, char **argv){
int m=5,n=4;    // matrix[5][4]
int a=50,b=20;  // range: 20-50

int **matrix;

imatrix(matrix,m,n,a,b);
print(matrix,m,n);

first step: filling address with value

    void imatrix(int **matrix,int m,int n,int max, int min){
    srand(time(NULL));
    int i=0,j=0;
    
    matrix = (int **)malloc( m * sizeof(int*) );
    if( matrix == NULL ){
        printf( "memory req.!" );
    }
    
    
    for( i = 0; i < m; i++ ) {
        matrix[i] = (int *)malloc( n * sizeof(int) );
        if( matrix[i] == NULL ){
            printf( "memory req.!" );
        }
    }
    
    
    for(i=0;i<m;i++){
        for(j=0;j<n;j++){
            matrix[i][j]=(rand()%(max-min))+min;
            printf("%2d ",matrix[i][j]);
        }
        printf("\n\n");
    }
    }

Anything is okey till here. I get Segmentation fault: 11 after below code and line of "Control" never works

void print(int **matrix, int m, int n){
int i,j;
for(i=0; i < m; i++){
    for(j=0; j < n; j++){
       printf("%d",*(*(matrix + i) + j));
    }
    printf("\n");
}

printf("control");
}

Answer 1

It's not the printf("control") which segfaults (although you'd be a lot better off not using printf for that: consider changing it to puts("control"), which also outputs a newline character.) It's the previous attempt to print the matrix, which is dereferencing an uninitialised value.

That happens because your imatrix function does not return the matrix it created, and the matrix in your main is not given any value.

In fact, imatrix doesn't return anything, since it is defined as returning void. It takes a matrix argument, which I suppose was intended to be an output parameter, but:

• that parameter has the wrong type to be an output parameter (an output parameter needs to be a pointer to the object to be returned;)

• it never attempts to use the pointer to the object to be returned in order to return the object.

In order to comply with those requirements, you'd need the prototype

void imatrix(int*** matrix, int m, int n, int max, int min);

And you would have to add an additional level of indirection for each usage of matrix [Note 1]:

    *matrix = malloc(m * sizeof(**matrix));
    if (*matrix == NULL) {
        printf( "memory req.!" );
        /* NOTE: You shouldn't attempt to continue after the malloc fails.
         * Return immediately and let the caller deal with the malloc failure.
         */
    }

    for( i = 0; i < m; i++ ) {
        (*matrix)[i] = malloc( n * sizeof(*(*matrix)[i])) );
        if( (*matrix)[i] == NULL ){
    // etc.

I think we can agree that that is a major PITA and not worth the trouble unless necessary.

Personally, I find it less confusing to not use output parameters at all unless strictly necessary. In this case it isn't necessary at all because you can just return the pointer to allocate memory:

/* CHANGE: Prototype */
int** imatrix(int m, int n, int max, int min){
    srand(time(NULL)); /* TODO: This initialisation should really be done in main */
    int i=0, j=0;
    
    /* CHANGE: matrix is a local variable */
    int** matrix = malloc( m * sizeof(*matrix) );
    /* CHANGE: Immediate return on malloc failure */
    if( matrix == NULL ) {
        return matrix;
    }
    
    for( i = 0; i < m; i++ ) {
        matrix[i] = malloc( n * sizeof(*matrix[i]) );
        if( matrix[i] == NULL ){
            printf( "memory req.!" );
        }
    }
    
    
    for(i=0;i<m;i++){
        for(j=0;j<n;j++){
            matrix[i][j]=(rand()%(max-min))+min;
            printf("%2d ",matrix[i][j]);
        }
        printf("\n\n");
    }

    /* CHANGE: matrix is returned */
    return matrix;
}

That has a slightly different usage pattern:

int main(int argc, char **argv){
    int m=5, n=4;    // matrix[5][4]
    int a=50, b=20;  // range: 20-50

    /* CHANGE: imatrix returns the new matrix */
    int **matrix = imatrix(m, n, a, b);
    /* CHANGE: check for failure */
    if (matrix == NULL) {
         fprintf(stderr, "%s\n", "imatrix failed to allocate memory.");
         exit(1);
    }

    print(matrix, m, n);
    /* TODO: Free the storage allocated for matrix */
}

---

Notes

1. Throughout this code, I changed the usage of malloc from:

   lvalue = (RedundantCast*)malloc(count * sizeof(FixedType));
   

   to idiomatic C:

   lvalue = malloc(count * sizeof(*lvalue));
   

   The explicit cast of the return value of malloc is at best pointless in C, since malloc returns a void* and C is happy to automatically convert a void* into a pointer of any type. Using the type of the object pointed to by the target (sizeof(*lvalue)) rather than inserting a specific type protects you against the possibility that the type will change in a future edit, and you forget to make that change in all calls to malloc. (Consider what would happen if you decided to make matrix a matrix of long long instead of int, for example.)