When to check the return value of malloc

[Joachim Schmitz]

Ulrich Eckhardt wrote:

In that light, I'd even say that xmalloc should start like this:

if(s==0)
return NULL;
void* p=malloc(s);
...

The simple rationale is that dynamic allocations are typically
expensive, so avoiding them is worthwhile.

Well, any decent implementation will do that _first thing inside_ malloc(),
so the overhead would be that of a function call, no big deal.

There is one caveat though: IIRC, quite a few implementations choke on
free(NULL), even more than those that choke on malloc(0).

Neither is allowed to a conforming implementation.

free() has to take anything malloc() might return, that includes NULL and
malloc(0) is explictly allowed, and allthough it does have an implementation
defined behavoir, there are two choices allowed and choking on it is not
amongst them.

Bye, Jojo

 

[Keith Thompson]

Well, I don't usually check the result of a call such as
fprintf(stderr, "Can't frobnicate %s: %s\n", frob, strerror(errno)),
because I don't know what should I do if it failed, but I don't think that
just terminating the program would be a good idea (unless I were going to
terminate it right after the fprintf regardless of its success, that is).

I was referring specifically to malloc() failures.

 

[Keith Thompson]

Typically something between 33% to 50% of code will be there to handle
malloc() failures, in constructor-like C functions. If malloc() fails
they destroy the partially-constructed object and return null to
caller. Typically caller has to simply terminate, or destroy itself
and return null to a higher level, which terminates.

So xmalloc() is a worthwhile simplifier. But it is not a good strategy
for dealing with out of memory conditions. It's simply the best
strategy that I can think of that can be implemented in conforming
C. When the error-handling code is called only once in a trillion
years or so of operation, obviously we are not too bothered about
ideal operation. Howevewr you cannot be expected to do this
calaculation all the time. The heuristic is "is this aloocation likely
to be large enough to have a realistic chance of failure?" If the
answer is yes, call malloc(), check the result, and treat null as one
o fthe normal execution paths of the program. Otherwise call
xmalloc(), and just tolerate the sub-optimal processing.

But that's not what you wrote upthread. What you wrote was:

[...]
The chance of the computer breaking during this period is so so
much higher, there is in this case no point checking the malloc().

You seemd to be talking about calling malloc() and blindly assuming
that it succeeded, not about using xmalloc() to abort the program if
malloc() fails.

Perhaps that's not really what you meant.

 

[Bart C]

There certainly IS a point in checking the return value of malloc()
EVERY SINGLE TIME. If the program fails and you don't have a check
in there, YOU get the blame...

If there are a lot of these malloc()s bunched together, all these checks can
get very untidy and obscure the code. And still leaves the problem of
exactly how to deal with failures, right in the middle of some otherwise
elegant code. Especially annoying when the probability of failure is known
to be low.

I think this is the point Malcolm was making.

But there are any number of strategies for offloading the allocation
checking elsewhere, depending on too many factors to be too specific.

 

[Bill Reid]

Say we've got this, not too contrived situation

Not very contrived at all...

typedef struct
{
int Nemployees;
char **names;
float *salaries;
} Workforce;

when constructing a Workforce, we set up the names array correctly, and
malloc() fails on the employees.

??? how dat goan to happun???

The "constructor" of a "Workforce" starts with a malloc for the
trivial size of an int and two pointers, assigns the number of employees
to "Nemployees", then proceeds to allocate the memory for "names"
and "salaries" based on the sizeof each of those two times "Nemployees".
The "destructor" works in reverse, and is written so it can be called
for any failure to allocate any element of a "Workforce", as well as
just a general deallocation routine:

void free_workforce(Workforce *workforce) {
int num_employees=workforce->Nemployees;

if(workforce!=NULL) {

if(names!=NULL) {

while(num_employees>0) {

num_employees--;
free(workforce->names[num_employees]);
}

workforce->names=NULL;
}

if(workforce->salaries!=NULL) {

free(workforce->salaries);
workforce->salaries=NULL;
}

free(workforce);
workforce=NULL;
}
}

We've now got to be pretty careful. Nemployees is needed to destroy the
names correctly, but salaries is null.

We never allocate all the elements of the structure unless the structure
itself can be allocated in the first place, meaning we ALWAYS have
"Nemployees" available. We SHOULD always be able to get the number
of employees from some source or calculation in the first place, riiiiight?

It can be negotiated successfully, but remember how hard this code is to
test.

Is it? What am I missing here? It's kind of a brain-cramp to write
the "constructors" and "destructors" for this kind of thing, particularly
for large complex structures, verifying it works is a pain in the ass, not
verifying it is a disaster, but once it's done it should work flawlessly in
"production"...at least it seems that way to me, having done something
like this about a zillion times, no leaks or crashes noted...

