saving struct with pointer to disk seg faults

[9three]

Hello,

I have structs that are being saved to disk but when I try to access the pointers in the struct, it causes a seg fault:

struct Address {
    int id;
    int set;
    char *name;
};

struct Connection *open_database(const char *filename, char mode) {
    struct Connection *conn = malloc(sizeof(struct Connection));

    if (!conn) {
        die("Memory error");
    }

    conn->db = malloc(sizeof(struct Database));

    if (mode == 'c') {
        conn->file = fopen(filename, "w");
    } else {
        conn->file = fopen(filename, "r+");

        if (conn->file) {
            load_database(conn);
        }
    }

    if (!conn->file) {
        die("Failed to open the file");
    }

    return conn;
}

void load_database(struct Connection *conn) {
    int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);

    if (rc != 1) {
        die("Failed to load database.");
    }
}

void create_database(struct Connection *conn) {
    int i = 0;

    for (i = 0; i < MAX_ROWS; i++) {
        //initial values
        struct Address addr = {.id = i, .set = 0};

        addr.name = malloc(sizeof(char*));

        conn->db->rows[i] = addr;
    }
}

void set_database(struct Connection *conn, int id, const char *name) {
    struct Address *addr = &conn->db->rows[id];

    if (addr->set) {
        die("Already set, delete it first");
    }

    addr->set = 1;
    addr->name = strdup(name);
}

void write_database(struct Connection *conn) {
    rewind(conn->file);

    int rc = fwrite(conn->db, sizeof(struct Database), 1, conn->file);

    if (rc != 1) {
        die("Failed to write database.");
    }

    rc = fflush(conn->file);

    if (rc == -1) {
        die("Cannot flush database.");
    }
}

void list_database(struct Connection *conn) {
    int i = 0 ;
    struct Database *db = conn->db;

    for (i = 0; i < MAX_ROWS; i++) {
        struct Address *cur = &db->rows[i];

        if (cur->set) {
            print_address(cur);
        }
    }
}

void print_address(struct Address *addr) {
    printf("%d %s\n", addr->id, addr->name);
}

The flow is as follows:

open connection => create_database =>write_database =>set_database =>write_database => list_database

The seg fault happens in when list_database() calls print_address. The code works if I change struct Address pointer *name to be char name[512] (size irrelevant here).

I've been stuck trying to figure out why it causes when I'm allocating space for a char*.

Thanks

[TheBigH]

Hmm. I can't really see where the problem is.
Can you test the following things:
1) Check the return value of strdup. Is it actually returing a valid pointer?
2) In print_address, try just printing addr->id.

[oogabooga]

The problem is that you're saving char pointers to disk and then expecting them to work once you read them back. You have to save the character data. That's why it works when you use an array instead of a pointer.

[9three]

Here's the modifications:

void set_database(struct Connection *conn, int id, const char *name, const char *email) {
    struct Address *addr = &conn->db->rows[id];

    if (addr->set) {
        die("Already set, delete it first");
    }

    addr->set = 1;
    addr->name = strdup(name);

    if (!addr->name) {
        die("unable to strdup name in set_database()");
    }

    addr->email = strdup(email);

    if (!addr->email) {
        die("unable to strdup email in set_database()");
    }
}

void print_address(struct Address *addr) {
    //printf("%d %s %s\n", addr->id, addr->name, addr->email);
    printf("%d\n", addr->id);
}

When adding a new entry, the if statements never evaluates to true. Also, only printing out the id from the struct works correctly.

@oogabooga

Makes sense what you are saying. So I removed the malloc calls from create_database() and only using strdup() when creating a real entry. But it still gives me a seg fault. How can I save the data while also keeping it as a pointer?

void create_database(struct Connection *conn) {
    int i = 0;

    for (i = 0; i < MAX_ROWS; i++) {
        //initial values
        struct Address addr = {.id = i, .set = 0};

        conn->db->rows[i] = addr;
    }
}

thanks

[oogabooga]

Assuming you want to stick to the MAX_ROWS idea (instead of a variable number of rows), and assuming you want to stick to binary files (both of which assumptions you may wish to reconsider), your write_database function could contain something like:

    for (i = 0; i < MAX_ROWS; i++) {
        struct Address *cur = &db->rows[i];
        fwrite(&cur->id, sizeof curr->id, 1, conn->file);
        fwrite(&cur->set, sizeof curr->set, 1, conn->file);
        if (cur->set) {
            len = strlen(cur->name);
            fwrite(&len, sizeof len, 1, conn->file);
            fwrite(cur->name, len, 1, conn->file);
        }
    }

