Dynamically sized arrays in C

Question

I'm attempting to learn C using http://c.learncodethehardway.org/ but I'm stuck on one of the extra credit questions in chapter 18 (http://c.learncodethehardway.org/book/learn-c-the-hard-waych18.html) and I'm hoping someone can help me.

The specific problem I'm having is there are a couple of structs defined like this:

#define MAX_ROWS = 500;
#define MAX_DATA = 512;

struct Address {
    int id;
    int set;
    char name[MAX_DATA];
    char email[MAX_DATA];
};

struct Database {
    struct Address rows[MAX_ROWS];       
};     

struct Connection {
    FILE *file;
    struct Database *db;
};

The challenge is to rework that so that rows can have a variable size that doesn't rely on that constant.

So in my Database_create method I'm attempting to initialize rows with the following:

conn->db->rows = (struct Address*) malloc(max_rows * sizeof(struct Address));

where conn->db points to an instance of Database and max_rows is an int that gets passed to the function. I've also changed the Database struct to

struct Database{
    struct Address* rows;
}

That bit of code seems to run ok but if I try and access any member of rows I get a segmentation fault which I believe means I'm attempting to access bits of memory that aren't in use.

I've spent a good few hours on this and I'm sure I can't be too far off but I'd really appreciate any guidance to get me on the right track.

EDIT: Just wanted to add some more detail to this after running it with Valgrind, that throws up the error:

==11972== Invalid read of size 4
==11972==    at 0x100001578: Database_set (ex18.c:107)
==11972==    by 0x100001A2F: main (ex18.c:175)
==11972==  Address 0x7febac00140c is not stack'd, malloc'd or (recently) free'd

The line of code it's point to is:

struct Address *addr = &conn->db->rows[id];
if(addr->set) die("Already set, delete it first");

Line 107 is the if(addr->set) which I presume means it's trying to read something it can't

If you are using c99 compliant compiler they do allow variable length arrays. — Alok Save, Jun 22 '12 at 13:04
As a slight aside to the question, you don't want to cast the return value of malloc (see: http://stackoverflow.com/questions/1565496/specifically-whats-dangerous-about-casting-the-result-of-malloc). — Scroog1, Jun 22 '12 at 13:17
I am always fuzzy on operator precedence between `&` and `->` so when in doubt, surround with parens. Just to be sure, have you tried `struct Address *addr = &(conn->db->rows[id]);`? — Dan F, Jun 22 '12 at 13:22
@Scroog1 Reading the accepted answer of that question, it looks like you just don't want to use malloc without including stdlib.h. — Eric Finn, Jun 22 '12 at 13:24
`struct Address *addr = &conn->db->rows[id];` Can I see the definition of `conn`? — Eric Finn, Jun 22 '12 at 13:25
I've added the struct definition for conn, I've also tried adding the parenthesis and that didn't help either — pogo, Jun 22 '12 at 13:27
@EricFinn that was just the problem that casting caused with the OP's code in the question; there are plenty of other reasons listed there not to cast the result of malloc. — Scroog1, Jun 22 '12 at 13:29
@pogo I meant to ask for the definition of `conn`, not the type of `conn`, although that helps too. It's just that I notice you're using `conn->db->rows` in one place and `&conn->db->rows` in another. Are these the same `conn`? If the `conn` where you use `&conn` is already a pointer, that could explain the error. — Eric Finn, Jun 22 '12 at 14:28
@EricFinn they're in two separate functions and in both cases conn is a parameter for that function, it's defined as struct Connection *conn. If you look at http://c.learncodethehardway.org/book/learn-c-the-hard-waych18.html you'll be able to see the code I'm basing this on if it helps. — pogo, Jun 22 '12 at 14:37
Alright, I see. Try Dan F's suggestion to change that line of code to `struct Address *addr = &(conn->db->rows[id]);` and see if that helps. — Eric Finn, Jun 22 '12 at 14:44
Still not working I'm afraid, if it helps I've dumped the entire contents of what I have here: http://pastebin.com/cckVCyeB — pogo, Jun 22 '12 at 15:01
Unfortunately, pastebin is blocked here, so I can't see it. Good luck, though. — Eric Finn, Jun 22 '12 at 15:17

score 2 · Answer 1 · answered Jun 22 '12 at 13:04

2

You want sizeof(struct Address) not sizeof(struct Address*)

sizeof(struct Address*) is likely returning a size of 4 (completely dependent on the target platform, though) whereas the actual size of the Address struct is closer to 1040 (assuming 1 byte per char and 4 per int)

answered Jun 22 '12 at 13:04

Dan F

17,654
5
72
110

That doesn't seem to have fixed it, I've just updated my original post to reflect that but I've also added the error I get back when I run it through Valgrind which will hopefully shed some more light on what's going on – pogo Jun 22 '12 at 13:19

Eric Finn · Answer 2 · 2012-06-22T13:27:17.653

EDIT: Well, it looks like (with your latest edit), you actually are doing this part properly.

You are not actually allocating enough size for the Address structures. What you need is something like this:

struct Database{
    struct Address** rows;
}
//create array of pointers to Address structures
// (each element has size of the pointer)
conn->db->rows = (struct Address**) malloc(max_rows * sizeof(struct Address*));
for(int i=0; i < max_rows; i++) {
    conn->db->rows[i] = (struct Address*) malloc(sizeof(struct Address));
}

or this:

struct Database{
    struct Address* rows;
}
//create array of Address structures
// (each element has size of the structure)
conn->db->rows = (struct Address*) malloc(max_rows * sizeof(struct Address));

Good answer highlighting why you might want to use `sizeof(struct Address*)` in some situations — Dan F, Jun 22 '12 at 13:43

score 1 · Accepted Answer · answered Jun 22 '12 at 16:16

1

When you're loading

void Database_load(struct Connection *conn)
{
        int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);
        if(rc != 1) die("Failed to load database");
}