We've got to test for several patterns of allocations failing and the
recovery code saving the state properly and destroying the object properly.
It is also very easy to forget to initialise the salaries member to ull -
you're intialising it on the malloc, after all. However should a name fail
to allocate, you're left with a garbage pointer.

Ah, yes, note how carefully I set the freed memory pointers to
NULL above, and realize that the "constructor" is really just the
REVERSE of the "destructor", so guess what the first block of
code that allocates the structure does in the "constructor":

unsigned <or Workforce *>
allocate_workforce(Workforce *workforce,int num_employees) {

<well, here is where we malloc workforce>
<guess what we return on failure>

<guess what goes here...maybe, initializations to NULL, perhaps>
<oh, and a good guess would be we assign Nemployees here too>

<do you think we might want to allocate the remaining elements here?>
<guess what we call if we have any failures to allocate workforce
elements>
<and guess what we return on any failure>
}

Like so many things, once you grasp the pattern, you just keep
using the same basic "template" (OMG, more C++ terminology)
over and over and over and over and over and over again...

 

[Ulrich Eckhardt]

Say we've got this, not too contrived situation

typedef struct
{
int Nemployees;
char **names;
float *salaries;
} Workforce;

when constructing a Workforce, we set up the names array correctly, and
malloc() fails on the employees. We've now got to be pretty careful.
Nemployees is needed to destroy the names correctly, but salaries is
null.

Nah, let's refactore the code:

struct workforce {
size_t num_employees;
char** employees;
size_t num_salaries;
float* salaries;
};

Since two containers need allocation, we simply handle them separately. Of
course that is nonsense, real code would rather do it this way:

struct employee {
char* name;
float salary;
};
struct workforce {
size_t num_employees;
struct employee* employees;
};

This has less allocation overhead and handling is simpler because only one
allocation for the workforce overall and one for each employee is made.
Note: alternatives like linked list would change this again.

void
add_worker( workforce_t* wf, char const* name, float salary) {
assert( wf && name);
employee_t tmp = {
xstrdup(name),
salary
};
wf->employees = xrealloc( wf->employees,
wf->num_employees*(sizeof *wf->employees));
wf->employees[wf->num_employees] = tmp;
wf->num_employees++;
}

It can be negotiated successfully, but remember how hard this code is to
test. We've got to test for several patterns of allocations failing and
the recovery code saving the state properly and destroying the object
properly.

I didn't do it in above code, but checking for allocation failure and
returning e.g. ENOMEM instead would not be hard to add. Also, it is almost
trivial to see that it would work correctly. The tricky part is that you
must not forget to free the allocated name in case realloc() fails.

It is also very easy to forget to initialise the salaries member to null
- you're intialising it on the malloc, after all. However should a name
fail to allocate, you're left with a garbage pointer.

Forgetting to initialise things is obviously bad, agreed. However, doing
things in the order I did it above will avoid this problem. Also, if anyone
lateron adds members to struct employee this will keep working as is (well,
it would really be so if it used C99 initialisers) because the remaining
members will be initialised to zero.

Uli

 

[Army1987]

I'd want

int user_input = get_user_input();
/* if I was sensible, sanity check here, but this is a sloppy program */
int s = user_input * sizeof(record_t);
/* at this point we've got an overflow, so compiler issues "arithmetic
overflow" and terminates. */

Unfortunately we don't get this yet. Almost no compilers will handle oveflow
nicely. However if the result is negative, which is a sporting chance, at
least we can pick it up in the call to xmalloc(). If we are in a loop
with essentially random values being typed in by user, the chance of a
negative becomes statistically certain.

OMG...
What's wrong with
if(INT_MAX / sizeof(record_t) >= user_input) {
/*do something*/
}
? Essentially you mean that the program should terminate by chance. Btw,
there is no overflow. The expression `user_input * sizeof(record_t)` has
type size_t (unless SIZE_MAX < INT_MAX...), it is reduced modulo
SIZE_MAX + 1, and it is converted to an int, and if it doesn't fit there
the result is implementation defined. And what happens if the user knows
how that works and enters numbers so that it will never cause xmalloc to
be called with a negative argument?

 

[Army1987]

Please cite the research that measured this surprising figure.

Probably the same one which says that more than 50% of integers are used
to index arrays?

 

[CBFalconer]

But the original example did check for zero. I was merely
proposing an alternative to blindly changing the requested size
to one. A user provided handler can take into account the
implementation's behaviour, if required.

The point is that the standard allows for alternative actions by
malloc. By avoiding calling malloc with a 0 argument, we avoid the
alternative results. Now user code can, if desired, abort programs
on malloc failure, or take extensive 'fixitup' attempts.

I suspect the provision is there only to allow for existing
systems. My own versions of malloc always return a non-null result
for a successful call, so the problem doesn't arise.

 

