C Memory Allocation: Fixed-length array in a struct in a struct

Question

Edit [2016-08-26-001]: I've corrected the errors as pointed out in the comments. As I mentioned below, I quickly grabbed and modified some of the code just to illustrate the problem, sorry :(

The following is a modified extract from my original source (which is still a mess!!!) but it should do:

#define STORAGE_LIMIT_NAME  64
#define DB_INIT_ENTRIES     100

struct entry {
        char fname[STORAGE_LIMIT_NAME];
        char sname[STORAGE_LIMIT_NAME];
};

struct database {
        struct entry *data;
        unsigned int entries;
        unsigned int entrysz;
};

struct database mydb;

mydb.entrysz = sizeof(struct entry);
mydb.entries = DB_INIT_ENTRIES;
mydb.data = malloc(mydb.entrysz * DB_INIT_ENTRIES);

free(mydb.data);

I need to know the following:

1) Am I doing it right? That is, is the memory all being allocated to the heap and thus being freed? I don't want a memory leak.

2) Is there a more elegant solution that doesn't over-complicate things and still allows easy limiting of name lengths? Also, it would be preferably fast.

Sorry if this has been asked before, I've looked all over this site and Google, but all I've found is other people occassionally doing vaguely similar things while asking completely different questions. This was not included in any of the study guides I've looked at online so far. I need to understand this if I want to get better as a programmer and I'm self-taught so I don't have a lecturer to run to. By the way, any links to good free C (not C++) study material is welcomed. Thanks lots.

for one you are using malloc incorrectly, check doc (http://stackoverflow.com/questions/7536413/why-calloc-takes-two-arguments-while-malloc-only-one) — Giorgi Moniava, Aug 26 '16 at 12:51
`mydb.data = malloc(mydb.entrysz, DB_INIT_ENTRIES);` --> `mydb.data = malloc(mydb.entrysz * DB_INIT_ENTRIES);` — BLUEPIXY, Aug 26 '16 at 12:54
@GiorgiMoniava: Was a typo because I just threw this together illustratively. In my original source I did use calloc (which I always use for strings because it saves me using memset.) — jskellis, Aug 26 '16 at 15:02
@pmg: I know, this is just illustrative, I didn't do a number of things (like best practices) I should have, and I do in the original source. There isn't even a main() function. — jskellis, Aug 26 '16 at 15:03
@jskellis: this is why it's paramount to always post *actual code* - either your original code, if it's sufficiently small and self-contained, or better still a [mcve]. Otherwise people end up chasing non-existent problems, which can be very inefficient and time-consuming. — Paul R, Aug 26 '16 at 15:46
@PaulR: Ah, oops, I seem to have left out the V (as in Verifiable part). Sorry, I'm a noob. I'll be more careful next time. Thanks for that link, too. — jskellis, Aug 28 '16 at 12:11
@jskellis: no problem - we were all noobs at one time or another. ;-) — Paul R, Aug 28 '16 at 12:12

score 2 · Accepted Answer · answered Aug 26 '16 at 15:10

1) Yes, your code here allocates enough space for an array of DB_INIT_ENTRIES structs, each being sizeof(struct entry) bytes in size:

mydb.entrysz = sizeof(struct entry);
mydb.entries = DB_INIT_ENTRIES;
mydb.data = malloc(mydb.entrysz * DB_INIT_ENTRIES);

This call then frees that entire array:

free(mydb.data);

What you do not show is how to access each struct within the allocated array:

struct entry *  entp;     // Entry pointer
entp = &mydb.data[i];     // Where 0 <= i < mydb.entries

Tangent

As a matter of programming style (which has nothing to do with your question), I prefer to make it explicit that a character array is intended to hold a null-terminated string by adding an explicit +1 to the array size, to wit:

#define STORAGE_LIMIT_NAME  63

struct entry {
    char fname[STORAGE_LIMIT_NAME+1];
    char sname[STORAGE_LIMIT_NAME+1];
};

The explicit +1 makes it clear that the name array ends with an extra NUL character. It also makes STORAGE_LIMIT_NAME define the maximum name length in terms of characters, not its allocation size.

[*] BTW, entry used to be a reserved keyword in early C, but was removed when the language was standardized in 1988.

Cool, thanks. That idea with the +1 is something a few people have mentioned to me, but they never mentioned that it could help me limit the *actual* length too. It's nice to be explained properly! Also, I have no idea about "entry", tbh, but I'll keep in mind to be more careful before I name things (maybe use "my" as a prefix for everything, lol). — jskellis, Aug 28 '16 at 12:18

score 1 · Answer 2 · answered Aug 26 '16 at 16:21

1

Yes, there are no leaks. I encourage you to get familiar with Valgrind, which is commonly used for memory leaks.

Also, I would recommend to always typecast your malloc to your specified data type! Don't leave it to the compiler.

And, yes I agree with both the answers above to have +1 bytes to the space for the Null character.

Bonus Material:

Get familiar with GDB, as it is the life saver for many programmers. It's a great debugger and you can be sure that your code runs/does as you expect. I am attaching a cheat sheet for GDB and Valgrind.

GDB

Valgrind

answered Aug 26 '16 at 16:21

Dark Innocence

1,389
9
17

Thanks for the useful links!!! I didn't cast malloc() because I read somewhere that you only need to cast calloc() and that casting malloc was wrong/bad/pointless. It seems there are differing opinions on this (just to add more confusion to my life). Do you perhaps have a link to some material explaining the situation to me? Thanks – jskellis Aug 28 '16 at 12:23

score 0 · Answer 3 · edited May 23 '17 at 10:29

1) Yes, there are no leaks.
Your malloc() allocates in the heap mydb.entrysz * DB_INIT_ENTRIES bytes, which can be abstracted as an array of struct entry with size DB_INIT_ENTRIES.

The free(mydb.data) knows how many bytes were dynamically allocated on mydb.data, so it frees all those bytes.

Additionally you should set mydb.data to NULL after the free():
Setting variable to NULL after free

2) If your struct entry stores binary data, I would do exactly as you, but if the names are c strings, I would allocate one more char to the ending '\0' in each array:

struct entry {
    char fname[STORAGE_LIMIT_NAME+1];
    char sname[STORAGE_LIMIT_NAME+1];
};

This way STORAGE_LIMIT_NAME represents the number of bytes you can store.

Thanks. I would normall set a pointer to NULL after free(), the code was just to illustrate the thing that was confusing me a bit. — jskellis, Aug 28 '16 at 12:25

C Memory Allocation: Fixed-length array in a struct in a struct

3 Answers3