Proper way to check malloc return

[Billy Mays]

I typically check my malloc calls like this:

if( !(pointer = malloc(number)) )
{
fprintf(stderr, "An error!\n");
exit(EXIT_FAILURE);
}

.... or something similar. However, I often see people do this:

pointer = malloc(number);
if( number == NULL )
{
fprintf(stderr, "An error!\n");
exit(EXIT_FAILURE);
}


Is there a proper way to check the return value of a malloc call? Are
there any benefits to doing it one way over another?

Bill

 

[Tom St Denis]

I typically check my malloc calls like this:

if( !(pointer = malloc(number)) )
{
     fprintf(stderr, "An error!\n");
     exit(EXIT_FAILURE);

}

... or something similar.  However, I often see people do this:

pointer = malloc(number);
if( number == NULL )

I'm assuming this was supposed to be pointer == NULL ...

{
     fprintf(stderr, "An error!\n");
     exit(EXIT_FAILURE);

}

Is there a proper way to check the return value of a malloc call?  Are
there any benefits to doing it one way over another?

Well generally putting assignments inside other statements is
considered bad form (exceptions being the 'for' keyword).

I'd still check for NULL eitherway though

if ((pointer = malloc(number)) == NULL) {
... error
}

And not use ! on a pointer (though it's probably fine, I don't care to
check though).

Tom

 

[Tim Streater]

I'm assuming this was supposed to be pointer == NULL ...


Well generally putting assignments inside other statements is
considered bad form (exceptions being the 'for' keyword).

I'd still check for NULL eitherway though

if ((pointer = malloc(number)) == NULL) {
... error
}

And not use ! on a pointer (though it's probably fine, I don't care to
check though).

I don't put assignments inside an "if" because I reckon if I'm doing an
"if" then I'm testing something, and I don't expect it to have
side-effects.

I would also be inclined to say:

pointer = malloc (number);
if (pointer==NULL) return NO_MEMORY;

I view it as the caller's responsibility to decide what to do next.

 

[Nick Keighley]

I'm assuming this was supposed to be pointer == NULL ...

nope. There are various ok ways. If its clear its ok. Personnally I
use

if ((pointer = malloc (number)) == NULL)
{
fprintf (stderr, "memory error\n");
exit 9EXIT_FAILURE);
}

as I always have to think twice what

if (!pointer)

actually means

not really. The combined assign and test is a bit shorter. Both are
idiomatic C so shouldn't surprise anyone with more than minimal C
experience.

Well generally putting assignments inside other statements is
considered bad form (exceptions being the 'for' keyword).

really? I thought assignment combined with if/while was pretty common.
I use it with fopen, fgets and anything else where you test immediatly
after assigning.

while ((c = fgetc (in_stream)) != EOF)
process_char (c);

 

[Eric Sosman]

I typically check my malloc calls like this:

if( !(pointer = malloc(number)) )
{
fprintf(stderr, "An error!\n");
exit(EXIT_FAILURE);
}

... or something similar. However, I often see people do this:

pointer = malloc(number);
if( number == NULL )
{
fprintf(stderr, "An error!\n");
exit(EXIT_FAILURE);
}

Is there a proper way to check the return value of a malloc call? Are
there any benefits to doing it one way over another?

Either is fine (with the correction noted by Tom St Denis).
The latter form may be more convenient if you're making multiple
requests and would like to check them all at once:

p = malloc(NP * sizeof *p);
q = malloc(NQ * sizeof *q);
r = malloc(NR * sizeof *r);
if (p == NULL || q == NULL || r == NULL) {
free (p);
free (q);
free (r);
complain();
return BAD_NEWS;
}

.... as opposed to

if ((p = malloc(NP * sizeof *p)) == NULL) {
complain();
return BAD_NEWS;
}
if ((q = malloc(NQ * sizeof *q)) == NULL) {
free (p);
complain();
return BAD_NEWS;
}
if ((r = malloc(NR * sizeof *q)) == NULL) {
free (p);
free (q);
complain();
return BAD_NEWS;
}