or writing the database,

void Database_write(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.");
}

you're not reading or writing the contents (the struct Addresses), but only the pointer to the memory location. When reading a previously written database, that pointer doesn't point at anything specific, it's a wild pointer. Then of course trying to dereference it is very likely to cause a segmentation fault, if perchance it doesn't, you'll get nonsense pseudo-data.

If you change your struct Database so that rows is a struct Address*, you need to keep a count of items and change your reading and writing code to also handle the data pointed to by rows. First write how many items you have, then write the rest (max_data, max_rows and the items); and when reading, read how many items you have, allocate space for them, read max_data and max_rows and the items.

answered Jun 22 '12 at 16:16

Daniel Fischer

181,706
17
308
431

I think this is definitely what's going on, I noticed when I ran the code in the example it creates a large data file but when I run mine it's very small and the segmentation fault happens when trying to read back from it. Do you know of any articles / example code I can look at that would explain how to make sure the file is big enough when it's written to? – pogo Jul 02 '12 at 08:57
The file size isn't the problem, it's just a symptom of the problem. With `char name[MAX_DATA];` (same for `email`) in the struct, the arrays containing the data were part of the struct. So when writing the struct, the name and email were written too (plus some junk, because the name and email were shorter). When you have `char *name; char *email;` in the struct, the struct only contains the address of the data. So when you `write` the struct, you don't write the data, just the address it has in memory (in this run of the programme). That doesn't help you at all, since when you read the data – Daniel Fischer Jul 02 '12 at 09:34
... back in (if you wrote it at all), it will probably reside at a different location. What you need to do is 1. write the `id` and `set`, 2. write the size (length) of the name, and the name itself, 3. write the size (length) of the email and the email itself. And when reading back in, you need to read `id` and `set`, the size of the name, allocate a buffer of appropriate length, read the name into that buffer, set the `email` field to the address of the buffer, read the size of the email, allocate another buffer, read the email into that and set the `email` field to the address of the buffer – Daniel Fischer Jul 02 '12 at 09:40

Dynamically sized arrays in C

3 Answers3