MEM02-C. Immediately cast the result of a memory allocation function call into a pointer to the allocated type

First, it seems to me that the correct recommendation should be: don't use implicit declarations for malloc, or any other function.  i.e., don't use malloc without #including stdlib.h.   For one thing, implicit function declarations are unsafe in general (e.g. if void* is not the same size as int), and for another thing they have been removed from the ANSI C99 standard.

Second, it seems to me that the risk assessment is wrong: "Explicitly casting the return value of malloc() eliminates the warning for the implicit declaration of malloc()."  This statement about compiler warnings is compiler-dependent; for example, gcc 4.1.2 gives a warning for uses of implicitly declared malloc() whether or not you explictly cast the return value. The basic point is that, if you use the implicit declaration of malloc(), you are inviting trouble via compiler- and architecture-dependent behavior no matter whether you cast the result or not.

Third, it seems like a bad habit to write C code that is gratuitously incompatible with C++ like this. Particularly since one platform's primary compiler (Microsoft Visual C++ ) is C++ only. 

Instead of using sizeof(type), use sizeof *p, like so:

widget *p;
...
p = malloc(sizeof *p);

Presto -- no type mismatch issues, no need for casting.   You allocate the right amount of memory every time. 

I really, really, really do not like the idea of casting the return value of malloc().  Furthermore, for ADTs I'd recommend creating constructor- and destructor-like functions, and abstract away the calls to malloc() and free():

struct widget;
typedef struct widget widget_t;
 
struct gadget;
typedef struct gadget gadget_t;
 
widget_t *newWidget(void)
{
    widget_t *p = malloc(sizeof *p);
    if (p) 
        /* initialize members of *p as necessary */
    return p;
} 
 
gadget_t *newGadget(void)
{
    gadget_t *p = malloc(sizeof *p);
    if (p)
        /* initialize members of *p as necessary */
    return p;
}
 
void deleteWidget(widget_t **p)
{
     /* delete any subelements of *p */
     free(*p);
     *p = NULL;
}
 
void deleteGadget(gadget_t **p)
{
    /* delete any subelements of *p */
    free(*p);
    *p = NULL;
}
 
...
 
widget_t *p = newWidget();
gadget_t *g = newGadget();

if (p)
    /* do stuff with p */
 
if (g)
    /* do stuff with g */
...
deleteWidget(&p); 
deleteGadget(&g); 

I submit that this method is safer than using macros.  The tradeoff is that it's a bit more work, but I think it's worth the effort. 

So, my recommendations would be as follows:

• Use the size of the object being allocated when calling malloc, instead of the size of the type;
• Abstract away the calls to malloc() and free() by using creator and destroyer functions for ADTs. 

It may be useful to note that the multiplication operation used in the MALLOC_ARRAY macro should be checked for overflow if the user can manipulate the number of elements in the array.

I'm disturbed by the first 'compliant solution' which generates a compiler error. It's an odd sort compliance.

I sympathize with those who dislike casting return values from malloc et al. However, in my own code, I write in the C-compliant subset of C++, so the majority of my code can be compiled with a C or C++ compiler. To make memory allocation calls compile, I have to cast the return value, so I do, and I annotate it /*=C++=*/ to indicate why the cast is there. That doesn't make it foolproof - but does minimize the damage IMO.

I also use a CONST_CAST(type,value) macro with the C implementation ((type)(value)) and the C++ implementation const_cast(value). The C++ compilations validate that all the cast does is cast away const-ness. In C, it documents what is going on, though I could make mistakes.

I am rather concerned by the recommendation in this page. It is unsafe, dangerous, and worse it can result in undefined behavior!

This page seems to concern itself too much with logic errors rather than noncompliant code problems.  The main reason for NOT using a cast is it hides errors - worse it hides undefined behavior.

 A lot of a code still compiles under C89 mode. Probably more C89 code is written every day than any other version of the standard. Now, to understand the issue at hand we must understand C89 function declaration.

First, Let's see what the standard has to say about this

C89; §3.3.2.2 Function calls:

If the expression that precedes the parenthesized argument list in a function call consists solely of an identifier, and if no declaration is visible for this identifier, the identifier is implicitly declared exactly as if, in the innermost block containing the function call, the declaration:

extern int identifier();

appeared.

What's it saying? Well, if a function is called and the compiler has not seen an explicit prototype for it, the compiler is to assume it's a function that takes an unspecified number of arguments and returns an int.

So what does all of this have to do with casting malloc()?

Good question; it's rather common to forget to include <stdlib.h>. This is actually the root of the problem.

So what happens when we don't have an explicit prototype for malloc()?

As the standard states, it's assumed to be:

extern int malloc();

And here is where your troubles began. Let's take a look at the standard again: 

C89; §3.3.4 Cast operators

Semantics