I don't know if you need an email in there as well, but you'd do the same with that (save its length then its data). When reading it back in, do it similarly, reading the length first, mallocing the length + 1, reading in the string data and zero-terminating the string.

Also, you should be opening your file in binary mode (just in case your program is ever run on a system where that makes a difference).

[9three]

Great. This is only for me to practice more than production ready code.

I guess I don't fully understand fwrite as well. Let me try to explain what I think is going on inside the if statement:

get the length (total amount of characters) of cur->name
give the address, the number of bytes of len and conn->file which is a pointer to a FILE to frwrite (I don't understand why you need this one)
give the pointer to char, the length (not bytes) and and conn->file which is a pointer to a FILE

I guess I don't understand your solution fully because fwrite's definition is:

size_t fwrite ( const void * ptr, size_t size, size_t count, FILE * stream );

ptr - Pointer to the array of elements to be written.
size - Size in bytes of each element to be written.
count - Number of elements, each one with a size of size bytes.
stream - Pointer to a FILE object that specifies an output stream

and you're only passing 3 parameters.

[oogabooga]

I guess I don't understand your solution fully because fwrite's definition is:
size_t fwrite ( const void * ptr, size_t size, size_t count, FILE * stream );

You're right! I forgot the count parameter. I've edited the post above and set it to 1 in each fwrite.

[oogabooga]

Let me try to explain what I think is going on inside the if statement:

We're getting the length of the string, then writing the length to the file, then writing the string data. That way when we read it back in we can read in the length, malloc the correct amount of space, then read in the string data.

[9three]

I don't understand the relation between fwrite(&len, sizeof len, 1, conn->file) and fwrite(cur->name, len, 1, conn->file)

Couldn't it just be: fwrite(cur->name, sizeof(cur->name), 1, conn->file)

[quzah]

You can't just write out what your structure stores, because your structure only stores a pointer. You have to instead create a different style of writing. You have to write something that tells you when you are reading it back in what you are supposed to be reading. Why? Because your strings can be of any size. So here's what you do:

1. Write id.
2. Write set.
3. Write an integer that represents the length of the string that is following this integer.
4. Write the actual string out.

Now reading back in, you do this:

1. Read the id.
2. Read set.
3. Read the length value, so you know how much to read from the file to stick in the string.
4. Read that string of that length in.

Now naturally you'll want to just store that length in a temporary variable, and then use a temporary buffer to hold the string you are about to read in. Then you malloc up a block of memory, make your structure point at that block of memory, and copy your string from the temporary buffer to that.


Quzah.

[iMalc]

I don't understand the relation between fwrite(&len, sizeof len, 1, conn->file) and fwrite(cur->name, len, 1, conn->file)

Couldn't it just be: fwrite(cur->name, sizeof(cur->name), 1, conn->file)

There are two ways of storing a variable length string on disk. One is to store the length first, followed by the data, the second is to write an extra termination sequence or character at the end. In other words you could instead do:

fwrite(cur-<name, len+1, 1, conn->file);

The disadvantage of this approach of course is that when reading it in you don't know how long the string is. You have to keep reading until you hit the terminating null char and then for the data you probably either need to go back and read it in again, or use a buffer that is either pessimistically large or resizeable.

Really for a binary file, storing the size up front is the easiest way to go about it.

[9three]

Great, thanks for the responses.

@iMalc

The disadvantage of this approach of course is that when reading it in you don't know how long the string is. You have to keep reading until you hit the terminating null char and then for the data you probably either need to go back and read it in again, or use a buffer that is either pessimistically large or resizeable.

Really? It seems natural to me to do fread(cur->name, sizeof(char*), 1, conn->file). Basically this says "keep reading until you have read sizeof(char*) bytes, then stop. Is that right?

I'm confused when you read back the data. How do I know I'm reading back the correct data? I don't have some flag or anything to let me know that it is.

My initial thought is that, that's what the first pointer in fread/fwrite is for, to help find the correct location/data. But that doesn't seem correct either because

size_t len = strlen(cur->name);
fwrite(&len, sizeof len, 1, conn->file);

The address of len could potentially change when reading cur->name. If that happens, then it would read from the incorrect location, giving me the wrong data. I hope I'm making some sense here!

thank you