Linked lists segmentation fault

Question

I have to create linked lists which will represent a matrix. In the first linked list I have the row coordinate, a link to the next node and a link to another structure which contain the column coordinate, the link to the next node and the value of the number from the matrix at those coordinates. Here are my different structures which might be more explicit.

#include "sparsemat.h"
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>

int main() {
    matrix* tempMatrix = (matrix*)malloc(sizeof(matrix));
    tempMatrix = sp_read("mat_100x5");
    return 0;
}

struct matrix* sp_read(char* path) {
    matrix* matrice = (matrix*)malloc(sizeof(matrix));
    matrice->rowFirst = NULL;
    matrice->nRows = 0;
    matrice->nCols = 0;
    row* rows = (row*)malloc(sizeof(row));
    rows->indiceRow = 0;
    rows->data = NULL;
    rows->next = NULL;
    col* cols = (col*)malloc(sizeof(col));
    cols->value = 0, 0;
    cols->indiceColumn = 0;
    cols->next = NULL;

    FILE* file = NULL;
    file = fopen(path, "rb");

    int nRow;
    int nCol;

    fread(&nRow, sizeof(int), 1, file);
    fread(&nCol, sizeof(int), 1, file);
    printf("%d , %d \n", nRow, nCol);

    int i;
    int j;

    double* currentDigit = (double*)malloc(sizeof(double));
    int count;

    for (i = 1; i <= nRow; i++) {
        if (i == 1) { // initialize the matrix
            matrice->nRows = nRow;
            matrice->nCols = nCol;
            matrice->rowFirst = rows;
        }

        rows = pushRow(NULL, &i, matrice);
        count = 0;
        for (j = 1; j <= nCol; j++) {
            fread(currentDigit, sizeof(double), 1, file);

            if (*(currentDigit) != 0.0) {
                count++;
                if (count == 1) { // begin a new column list on the next node of
                                  // the row list
                    cols = NULL;
                    rows->data = cols;
                }
                cols = pushCol(&j, currentDigit, cols);
            }
        }
    }

    fclose(file);
    free(currentDigit);
    return matrice;
}

row* pushRow(col* data, int* rows, matrix* top) {
    row* f = NULL;
    f = (row*)malloc(sizeof(row));

    if (f == NULL) {
        exit(EXIT_FAILURE);
    }

    f = top->rowFirst;

    if (f->indiceRow == 0) {
        printf("null");
        f->data = data;
        f->indiceRow = *(rows);
        f->next = NULL;
    }
    else {
        while (f->next != NULL) {
            f = f->next;
        }
        row* temp = NULL;
        temp = (row*)malloc(sizeof(row));
        f->next = temp;
        f->next->indiceRow = *(rows);
        f->next->data = data;
        f = f->next;
    }

    return f;
}

col* pushCol(int* column, double* value, col* dataRow) {
    col* f = NULL;
    f = (col*)malloc(sizeof(col));

    if (f == NULL) {
        exit(EXIT_FAILURE);
    }

    f = dataRow;
    if (f->indiceColumn == 0) {
        f->indiceColumn = *(column);
        f->value = *(value);
        dataRow = f;
    }
    else {
        while (f->next != NULL) {
            f = f->next;
        }
        col* temp = NULL;
        temp = (col*)malloc(sizeof(col));
        f->next = temp;
        f->next->indiceColumn = *(column);
        f->next->value = *(value);
        f = f->next;
        dataRow = f;
    }

    return dataRow;
}

The program execute the printf of the nRows and nCols at the line 35 but then I receive a segmentation fault (core dumped).

Answer 1

Since you've not shown us the structure definitions for row, col or matrix, it is not possible to reliably compile the code. That's bad in a question. It also means it is hard to guess how the data is supposed to be organized. It seems you have a value for every data item in the disk file, but there are enough zeros in the data that it is worth making a sparse matrix (based on the hint in the missing header file name).

In main(), you have a memory leak:

matrix* tempMatrix = (matrix*)malloc(sizeof(matrix));
tempMatrix = sp_read("mat_100x5");

You assign tempMatrix from malloc(), and then overwrite it with the result of sp_read(). However, that is not the core dump problem.

