Trouble using fread() to convert a binary file to a struct

Question

I'm having trouble using fread() to convert a binary file into a linked list of structs.

The struct:

struct MdbRec {
    char name[16];
    char  msg[24];
};

Relevant code:

    FILE *file;
    file = fopen( argv[1], "rb" );

    struct List db;
    initList(&db);
    struct MdbRec *data = (struct MdbRec *)malloc(sizeof(struct MdbRec));
    struct Node *last;
    while( fread( data, 40, 1, file ) )
    {
            struct MdbRec *message = (struct MdbRec *)malloc(sizeof(struct MdbRec));
            message = data;
            if( !db.head )
            {
                    last = addFront( &db, message );
                    db.head = last;
            }
            else
                    last = addAfter( &db, last, message );
            if( fseek( file, 40, SEEK_CUR ) != 0)
                    break;
            printf("read\n");
    }
    free(data);
    removeAllNodes( &db );

addFront() and addAfter are methods of the linkedlist structure that mallocs space the data field.

When I run it with Valgrind it shows that I'm successfully having 2 allocations of memory. One is obviously the data variable. The other 568 bytes and it's very confusing to me. Valgrind says the error is coming from when I run fread().

` message = data;` will overwrite the value you assigned to message via the initialiser. — wildplasser, Oct 21 '12 at 19:58
@Soke, he's just as disappointed in you if you're finding this question ;) — Nate Brennand, Feb 26 '16 at 17:08
I found this googling "MdbRec" because I was confused what "Rec" stood for :P — MT_, Feb 26 '16 at 19:09

score 1 · Accepted Answer · edited May 23 '17 at 12:07

This is a memory leak:

struct MdbRec *message = (struct MdbRec *)malloc(sizeof(struct MdbRec));
message = data;

as message is now pointing to data and no longer points to the just malloc()d memory, which is now unreachable. I suspect you actually meant to copy data to message:

*message = *data;

Other points:

Check result of fopen() before using it.
There appears to be no reason to not use a stack allocated object for data, which would avoid needless dynamic allocation management for it.
The fread() argument that specifies the size of the object to read, 40, is error prone. Any changes to struct MdbRec would break it: use sizeof(struct MdbRec) instead.
Casting the return value of malloc() is not necessary and possibly dangerous (see Do I cast the result of malloc?).

Thanks, this seems to have worked. I added the fopen() check, though the issue wasn't there. The other errors were things that initially had right but changed in attempts to debug this. — Nate Brennand, Oct 21 '12 at 20:27

score 1 · Answer 2 · answered Oct 21 '12 at 19:59

1

struct MdbRec *message = (struct MdbRec *)malloc(sizeof(struct MdbRec));
message = data;

You reallocated a struct MdbRec and discarded it.

answered Oct 21 '12 at 19:59

quantum

3,672
29
51

As hmjd pointed out, I meant to copy data to message. I didn't intend to discard it. My mistake, thanks for the help. – Nate Brennand Oct 21 '12 at 20:28

Trouble using fread() to convert a binary file to a struct

2 Answers2