[CBFalconer]

CBFalconer wrote

Yeah, but with a name like xmalloc(), you don't expect munging
of the behavior. If you called it something like
mymallocthatfixessillyrequests() maybe. I'd much rather have
it do what I ask of it, than that, and certainly not do so
quietly.

What behaviour is munged? The caller receives a pointer back. He
is not allowed to store any data there. Oh, it won't fail for 1
byte. So what. Do all systems fail for one byte overrun? The
caller never gets the modified s back. The behaviour exactly fits
the standard specification for malloc. However, you now know what
it does when called with a zero argument.

 

[Malcolm McLean]

OMG...
What's wrong with
if(INT_MAX / sizeof(record_t) >= user_input) {
/*do something*/
}
?

You've missed the comment. "If I was sensible I'd sanity check here". That
is to say, at the point of input.
The objection to xmalloc() is that since it takes an int rather than a
size_t, it might break in nasty way on large allocations. Which is true.
However it isn't intended for large allocations which have a realistic
chance of failing. It is meant for situations where the chance of failure is
too low for it to be worth writing custom error-handling code.

 

[CBFalconer]

.... snip ...

Don't bother checking the return value of malloc(0) (where the
zero value is a constant). The easist way to deal with this is
to never call malloc(0) in the first place.

It is permitted for malloc(0) to return a pointer that may not be
dereferenced. On some (probably most) malloc systems this will eat
up some memory to keep track of the size etc. of the data
allocated. This may fail, resulting in a NULL return. So you
should ALWAYS check the result of any malloc call.

 

[Malcolm McLean]

But that's not what you wrote upthread. What you wrote was:

[...]
The chance of the computer breaking during this period is so so
much higher, there is in this case no point checking the malloc().

You seemd to be talking about calling malloc() and blindly assuming
that it succeeded, not about using xmalloc() to abort the program if
malloc() fails.

Perhaps that's not really what you meant.

There's a strong case for making programs correct. There is also a strong
case for taking code that will almost certainly never be executed out of
functions.
xmalloc() is an attempt to solve both issues.
However the literal answer to the OP's question is "when the chance of a
malloc() failure is higher than the chance of the computer breaking down, it
is worthwhile checking the return value of malloc()".
We can calculate it - it took only a few minutes with calculator to work out
that Flash's example - computer running out of memory every month, 2GB of
memory, a non-loop allocation of 20 bytes means that we are talking of about
one failure of that allocation in every ten million years of operation. I
can cope with one irate phone call every ten million years. But it isn't
always so easy to do the calculation.

 

[Malcolm McLean]

Not very contrived at all...


??? how dat goan to happun???

I nodded. I meant "salaries". Which somewhat reduces the force of what I am
saying.

Is it? What am I missing here? It's kind of a brain-cramp to write
the "constructors" and "destructors" for this kind of thing, particularly
for large complex structures, verifying it works is a pain in the ass, not
verifying it is a disaster, but once it's done it should work flawlessly
in
"production"...at least it seems that way to me, having done something
like this about a zillion times, no leaks or crashes noted...

You've forgotten half of the challenge. To save the state, then destroy the
object.
That's why the example was slightly contrived to hold salaries in a parallel
array.
You need to save the names. They are precious data. However you don't have
any salaries. So you've got to devise some code for "salaries missing", and
dump that to the state file. Then you've got to test that the save and
recovery code works, for all possible combinations of missing names, missing
salaries, missing entire structures.
The fact that you didn't spot this, despite being an experienced programmer,
was what I expected. It was not a trick question, but it was a tricky
question. I spoilt it a bit by the typing error. It is an utter nightmare
trying to save and recover partially-constructed objects to disk.

 

[Bart C]

However the literal answer to the OP's question is "when the chance of a
malloc() failure is higher than the chance of the computer breaking down,
it is worthwhile checking the return value of malloc()".
We can calculate it - it took only a few minutes with calculator to work
out that Flash's example - computer running out of memory every month, 2GB
of memory, a non-loop allocation of 20 bytes means that we are talking of
about one failure of that allocation in every ten million years of
operation. I can cope with one irate phone call every ten million years.
But it isn't always so easy to do the calculation.

I haven't tried to repeat your calculation but it somehow feels wrong.

Failing on one specific 20-byte allocation is, yes, unlikely (unlikely
enough for an appropriate wrapper, if you feel confident enough to use it,
to take the drastic step of aborting), but once in 10,000,000 years?

According to Flash it's more like once a month? And when you have many such
allocations? And when there may be thousands of copies of your software
being run daily on all sorts of machines with all sorts of unknown and
possibly dodgy software running at the same time?

You might get that phone call sooner than you think.

 

[Flash Gordon]

