Segmentation fault. Where am I wrong? C

Question

I need to realize dynamic matrix in C.

I have this code in main:

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

int main() {
  int rows, cols;

  scanf("%d", &rows);
  scanf("%d", &cols);

  int** A = create_matrix(rows, cols);
  fill_matrix(A, rows, cols);

  add_row(A, &rows, cols);
  print_matrix(A, rows, cols);
  free_matrix(A, rows);
  return 0;
}

As you can see, I allocate matrix, then fill it by:

4 3
1 1 1
2 2 2
3 3 3
4 4 4

and I want to add new row [1, 1, 1], then print, then free. Here are functions

void add_row(int **matrix, int *rows_amount, int cols_amount)
{
  printf("started\n");

  (*rows_amount)++;
  matrix = (int**)realloc(matrix, (*rows_amount) * sizeof(int*));
  matrix[*rows_amount - 1] = (int*)malloc(cols_amount * sizeof(int));

  for (int j = 0; j < cols_amount; j++)
    matrix[*rows_amount - 1][j] = 1;

  printf("ended\n");
  return;
}

int **create_matrix(int rows_amount, int cols_amount) 
{
    int **matrix = (int**)malloc(rows_amount * sizeof(int*));
    for(int i = 0; i < rows_amount; i++) 
        matrix[i] = (int*)malloc(cols_amount * sizeof(int));
    return matrix;
}

void fill_matrix(int **matrix, int rows_amount, int cols_amount) 
{
  for(int i = 0; i < rows_amount; i++)  
        for(int j = 0; j < cols_amount; j++)
            scanf("%d", matrix[i] + j);
}

void free_matrix(int **matrix, int rows_amount) 
{
    for(int i = 0; i < rows_amount; i++)  
        free(matrix[i]);
    free(matrix);
}

void print_matrix(int **matrix, int rows_amount, int cols_amount) 
{
    for(int i = 0; i < rows_amount; i++)
  {
        for(int j = 0; j < cols_amount; j++)  
            printf("%d ", matrix[i][j]);
        printf("\n");
  }
}

When I use valgrind to find the line causes the SEGFAULT, I get this:

stepan@WIN-KBCGJ2G97TU:/mnt/d/Desktop/study/sem3/c_prog/lab_09$ valgrind ./app.out < 1.txt 
==2715== Memcheck, a memory error detector
==2715== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2715== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==2715== Command: ./app.out // I add < d.txt which contains test
==2715==
==2715== error calling PR_SET_PTRACER, vgdb might block
started // between this and next lines no errors! this is in add_row
ended
==2715== Invalid read of size 8
==2715==    at 0x1089F9: print_matrix (matrix.c:33)
==2715==    by 0x109048: main (main.c:19)
==2715==  Address 0x522e080 is 0 bytes inside a block of size 32 free'd
==2715==    at 0x4C31D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)      
==2715==    by 0x108DC5: add_row (matrix.c:102)
==2715==    by 0x109034: main (main.c:17)
==2715==  Block was alloc'd at
==2715==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)       
==2715==    by 0x1088A9: create_matrix (matrix.c:8)
==2715==    by 0x109006: main (main.c:11)
==2715==
1 1 1
2 2 2
3 3 3
4 4 4 // on the next line must have been 1 1 1
==2715== Invalid read of size 4
==2715==    at 0x108A09: print_matrix (matrix.c:33)
==2715==    by 0x109048: main (main.c:19)
==2715==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==2715==
==2715==
==2715== Process terminating with default action of signal 11 (SIGSEGV)
==2715==  Access not within mapped region at address 0x0
==2715==    at 0x108A09: print_matrix (matrix.c:33)
==2715==    by 0x109048: main (main.c:19)
==2715==  If you believe this happened as a result of a stack
==2715==  overflow in your program's main thread (unlikely but
==2715==  possible), you can try to increase the size of the
==2715==  main thread stack using the --main-stacksize= flag.
==2715==  The main thread stack size used in this run was 8388608.
==2715== 
==2715== HEAP SUMMARY:
==2715==     in use at exit: 100 bytes in 6 blocks
==2715==   total heap usage: 9 allocs, 3 frees, 8,324 bytes allocated
==2715==
==2715== LEAK SUMMARY:
==2715==    definitely lost: 40 bytes in 1 blocks
==2715==    indirectly lost: 60 bytes in 5 blocks
==2715==      possibly lost: 0 bytes in 0 blocks
==2715==    still reachable: 0 bytes in 0 blocks
==2715==         suppressed: 0 bytes in 0 blocks
==2715== Rerun with --leak-check=full to see details of leaked memory
==2715==
==2715== For counts of detected and suppressed errors, rerun with: -v
==2715== ERROR SUMMARY: 14 errors from 2 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)

So, it seems that some things happen in add_row:

matrix = (int**)realloc(matrix, (*rows_amount) * sizeof(int*));
matrix[*rows_amount - 1] = (int*)malloc(cols_amount * sizeof(int));

It is true 99.9% because without adding row, just create, fill and free there is no memory errors.

And also I don't understand why Invalid read of size 8 before all lines printed (matrix.c:33 is printf("%d ", matrix[i][j]);), and then he tries to read 4 bytes ¯_(ツ)_/¯.

What is wrong?

UPDATE.

I added return matrix; And in main A = add_row... And it works! Thanks all you guys for C Memory lesson :)

Answer 1

The problem is that add_row() changes the value of matrix via realloc(), but the calling function - in main(), maybe? doesn't see that change.

In the same way, if add_row() changed the value of cols_amount, that change would not be visible to the caller. You don't care about that.

But you DO care about the row count change being visible to the caller, so you correctly pass the address of rows_amount so that when add_row() adds one to the row count, the caller sees that change.

Do the same thing with matrix by passing the address of the pointer:

int add_row(int ***pMatrix, int *rows_amount, int cols_amount)
{
    (*pMatrix) = realloc(*pMatrix, ...)
    (*pMatrix)[*rows_amount - 1] = ...

    // use (*pMatrix) where you used to use matrix
}

int main()
{
   ...
   add_row(&matrix, &rows, cols);

EDIT: to answer something from the comments;

When you allocate memory, that space is reserved for you and you get a pointer back, and the next memory request is often allocated right after the first one, perhaps separated by a bit of padding or housekeeping. This means that the first and second allocations are more or less right next to each other.

|---alloc1---|

|---alloc1---| |---alloc2---|

The only way realloc(alloc1) can return the same pointer is if there's free space after the allocation: if this happens, then it will adjust housekeeping and return the same pointer.

But here we find that alloc2 is in the way, there's no room to extend alloc1, so it has no choice but to reserve a new spot, copy stuff over, and release the first one:

|---alloc1---|

|---alloc1---| |---alloc2---|
               |---alloc2---| |---bigger alloc1-------|

I would imagine that asking realloc() for less memory would return the same pointer because it can just update the housekeeping, but it's not required to do so, and in practice you never ever ever rely on it.

EDIT2 In practice, I would not solve the problem this way. I'd create a structure that holds the pointer plus the row/column counts, so it could be passed around as a single item rather than all the three parts at a time.

struct matrix {
    int rows;
    int cols;
    int **cells;
};

Answer 2

Your add_row function modifies matrix and renders the old value invalid but doesn't return the new value to the caller.