Conversions that involve pointers (other than as permitted by the constraints of $3.3.16.1) shall be specified by means of an explicit cast; they have implementation-defined aspects: A pointer may be converted to an integral type. The size of integer required and the result are implementation-defined. If the space provided is not long enough, the behavior is undefined. An arbitrary integer may be converted to a pointer. The result is implementation-defined./37/ A pointer to an object or incomplete type may be converted to a pointer to a different object type or a different incomplete type. The resulting pointer might not be valid if it is improperly aligned for the type pointed to.

I am sure you can start to see where this is going. Let's consider the following program:

int main(void)
{
    char *foo = (char*)malloc(10);
    free(foo);
    return 0;
}

This program may seem right, right? WRONG! since we forgot to include <stdlib.h>, our program is seen by the compiler more like this:

extern int malloc();
extern int free();
int main(void)
{
    char *foo = (char*)malloc(10);
    free(foo);
    return 0;
}

Now, what's going on in this program. Firstly, the actual malloc() implementation is returning us a void*, in our program, the compiler knows it as an int. It is now allowed to handle it as an int for all practical purposes. This is a problem. If we are lucky and the space provided is long enough, the result is implementation-defined. If we are not so lucky and the space provided is not longer enough, the result behavior is undefined. A simple implementation where a void* is 64-bits and your int is 32-bit WILL invoke UB.

Now we are getting to why the cast is *HORRIBLE*. The standard says that such conversion can only be done by the means of an explicit cast. Your recommendation is masking this error by means of making illegal code legal. If we left the cast out like you should, a conversion from `int` to `char*` will generate some diagnostic message telling you there is a problem - since it's illegal in C. Hopefully such diagnostic message should give the programmer a hint that something is wrong.

I also saw a note about compiling the code as C++. How C++ handles things should be not be mixed with how C handles things, this is not a vegetable soup.

The recommendations on this page are very harmful to the unsuspected stranger.

It is my opinion that this recommendation does more harm than good. This recommendation does not solve any issue in any meaningful way.

Most C programmers would probably tell you that a better recommendation is to use sizeof *ptr rather than sizeof (type) which would easily solve the problem shown in this page - it would not however solve the problem I've explained above which only NOT using a cast will help. 

widget *p;
/* ... */
p = malloc(sizeof *p);

No possible for memory issues, type issues, and bonus no need to cast - unhiding the correct problem. I just really really REALLY hate the fact you guys recommend casting malloc(), it's a total sin.

I think a middle ground that could be reached is to rename or change this page (I don't know how you guys organize those things) to a set of guidelines with explanations about the various issues involving malloc.

David,

I guess I could have deleted this rule years ago as "controversial" but it sure generates alot of interesting discussion.

We initially developed this coding standard for C99, and then published the book for this version of the standard.  We are now updating this standard for C11.

We will make notes occasionally when our recommendations might be different for an older version of the standard.  I think that is where Svoboda and Aaron were going.  Although MSVC doesn't support C99, it is an important platform so we need to pay attention to it.

The erroneous code you wrote violates:

DCL31-C. Declare identifiers before using them

This is actually a rule and not a recommendation like this one (MEM02-C).   When we give noncompliant examples, we assume they are free of other problems that are prevented by other rules.

I'm thinking there may be some value in a historical note for this rule, discussing some of the problems with C89/90.  I don't think we should reverse the guidance (again).  The other possibility is to scrap this as controversial and write a paper titled "malloc, the casting controversy".

Yes, and because we are splitting hairs, we probably don't want to suggest to an analyzer that we consider the above code to be noncompliant and have them flagging lines without casts.

I'm having a few issues with this recommendation.  The title of this recommendation implies that our recommendation is to cast the result of malloc to a pointer of the allocated type. If this is the case then in the 3rd box of our non-complant code example where

*widget p;

p = (gadget *)malloc(sizeof(gadget));

is valid in the context of this recommendation because the result of mallocing space for a gadget is immediately cast into a pointer to a gadget. This can cause UB because it violates EXP39-C. Do not access a variable through a pointer of an incompatible type and MEM35-C. Allocate sufficient memory for an object.

I view the CS of

widget *p;

...

p = malloc(size of *p);

as a good compliant solution so I don't think this recommendation should require that we immediately cast the result of malloc into a pointer of the right type.

Alternatively, use this macro in order to automatically allocate values of correct type, without having to use sizeof.

#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#define alloc(lvalue, size) ((lvalue) = malloc(sizeof *(lvalue) * (size)))
int main(void) {
    const int test[] = {1, 2};
    int *copy;
    alloc(copy, 2);
    memcpy(copy, test, sizeof test);
    copy[0] = 3;
    printf("%d %d %d %d\n", test[0], test[1], copy[0], copy[1]);
    free(copy);
    return 0;
}

Do we need an exception for strings? Casting a malloc return value to char* seems useless (as char* is also convertable to any other type).