Malcolm McLean wrote, On 20/01/08 17:58:

But that's not what you wrote upthread. What you wrote was:

[...]
The chance of the computer breaking during this period is so so
much higher, there is in this case no point checking the malloc().

You seemd to be talking about calling malloc() and blindly assuming
that it succeeded, not about using xmalloc() to abort the program if
malloc() fails.

Perhaps that's not really what you meant.

There's a strong case for making programs correct. There is also a
strong case for taking code that will almost certainly never be executed
out of functions.

Incorrect, because frequently when people believe an error condition is
unlikely to happen the real world comes along and proves them wrong.

xmalloc() is an attempt to solve both issues.

One with big holes as pointed out else where in this thread.

A decent malloc wrapper, on the other hand, can be an appropriate method
of handling malloc failure.

However the literal answer to the OP's question is "when the chance of a
malloc() failure is higher than the chance of the computer breaking
down, it is worthwhile checking the return value of malloc()".

Which works out as it is always worth checking the return value of
malloc unless you make stupid assumptions.

We can calculate it - it took only a few minutes with calculator to work
out that Flash's example - computer running out of memory every month,
2GB of memory, a non-loop allocation of 20 bytes means that we are
talking of about one failure of that allocation in every ten million
years of operation. I can cope with one irate phone call every ten
million years. But it isn't always so easy to do the calculation.

No, a computer running out of memory every month means several malloc
failures every month for just one person. Unless you are assuming that
your SW is hardly ever used.

Personally I don't generally bother writing software unless it will be
used a lot.

 

[Ben Bacarisse]

It is permitted for malloc(0) to return a pointer that may not be
dereferenced. On some (probably most) malloc systems this will eat
up some memory to keep track of the size etc. of the data
allocated. This may fail, resulting in a NULL return. So you
should ALWAYS check the result of any malloc call.

I disagree. malloc(0) is the one case where one need not check the
return and checking it may even be an error! Of course, since normal
code checks the return from malloc we sometimes have to turn the check
off. Take an algorithm that works on an array and needs a work array:

double algorithm(double a, size_t n)
{
double *work_space = malloc(n * sizeof *work);
if (work_space == NULL) {
/* complain, exit, whatever... */
}
/* do stuff */
free(work_space);
return ...;
}

and I have coded it up to work even when n == 0 (it will thus not
touch any a[x] nor use work_space[x]) it will still go wrong when
called with n == 0 on some systems. I need:

if (work_space == NULL && n != 0) {
/* complain, exit, whatever... */
}

(or similar) to make it work portably. xmalloc is special case of
this and hence, Eric Sosman's suggestion. Your alternative also works
but is more wasteful.

Testing the return of malloc(0) is a bad idea because it is not
well-defined.

 

[Ben Pfaff]

But the original example did check for zero. I was merely proposing an
alternative to blindly changing the requested size to one. A user
provided handler can take into account the implementation's behaviour,
if required.

It's just as easy to simply pass through the implementation's
behavior, e.g.

void* xmalloc(size_t s) {
void* res = malloc(s);
if(!res && s) {
fprintf( stderr, "malloc() failed\n");
exit(EXIT_FAILURE);
}
return res;
}

This is the way that I have implemented xmalloc() in some of my
own programs.

 

[Gordon Burditt]

Don't bother checking the return value of malloc(0) (where the

It is permitted for malloc(0) to return a pointer that may not be
dereferenced. On some (probably most) malloc systems this will eat
up some memory to keep track of the size etc. of the data
allocated. This may fail, resulting in a NULL return. So you
should ALWAYS check the result of any malloc call.

What's the difference between malloc(0) returning a pointer that
cannot be dereferenced (non-null) and returning a pointer that
cannot be dereferenced (null)?

Why would you ever want to call malloc(0) (argument is a constant
0, not a variable that happens to have the value zero) in the first
place? You cannot portably count on it being any different from
((void *)0), and ((void*)0) is probably faster.

 

[Herbert Rosenau]

Or you know that your system overcommits virtual memory, so that
malloc keeps ``working'' even though your total allocations have
already exceeded core + swap.

And then the system is set up for noswap and nocommit - and BANG!

There is nothing that forbids overcommit - but there is even nothing
that requires malloc to fail when a commit fails miserably.

malloc() is allowed to return NULL when it can not deliver memory for
a single object you needs - so check the return of c/m/realloc every
time to be sure to get memory you need and have a strategy what to do
when re/m/c/alloc tells you that it can't fullify your request.

How will malloc serve you with a memory block when overcommetted
memory gets so overcommited that still there is really no address room
left to overcommit?

--
Tschau/Bye
Herbert

Visit http://www.ecomstation.de the home of german eComStation
eComStation 1.2R Deutsch ist da!