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
– Alok Save, Commented Jun 22, 2012 at 13:04
As a slight aside to the question, you don't want to cast the return value of malloc (see: stackoverflow.com/questions/1565496/…). — Scroog1
– Scroog1, Commented Jun 22, 2012 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
– Dan F, Commented Jun 22, 2012 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
– Eric Finn, Commented Jun 22, 2012 at 13:24
struct Address *addr = &conn->db->rows[id]; Can I see the definition of conn? — Eric Finn
– Eric Finn, Commented Jun 22, 2012 at 13:25

Dan F · Accepted Answer · 2012-06-22 13:04:50Z

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, 2012 at 13:04

Dan F

17.7k6 gold badges76 silver badges111 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

pogo Over a year ago

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

Daniel Fischer · Accepted Answer · 2012-06-22 16:16:17Z

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, 2012 at 16:16

Daniel Fischer

184k19 gold badges319 silver badges436 bronze badges

3 Comments

pogo Over a year ago

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?

Daniel Fischer Over a year ago

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 Over a year ago

... 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

Eric Finn · Accepted Answer · 2012-06-22 13:27:17Z

1

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));

edited Jun 22, 2012 at 13:27

answered Jun 22, 2012 at 13:19

Eric Finn

9,0853 gold badges36 silver badges43 bronze badges

1 Comment

Dan F Over a year ago

Good answer highlighting why you might want to use sizeof(struct Address*) in some situations

Collectives™ on Stack Overflow

Dynamically sized arrays in C

3 Answers 3

1 Comment

3 Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

3 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related