Read and write struct in C

Question

I have a struct:

typedef struct student
{
        char fname[30];
        char sname[30];
        char tname[30];
        Faculty fac;
        int course;
        char group[10];
        int room;
        int bad;
} Student;

I read it from the file:

Database * dbOpen(char *fname)
{
        FILE *fp = fopen(fname, "rb");
        List *lst, *temp;
        Student *std;
        Database *db = malloc(sizeof(*db));

        if (!fp)
                return NULL;

        FileNameS = fname;

        std = malloc(sizeof(*std));
        if (!fread(std, sizeof(*std), 1, fp)) {
                db->head = db->tail = NULL;
                return db;
        }

        lst = malloc(sizeof(*lst));
        lst->s = std;
        lst->prev = NULL;
        db->head = lst;
        while (!feof(fp)) {
                fread(std, sizeof(*std), 1, fp); 
                temp = malloc(sizeof(*temp));
                temp->s = std;
                temp->prev = lst;
                lst->next = temp;
                lst = temp;
        }
        lst->next = NULL;
        db->tail = lst;

        fclose(fp);

        return db;
}

And I have a problem... At the last record i have a such file pointer: `fp 0x10311448 {_ptr=0x00344b90 "НННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННННН _ _iobuf *

` And i read last record 2 times...

Save file code:

void * dbClose(Database *db)
{
        FILE *fp = fopen(FileNameS, "w+b");
        List *lst, *temp;

        lst = db->head;
        while(lst != NULL) {
                fwrite(lst->s, sizeof(*(lst->s)), 1, fp);
                temp = lst;
                lst = lst->next;
                free(temp);
        }
        free(db);
        fclose(fp);
}

Answer 1

First pass without digging too deep yet, I'd say you've written pass the end of your record. The output you show looks a lot like debug memory output that you would see trailing a malloc buffer. Perhaps your database isn't aligned quite correctly? Looking some more now...

Answer 2

A couple of observed and seeming problems for you to consider:

(1) What if your file's length in bytes is not exactly a multiple of sizeof(Student)? Then this code:

while (!feof(fp)) {
    fread(std, sizeof(*std), 1, fp);
    ...

could read a partial struct into the memory pointed to by std. The contents of std would be partially filled and could leave an unterminated C string in memory.

You must check the return value of fread().

(2) This a more important issue. You are re-using the memory allocated and pointed to by std, although you store the pointer in each element of your linked list.

It seems that your intention would be to allocate new memory for each Student record. In this case, you would call malloc() within the body of the loop which reads multiple records from the file.

This error depends on the file length, also -- the file must contain more than one record.

(3) You could reorganize your code so that the first record is also read during a loop iteration, to remove needlessly duplicated code.

(4) You might consider using calloc() to ensure that you have initialized all the records of Student.

Like so:

Database * dbOpen(char *fname)
{
    FILE *fp = fopen(fname, "rb");
    List *lst, *temp;
    Student *std;
    Database *db = NULL;

    if (!fp)
            return db;

    FileNameS = fname;

    db = malloc(sizeof(*db));
    db->head = NULL;
    lst = NULL;
    while (!feof(fp)) {
            std = malloc(sizeof(*std));
            if (!fread(std, sizeof(*std), 1, fp))
            {
                free(std);
                fprintf(stderr, "Input file concludes in partial record.\n");
                break;
            }

            temp = malloc(sizeof(*temp));
            temp->s = std;
            temp->prev = lst;
            temp->next = NULL;

            if (lst == NULL)
                db->head = temp;
            else
                lst->next = temp;
            lst = temp;
    }
    assert(lst->next == NULL ); /* Now performed above by temp->next assignement. */
    db->tail = lst;

    fclose(fp);

    return db;
}

I didn't compile and test the above code, but it should be close to working. Notice how a special case is added to either initialize db->head (first time through the loop, with lst equal NULL), otherwise the previous list element is linked to the newly added element (later iterations). Of course we also should be checking the return values from malloc(), but that is left out for clarity here.

Answer 3

In this loop:

    while (!feof(fp)) {
            fread(std, sizeof(*std), 1, fp); 
            temp = malloc(sizeof(*temp));
            temp->s = std;
            temp->prev = lst;
            lst->next = temp;
            lst = temp;
    }

you are using the result of the fread without checking that it returned successfully. You should check feof(fp) or the return value of fread before assuming the data was successfully read.

Answer 4

This stands out to me:

while (!feof(fp)) { 
            fread(std, sizeof(*std), 1, fp);  
            temp = malloc(sizeof(*temp)); 
            temp->s = std; 
            temp->prev = lst; 
            lst->next = temp; 
            lst = temp; 
        } 

You should not use feof() as a loop condition; the end-of-file indicator is not set until after you attempt to read past the end of the file, so your loop will execute one too many times. Restructure your loop to use the return value of fread(), and check against feof() only if fread() fails:

while (fread(std, sizeof *std, 1, fp) == 1)
{
  temp = malloc(sizeof *temp);
  temp->s = std;
  ...
}
if (feof(fp)) 
  // handle end of file
else
  // handle other read error