1

I just started learning C and I wanted to try creating a test program that works with pointers, structures, and arrays, since I still have a hard time understanding them. I created this test file which is a distilled version of a larger project that I'm working on. The test file has a struct with a dynamic 2D array as a member of the struct:

typedef struct {
  int ** array;
  int rows, cols;
} Smaller;

However, after running the test file the terminal returns the following error:

zsh: segmentation fault ./a.out

I researched what this error means,

" Segmentation fault is a specific kind of error caused by accessing memory that “does not belong to you.” " (Link)

But I'm still confused on how fix this problem. I'm pretty sure I allocated the correct amount of memory for each row and column. It's even more confusing because the terminal doesn't indicate which line the error is. I would appreciate any help on this issue.

Below is the full code:

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

typedef struct {
  int ** array;
  int rows, cols;
} Smaller;


void printArray (Smaller * s);

int main () {
  int x, i, j;

  Smaller * sand;

  // allocate mem for number of rows
  sand->array = malloc (3 * sizeof(int *));

  //allocate mem for number of columns
  sand->array = malloc(4 * sizeof(int));
  sand->array = malloc(4 * sizeof(int));
  sand->array = malloc(4 * sizeof(int));

  // adding a constant value to the 2D array
  for (i = 0; i < 3; i ++) {
    for (j = 0; j < 4; j ++) {
      sand->array[i][j] = 6;
    }
  }

  printArray(sand);

  return 0;
}

