There are several things to consider when calling the malloc(3) family of functions:

• nelem * sizeof(elem) or sizeof(elem) * nelem?
• Use the type or the pointer name in sizeof()?
• To cast or not to cast?

TL;DR:

Use a simple and readable interface, and let it handle the safety for you. Of course, you need to write such an interface, wrapping the standard C library functions.

foo_t  *p;

p = MALLOC(nelem, foo_t);

Let's answer the questions one by one.

nelem * sizeof(elem) or sizeof(elem) * nelem?

None of the alternatives is good.

Use functions that take separate arguments:

• calloc(3) (ISO C)
• reallocarray(3) (several Unix systems)
• recallocarray(3) (several Unix systems)

I suggest writing your own mallocarray() and reallocarrayf() as wrappers around these. reallocarrayf() is more complex (read about reallocf(3)), but mallocarray() can be trivially implemented:

#define mallocarray(nmemb, size)  reallocarray(NULL, nmemb, size)

As a rule, I always use array allocation functions, even if I only want 1 element:

foo_t *p;

// p = MALLOC(1, foo_t);  // which expands to:

p = (foo_t *) mallocarray(1, sizeof(foo_t));

This has several benefits:

• It clearly shows the number of elements I want. If it's 1, or if it's 42.

  An exception is when allocating a structure with a flexible array member. In such case, you really need to call malloc(3) with the precise size you want:

struct foo {
    int    a;
    bar_t  b[];
};

struct foo *p;

p = (struct foo *) malloc(sizeof(struct foo) + sizeof(bar_t) * n);

• The multiplication is performed inside the function, inside a check to prevent overflow.

  However, as a general rule, when you multiply numbers, put first the value with the widest type, to avoid overflowing intermediate operations (read about precedence in operators).

int     a, b;
size_t  c, d;

...

d = a * b * c;  // might overflow `a * b` easily
d = c * a * b;  // less likely

Use the type or the pointer name in sizeof()?

None of the alternatives is good.

If you use the type, then if you change the type of the pointer might introduce silent bugs in malloc(3) calls, since there's no way that the compiler knows if your doing the right thing.

If you use the name, then you reduce ability to search in the code. Code is more often read (and searched) than modified. It's quite useful to be able to search through a code base for all allocations of a given type with something like the following.

grep -rni 'alloc.*\bfoo_t)' src

The solution comes thanks to the next question, which is about casting. As a starter, I'll say that in this case readability is preferable to safety; we'll try to make it safe somehow... continue reading.

To cast or not to cast? That is the question.

The unsurprising answer, again, is that none of the alternatives is good.

Casting, in general, can disable many warnings, and is a dreaded thing that should be used very carefully. However, that doesn't apply to malloc(3), since it (by returning void *) implicitly converts to any pointer type, so we can't do more damage by forcing a conversion.

Casting also had issues regarding implicit functions, but those are gone for good (since C99), so this is a non-issue. If you lived in a cave for the last $year - 1999 years, please get out and see the light.

Casts still have issues, which is why they're not ideal:

• Clutters the code, considerably reducing readability.
• They aren't DRY.

So let's not write the cast (which is not the same as don't cast).

Casting the result of malloc(3) is good because:

• It couples the call with the type of the pointer. Since the cast converts the void * to a non-void pointer, it no longer converts to anything implicitly. This means that if the type of the pointer ever changes, malloc(3) calls assigned to that pointer will no longer compile, and we will be able to fix any compilation errors.

So, the solution is to write the cast inside a macro, which uses the same information that we will need for sizeof(), thus avoiding repeating ourselves, and also avoiding readability issues.

#define MALLOC(n, type)  ((type *) mallocarray(n, sizeof(type)))

which is to be used as this:

foo_t  *p;

p = MALLOC(42, foo_t);  // 42 elements of type foo_t.

It's readable, it's concise, it's easily searchable, and it's safe.

posted over 1 year ago

CC BY-SA 4.0

1y ago

alx‭

628 reputation 16 10 91 129