In sp_read(), you should check that the fopen() worked before you use the returned file pointer. However, you'd be crashing on the first fread() if it didn't work, so you are getting away with it at the moment, but it is bad practice not to check fopen() because it fails. Similarly, you should be checking each and every fread() to ensure you get data. If you don't, you should not continue assuming you got values. Similarly, you should check the result of every memory allocation and avert crashes if the allocation fails.

In C, array bounds go from 0 .. N-1, not 1 .. N. You have:

for (i = 1; i <= nRow; i++) {
    if (i == 1) { // initialize the matrix
        matrice->nRows = nRow;
        matrice->nCols = nCol;
        matrice->rowFirst = rows;
    }

The loop should be for (i = 0; i < nRow; i++). Similarly with the loop on columns.

The initialize code should be outside the loop.

The code in pushCol() is another set of memory leaks:

col* pushCol(int* column, double* value, col* dataRow) {
    col* f = NULL;
    f = (col*)malloc(sizeof(col));

    if (f == NULL) {
        exit(EXIT_FAILURE);
    }

    f = dataRow;

You assign the result of a malloc() to f; then you overwrite f with the value in dataRow, thereby leaking the memory you just allocated. Initializing f to null and then assigning the result of malloc() is marginally pointless (but the most basic optimizing compiler will fix that for you). Simply assign the result of the malloc() directly to f.

The code in pushRow() is similarly leaking memory:

row* pushRow(col* data, int* rows, matrix* top) {
    row* f = NULL;
    f = (row*)malloc(sizeof(row));

    if (f == NULL) {
        exit(EXIT_FAILURE);
    }

    f = top->rowFirst;

The interface to pushCol() should be simpler; neither the column number nor the value needs to be a pointer — you can pass both by value. The interface to pushRow() could take int rows instead of int *rows.

Incidentally, writing:

 cols->value = 0, 0;

is a minor waste; it assigns 0, but there is no need for the comma operator. (This may be confusion with writing 0,0 for zero in your native locale, but in C, you need to write 0.0 if you include a decimal point, and the space would be erroneous if it were not for the use of the comma operator.)

---

I generated a set of definitions for the sparse matrix type like this:

typedef struct col col;
typedef struct row row;
typedef struct matrix matrix;

struct col
{
    int     indiceColumn;
    double  value;
    col    *next;
};

struct row
{
    int  indiceRow;
    col *data;
    row *next;
};

struct matrix
{
    row *rowFirst;
    int  nRows;
    int  nCols;
};

I also wrote a program to generate an identity matrix in a file, which is pretty sparse:

#include <stdio.h>

int main(void)
{
    int nRows = 5;
    int nCols = 10;

    fwrite(&nRows, sizeof(nRows), 1, stdout);
    fwrite(&nCols, sizeof(nCols), 1, stdout);
    for (int i = 0; i < nRows; i++)
    {
        for (int j = 0; j < nCols; j++)
        {
            double d = (i == j) ? 1.0 : 0.0;
            fwrite(&d, sizeof(d), 1, stdout);
        }
    }
    return 0;
}

With this in a data file, the program starts but crashes in pushCol() dereferencing f (if (f->indiceColumn == 0)), strongly indicating a null pointer.

Looking at the calling code:

        if (*(currentDigit) != 0.0) {
            count++;
            if (count == 1) { // begin a new column list on the next node of
                              // the row list
                cols = NULL;
                rows->data = cols;
            }
            cols = pushCol(&j, currentDigit, cols);

We can see that cols when passed to the function is a null pointer. Inside the function, that is the parameter dataRow, which is assigned to f, so you are dereferencing a null pointer. That's not a big surprise; it is a very common cause of crashes.

To Fix

You are going to need to revisit all your memory allocation code, making sure you are not leaking memory, and deciding more carefully which code is supposed to allocate which data. Frankly, it is something of a mess at the moment — it is rather hard to follow the intended logic. It looks as if each row should be a linked list of non-zero columns. With the data for the identity matrix, each row would have a single column value linked from it. But I'm not convinced you've allocated enough space for all the row structures, or allocated the columns properly.