void printArray (Smaller * sand) {
  printf("Welcome to the printArray function! \n");

  int i, j;

  for (i = 0; i < 3; i ++)
    for(j = 0; j < 4; j ++)
      printf("array[%d][%d] = %d \n", i, j, sand->array[i][j]);

}

  • 1
    You didn't allocate memory for the structure itself that `sand` is pointing to. The result is that `sand` can point to any random location in memory (although it can be set to `NULL` by the compiler). In any case it will cause problems. – tromgy Jan 24 '22 at 00:47
  • Take a look at this video [Pointers and 2-D Arrays | Two dimensional array | data structure](https://www.youtube.com/watch?v=tw-qWGG8y5g). Pretty decent explanation of pointers and arrays – Mahendra Gunawardena Jan 24 '22 at 00:52
  • @tromgy Oh, okay. So when allocating memory, I have to allocate memory to the structure members as well as the structure itself. I hope I've grasped what you're conveying. – user16052347 Jan 24 '22 at 00:56
  • You also have allocated sub-arrays ("columns") incorrectly. You're allocating the memory 3 times for the same pointer. You need to do it like `sand->array[x] = malloc(4 * sizeof(int));`, where x is 0, 1, 2. Or better yet do it in a loop. – tromgy Jan 24 '22 at 01:00

2 Answers2

1

The problem is, as @tromgy pointed out, you are overwriting the base sand->array with the column arrays instead of assigning them to it. A correct code would look like this:

#include <stdlib.h>
#define NUM_ROWS 3
#define NUM_COLS 4

typedef struct {
    int ** array;
    int rows;
    int cols;
} Smaller;

void print_array(Smaller * s);

int main(void) {
    Smaller * sand = malloc(sizeof(Smaller));
    if (!sand) return -1; /* allocation failed, abort */
    sand->rows = NUM_ROWS;
    sand->array = malloc(sizeof(int*[NUM_ROWS]));
    if (!sand->array) { /* allocation failed, abort */
        free(sand); /* free sand first, though */
        return -1;
    }
    for (size_t i = 0; i < NUM_ROWS; ++i) {
        sand->array[i] = malloc(sizeof(int[NUM_COLS]));
        if (!sand->array[i]) {
            /* free the previous rows */
            for (size_t j = 0; j < i; ++j) free(sand->array[j]);
            free(sand->array);
            free(sand);
            return -1;
        }
    }
    /* add a constant value to the array */
    for (size_t i = 0; i < NUM_ROWS; ++i) {
        for (size_t j = 0; j < NUM_COLS; j ++) {
            sand->array[i][j] = 6;
        }
    }
    print_array(sand);
    /* Ok, now free everything */
    for (size_t i = 0; i < NUM_COLS; ++i) {
        free(sand->array[i]);
    }
    free(sand->array);
    free(sand);
    /* NOW we may exit */
    return 0;
}

As you can see, allocating a structure like this is a lot of work, and you have to free whatever you allocate, so it's probably better to extract it out to a function, something like Smaller * smaller_init(size_t nrows, size_t ncols) and void smaller_destroy(Smaller * s) encapsulating all that work.

Wtrmute
  • 168
  • 5
0

I will left an example below so you can compare it to the way you wrote it originally...

About your code:

  • Declare loop variables inside the for command
  • May be Smaller do not need to be a pointer
  • Keep dimensions as variables. It is more flexible
  • You did not set the values for rows and cols in the struct. And in main() do not use fixed values as 3 and 4 as you did
  • You should set all cells to different values, not the same. You will feel safer when you see reversible values, like 100*row + column in the example... This way you can see if the loops are ok and all elements are being printed. See this output for printArray():
      0    1    2    3
    100  101  102  103
    200  201  202  203

Each line starts with the line number so you can test it a few times before going on.

  • make your program test itself. In printArray() for example show the dimensions like this:
    printArray[3,4]

      0    1    2    3
    100  101  102  103
    200  201  202  203

See the output of the example

  • always write the code to free the memory, in the reserve order of the allocation, maybe in a separate function that returns NULL in order to invalidate the pointer back in the calling code, like this
Smaller* freeArray(Smaller* A)
{
    printf("\nfreeArray()\n");
    for (int i = 0; i < A->rows; i++)
    {
        free(A->array[i]);  // delete lines
        printf("row %d free()\n", i);
    }
    free(A->array);  // delete cols
    printf("pointer to rows free()\n");
    free(A);  // delete struct
    printf("struct free()\n");
    return NULL;
}

This way you know that the pointer sand will not be left pointing to an area that has been free()d. Using such a pointer will crash your program so it may be good to write

    sand = freeArray(sand);

output of the example code

printArray[3,4]

  0    1    2    3
100  101  102  103
200  201  202  203

freeArray()
row 0 free()
row 1 free()
row 2 free()
pointer to rows free()
struct free()

Example code

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

typedef struct
{
    int** array;
    int   rows, cols;

} Smaller;

void     fillArray(Smaller*);
Smaller* freeArray(Smaller*);
Smaller* makeArray(size_t, size_t);
void     printArray(Smaller*);

int main(void)
{
    int y = 3; 
    int x = 4;
    // sand points to a Smaller
    Smaller* sand = makeArray(y, x);
    // adding known unique values to cells is easier
    fillArray(sand);
    printArray(sand); // show values
    sand = freeArray(sand); // delete all
    return 0;
}

void fillArray(Smaller* A)
{
    for (int i = 0; i < A->rows; i++)
        for (int j = 0; j < A->cols; j++)
            A->array[i][j] = 100 * i + j;
}

Smaller* freeArray(Smaller* A)
{
    printf("\nfreeArray()\n");
    for (int i = 0; i < A->rows; i++)
    {
        free(A->array[i]);  // delete lines
        printf("row %d free()\n", i);
    }
    free(A->array);  // delete cols
    printf("pointer to rows free()\n");
    free(A);  // delete struct
    printf("struct free()\n");
    return NULL;
}

Smaller* makeArray(size_t y, size_t x)
{
    // sand points to a Smaller
    Smaller* sand = (Smaller*)malloc(sizeof(Smaller));
    sand->rows    = y;
    sand->cols    = x;

    // allocate mem for number of rows, that is 'y'
    sand->array = malloc(y * sizeof(int*));

    // allocate mem for each of the 'x' columns
    for (size_t i = 0; i < y; i++)
        sand->array[i] = malloc(x * sizeof(int));

    return sand;
};

void printArray(Smaller* sand)
{
    printf("printArray[%d,%d]\n\n", sand->rows, sand->cols);
    for (int i = 0; i < sand->rows; i++)
    {
        for (int j = 0; j < sand->cols; j++)
            printf("%3d  ", sand->array[i][j]);
        printf("\n");
    }
}

About the code

Please SO people do not bother pointing me not to cast the result of malloc(). It is by decision. This common recommendation is a reminiscence of the C-faq of the 90's and now we know that implicit conversions maybe not so good. In fact implicit things may cost you a lot of time: if you malloc() a series of different structs in a program and omit the types if some of them are for example reversed keep in mind that the use of all casts would help you avoid this costly type of mistake...

arfneto
  • 1,227
  • 1
  • 6
  • 13