In the (commoner) case where you're making and checking just
one request, I can't see a reason to prefer either form over the
other. (I do, however, prefer an explicit `== NULL' written out,
as I think it reads more naturally. Chacun à son goût, though.)

 

[Willem]

Tom St Denis wrote:
)> I typically check my malloc calls like this:
)>
)> if( !(pointer = malloc(number)) )
)> {
)> ? ? ?fprintf(stderr, "An error!\n");
)> ? ? ?exit(EXIT_FAILURE);
)>
)> }
)>
)> ... or something similar. ?However, I often see people do this:
)>
)> pointer = malloc(number);
)> if( number == NULL )
)
) I'm assuming this was supposed to be pointer == NULL ...
)
)> {
)> ? ? ?fprintf(stderr, "An error!\n");
)> ? ? ?exit(EXIT_FAILURE);
)>
)> }
)>
)> Is there a proper way to check the return value of a malloc call? ?Are
)> there any benefits to doing it one way over another?
)
) Well generally putting assignments inside other statements is
) considered bad form (exceptions being the 'for' keyword).
)
) I'd still check for NULL eitherway though
)
) if ((pointer = malloc(number)) == NULL) {
) ... error
) }
)
) And not use ! on a pointer (though it's probably fine, I don't care to
) check though).

Of course it's fine.

I personally like the more natural-language style:


"allocate 'number' bytes into 'pointer', or print an error and exit."

Or, in C:

(pointer = malloc(number)) || (fprintf(stderr, "An error!\n") && exit(1));


The first style proposed by the OP is, in natural-language:

"if can't allocate 'number' bytes into pointer, print an error, then exit."


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[Malcolm McLean]

Is there a proper way to check the return value of a malloc call?  Are
there any benefits to doing it one way over another?

I use

ptr = malloc(N * sizeof(int) );
if(!ptr)
goto error_exit;

Usually the expression within the malloc call will be of mid-
complexity, and allocating a wrong size is a frequent source of
errors, so it's best to keep it on a line by itself. Almost always if
a call to malloc fails you'll want to fail the function, so jump to
special cleanup code. This allows you to adapt to changed policy
easily (eg exiting on errors rather then returning -1). Also, quite
commonly, you allocate a fairly complex structure with nested arrays.
It's necessary to destroy the whole thing carefully if it has been
incompletely constructed, so the cleanup code needs to be in one
place.

 

[eps]

I use

ptr = malloc(N * sizeof(int) );
if(!ptr)
goto error_exit;

Usually the expression within the malloc call will be of mid-
complexity, and allocating a wrong size is a frequent source of
errors, so it's best to keep it on a line by itself. Almost always if
a call to malloc fails you'll want to fail the function, so jump to
special cleanup code. This allows you to adapt to changed policy
easily (eg exiting on errors rather then returning -1). Also, quite
commonly, you allocate a fairly complex structure with nested arrays.
It's necessary to destroy the whole thing carefully if it has been
incompletely constructed, so the cleanup code needs to be in one
place.

obligatory http://xkcd.com/292/

 

[Ersek, Laszlo]

I personally like the more natural-language style:


"allocate 'number' bytes into 'pointer', or print an error and exit."

Or, in C:

(pointer = malloc(number)) || (fprintf(stderr, "An error!\n") && exit(1));

7.20.4.3 "The exit function", p1: "void exit(int status);"

6.5.13 "Logical AND operator", p2: "Each of the operands shall have scalar
type."

6.2.5 "Types", p18: "Integer and floating types are collectively called
arithmetic types", p21: "Arithmetic types and pointer types are
collectively called scalar types".

(pointer = malloc(number))
|| (fprintf(stderr, "An error!\n"), (exit(1), 0));

Cheers,
lacos

 

[Keith Thompson]

I use

ptr = malloc(N * sizeof(int) );
if(!ptr)
goto error_exit;

Even if ptr is declared as a double*? }

Seriously, the clc-recommended idiom for the malloc call is:
ptr = malloc(N * sizeof *ptr);
or, if you like some extra parentheses;
ptr = malloc(N * sizeof(*ptr));
Do you have some specific reason to repeat the type name?

Particularly since, as you point out:

Usually the expression within the malloc call will be of mid-
complexity, and allocating a wrong size is a frequent source of
errors, so it's best to keep it on a line by itself.

[...]

 

[ImpalerCore]

Even if ptr is declared as a double*?  }

Seriously, the clc-recommended idiom for the malloc call is:
    ptr = malloc(N * sizeof *ptr);
or, if you like some extra parentheses;
    ptr = malloc(N * sizeof(*ptr));
Do you have some specific reason to repeat the type name?

I'm fond of this myself:

#define c_new(type,n) (c_malloc( sizeof(type) * ((size_t)(n)) ))

i.e.

int* i_p = c_new( int, 20 );

The readability quotient for this (at least to me) is a bit higher,
but again I came to C with a C++ history. It does have the drawback
of requiring a manual edit if the type of ptr changes. The drawback
hasn't been significant to me yet, but may be to others.

I am kind of curious what the clc regulars' opinions are about this
syntactic sugar.

Best regards,
John D.

Particularly since, as you point out:

Usually the expression within the malloc call will be of mid-
complexity, and allocating a wrong size is a frequent source of
errors, so it's best to keep it on a line by itself.

[...]

--
Keith Thompson (The_Other_Keith) (e-mail address removed)  <http://www.ghoti.net/~kst>
Nokia
"We must do something.  This is something.  Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"

 

[Ben Pfaff]

I'm fond of this myself:

#define c_new(type,n) (c_malloc( sizeof(type) * ((size_t)(n)) ))

i.e.

int* i_p = c_new( int, 20 );

Do you mean malloc() in place of c_malloc()? If not, what does
c_malloc() do?

This sort of thing would make more sense if the macro used a cast
to convert the return value to the proper pointer type.

 

[ImpalerCore]

Do you mean malloc() in place of c_malloc()?  If not, what does
c_malloc() do?

Sorry, c_malloc is a malloc wrapper that I use that references a
global function table of malloc/calloc/realloc/free function
pointers. So the general case it should be malloc.

This sort of thing would make more sense if the macro used a cast
to convert the return value to the proper pointer type.

That's a valid point (and I do use that construct to wrap element
access in the couple of generic containers I use). So I can infer
that you prefer to manually type sizeof(type) or sizeof(*ptr) in your
malloc calls?

 

[Keith Thompson]

I'm fond of this myself:

#define c_new(type,n) (c_malloc( sizeof(type) * ((size_t)(n)) ))

i.e.

int* i_p = c_new( int, 20 );

Why not this (untested)?

#define c_new(ptr, n) (malloc((n) * sizeof *(ptr)))

int *i_p = c_new(i_p, 20);

But personally I wouldn't bother. I'd find the mental overhead of
remembering just how c_new() works to exceed the overhead of typing

int *i_p = malloc(20 * sizeof *i_p);

You're probably used to it yourself, but anyone else reading your code
will have to figure it out.

 

[ImpalerCore]

Why not this (untested)?

#define c_new(ptr, n) (malloc((n) * sizeof *(ptr)))

int *i_p = c_new(i_p, 20);

But personally I wouldn't bother.  I'd find the mental overhead of
remembering just how c_new() works to exceed the overhead of typing

int *i_p = malloc(20 * sizeof *i_p);

For this particular case, I think the malloc version is clearer,
because it's more idiomatic to C, and I don't think I ever saw the
'sizeof *i_p' paradigm until I visited clc so even making a macro for
it didn't come up. I pretty much used the 'sizeof(type)', but didn't
like manually typing 'sizeof(type)', so I made a kind of C++sh macro
to shorten it.

You're probably used to it yourself, but anyone else reading your code
will have to figure it out.

That's true. If I were to publicly release code, there's a lot more
pull to convert it to the more idiomatic method. I'll keep that in
mind.

 

[Ian Collins]

For this particular case, I think the malloc version is clearer,
because it's more idiomatic to C, and I don't think I ever saw the
'sizeof *i_p' paradigm until I visited clc so even making a macro for
it didn't come up. I pretty much used the 'sizeof(type)', but didn't
like manually typing 'sizeof(type)', so I made a kind of C++sh macro
to shorten it.

"C++sh macro" is an oxymoron!

 

[ImpalerCore]

"C++sh macro" is an oxymoron!

Yeah, there's probably a better expression when trying to use a macro
with a type in C to try to do something that sort of kinda
approximates what templates give you out of the box in C++.

The idea though is something like this.

\code snippet
c_array_t* gc_array_alloc( size_t type_sz, size_t n, c_bool zero );
#define c_array_new( type, n ) (gc_array_alloc( sizeof(type), (n),
FALSE ))

void gc_array_push_back( c_array_t* array, void* object, size_t
type_sz );
#define c_array_push_back( array, object, type )
(gc_array_push_back( (array), (object), sizeof(type) ))

void* gc_array_front( c_array_t* array );
#define c_array_front( array, type ) ( (type*)
(gc_array_front( (array) )) )

#define c_array_i( array, index, type ) ( ((type*) (void*) (array)-

buffer)[(index)] )

int value = 33;
c_array_t* integers = c_array_new( int, 20 );
c_array_push_back( integers, &value, int );
if ( *c_array_front( integers, int ) == 33 ) {
printf( "Booyah\n" );
}
c_array_i( integers, 0, int ) = 100;
\endcode

When working with a generic container, rather than working directly
with void* and casting, packaging the type into the interface imo
makes it much easier on the eyes and the noggin. The style was
inspired from C++, but it's obviously nowhere as good as what
templates can buy. This is where 'C++sh' comes from for lack of a
better phrase.

 

[andy_P]

You'd better check against NULL since C std states that malloc should
return NULL if allocation attempt fails.
NUL is not always defined as 0 (not for all systems)

So, if you want your code to be portable you'd better check against
NULL.

Andy.

 

[Ralf Damaschke]

You'd better check against NULL since C std states that malloc should
return NULL if allocation attempt fails.
Yes.

NUL is not always defined as 0 (not for all systems)

It does not matter how NULL actually is defined.
0 always is a null pointer constant and any pointer value that
compares equal to NULL also compares equal to 0.

See <http://c-faq.com/null/null2.html>
Q: "How do I get a null pointer in my programs?"

-- Ralf

 

[andy_P]

As far as i remember, Opposite to C++, C99 guarantees that (void*)0
will result in null pointer. It does not say that (int)NULL -> 0
(conversion from pointer to integer type). More, it states that it is
implementation-dependent.

Am I wrong?