When to check the return value of malloc

[Rob Hiddink]

Howdy,

I was reflecting recently on malloc.

Obviously, for tiny allocations like 20 bytes to strcpy a filename or
something, there's no point putting in a check on the return value of
malloc.

OTOH, if you're allocating a gigabyte for a large array, this might
fail, so you should definitely check for a NULL return.

So somewhere in between these extremes, there must be a point where you
stop ignoring malloc's return value, and start checking it.

Where do people draw this line? I guess it depends on the likely system
the program will be deployed on, but are there any good rule-of-thumbs?

When NOT to check for NULL

1. You hate your current job
2. You just heard somebody close to you died
3. Your girl or boyfriend just broke up with you
4. You're drunk or otherwise intoxicated
5. You are currently getting a blowjob
6. Its almost time to go home and you dont want to miss the next episode of
As the world turns

 

[Randy Howard]

I don't think that's true. The system fails to deliver 20 bytes. A window
pops up on screen saying "program Fred needs more memory to continue, please
shut down something else". User shuts down his Internet Explorer, which is
in the background doing nothing much, and program Fred continues.

If you can't malloc 20 bytes, odds are the message may never even
appear.

I know that modern OSes tend to warn the user long before this happens, and
Linux has a dangerous policy of overcomitment. But the basic idea is
reasonable.

That policy is configurable.

 

[Richard]

That is more easily handled by:

void *xmalloc(size_t s) {
void *res;

if (!s) s++;
Horrible.

if (!(res = malloc(s))) {
fputs("malloc() failed\n", stderr);
exit(EXIT_FAILURE); /* optional */
}
return res;
}

which can leave the exit decision to the caller. It also makes the
return of NULL a positive indication of failure.

No it doesn't.

 

[Ian Collins]

That is more easily handled by:

void *xmalloc(size_t s) {
void *res;

if (!s) s++;

Why?

 

[Randy Howard]

That is more easily handled by:

void *xmalloc(size_t s) {
void *res;

if (!s) s++;
What?

if (!(res = malloc(s))) {
fputs("malloc() failed\n", stderr);
exit(EXIT_FAILURE); /* optional */
}
return res;
}

which can leave the exit decision to the caller. It also makes the
return of NULL a positive indication of failure.

 

[CBFalconer]

.... snip ...

If you've got better semantics for xmalloc, then by all means
share them. I don't think it's an ideal solution at all. It's the
least bad solution to the problem of trivial requests failing
I've been able to devise so far.

What about the accurate:

if (!(p = malloc(szneeded * sizof *p)))
callAppropriateRecovery(szneeded, &p);
.... /* all well if recovery works */

 

[CBFalconer]

Just check my website. Virtually all the files are written in
"object style". That is to say, there is a function with the
same name as the public structure, though in lower case, and a
corresponding kill function. There are also opaque method
functions which manipulate the structures.

There are other possible conclusions. One is the horrible
inefficiency of "object style". Another is the possible flaws in
your coding style.

 

[Richard Heathfield]

Ian Collins said:

Why?

I'll take a stab at that. But before I do, I'll just say that I'd prefer to
express it as:

if(s == 0)
{
s = 1;
}

but then I always did call a spade a manually operated agricultural
excavation implement.

Now, I *think* the problem Chuck is trying to address is this: if you
allocate a zero amount of memory (and *why*, precisely, would anyone want
to do this?), the Standard has this to say: "If the size of the space
requested is zero, the behavior is implementation-defined; the value
returned shall be either a null pointer or a unique pointer."

So either you get NULL or you get a pointer you can't use, and you don't
know which will happen unless you consult the implementation's
documentation (which might not even be available - one of the benefits of
sticking to portable C is that we can write code and send it off to some
other party, who may well be using an implementation we've never heard of,
let alone have documentation for). I can easily see why it might be
convenient to "normalise" the behaviour by changing the allocation request
to one where malloc must return a valid and usable pointer if it succeeds.

It gets worse. On at least one implementation that I've used myself,
malloc(0) crashes (or "abends", since it was a mainframe implementation)
the process. That isn't one of the behaviours that the Standard allows,
but this *is* the real world, right? We can't allow for every possible bug
in every possible implementation, but we can at least take care of some of
the ones we *do* know about. So what is the Right Thing here? We could
simply reject the request completely, or we could allocate space for one
byte rather than zero bytes, simply to avoid a crash. Either strategy is
defensible. Personally, I think I'd prefer to reject the request, but
allocating one byte instead is a perfectly sensible workaround.

 

[Ian Collins]

Ian Collins said:


I'll take a stab at that. But before I do, I'll just say that I'd prefer to
express it as:

if(s == 0)
{
s = 1;
}

but then I always did call a spade a manually operated agricultural
excavation implement.

Now, I *think* the problem Chuck is trying to address is this: if you
allocate a zero amount of memory (and *why*, precisely, would anyone want
to do this?), the Standard has this to say: "If the size of the space
requested is zero, the behavior is implementation-defined; the value
returned shall be either a null pointer or a unique pointer."

I agree that's the problem here, but I don't agree with the solution.

My preference would be to make the action taken on a zero byte request a
policy decided by the user, something like

if(s == 0)
{
return zeroByteMallocHandler();
}

Which forces the user to make a call on what to do under these
circumstances. In the general case, requesting zero bytes is probably
an error and zeroByteMallocHandler() would issue a diagnostic and call
exit(). Changing s to 1 will mask this error and may cause weird
problems later when the pointer is used.

 

[CBFalconer]

Why?

That converts a request for 0 bytes to a request for 1 byte, and
avoids the return of NULL when malloc has not failed (an option,
according to the std).

 

[CBFalconer]

CBFalconer wrote


What?

Leaving s in the range 1 .. SIZE_MAX.

 

[Harald van Dijk]

That converts a request for 0 bytes to a request for 1 byte, and avoids
the return of NULL when malloc has not failed (an option, according to
the std).

Another interpretation is that the standard does not allow a null pointer
to be returned except as a failure indicator, but that it allows malloc
to unconditionally fail if the requested size is zero. The end result is
the same for malloc (which is probably why the standard doesn't specify
which is correct), but this interpretation is consistent with realloc,
where a null pointer indicates failure and does not cause the old object
to be freed, even if the new size is zero.

 

[Gordon Burditt]

I was reflecting recently on malloc.

Obviously, for tiny allocations like 20 bytes to strcpy a filename or
something, there's no point putting in a check on the return value of
malloc.

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 the program fails and you DO have
a check in there that prints an appropriate message, you can blame
someone else (like the guy that spec'd the system or someone running
a memory hog on it). Your job may depend on this.

OTOH, if you're allocating a gigabyte for a large array, this might
fail, so you should definitely check for a NULL return.

If you're allocating a gigabyte in 20 byte pieces, it might fail
at some point.

So somewhere in between these extremes, there must be a point where you
stop ignoring malloc's return value, and start checking it.

Where do people draw this line? I guess it depends on the likely system
the program will be deployed on, but are there any good rule-of-thumbs?

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.

Anything more than that, you should check.

 

[Ian Collins]

That converts a request for 0 bytes to a request for 1 byte, and
avoids the return of NULL when malloc has not failed (an option,
according to the std).

I still think it best to force fail or make the user provide a handler
for zero byte requests on the grounds they are probably an error.

One could argue that by invoking implementation defined behaviour from
malloc, it is always an error (or at least not portable).

 

[Randy Howard]

Leaving s in the range 1 .. SIZE_MAX.

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.

 

[Ben Pfaff]

I still think it best to force fail or make the user provide a handler
for zero byte requests on the grounds they are probably an error.

I disagree. In my own experience, some algorithms on arrays
gracefully degrade to the case of 0-length arrays. For
implementing such algorithms it is valuable to not have to insert
otherwise gratuitous checks for 0.

 

[Ben Pfaff]

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.

It still does what you ask of it, since the successful return
value of malloc(0) depends on the implementation: it may be a
null pointer or a unique pointer value that may not be
dereferenced. There seems to be a large contingent in the C
world in favor of the latter behavior, and this is not the first
xmalloc implementation that I have seen enforcing it.

 

[Randy Howard]

It still does what you ask of it, since the successful return
value of malloc(0) depends on the implementation: it may be a
null pointer or a unique pointer value that may not be
dereferenced. There seems to be a large contingent in the C
world in favor of the latter behavior, and this is not the first
xmalloc implementation that I have seen enforcing it.

What I was aiming at, but I didn't do very well, was I'd probably
rather have it spit out a log file entry or perhaps something on screen
if I called with size of 0 than quietly do something different, because
most of the time, I'd want to know that this was happening, not have it
quietly done without realizing it.

This seems like it might have some short term benefits, but make
debugging something really flaky actually harder in the long run. To
each his own.

 

[Ian Collins]

I disagree. In my own experience, some algorithms on arrays
gracefully degrade to the case of 0-length arrays. For
implementing such algorithms it is valuable to not have to insert
otherwise gratuitous checks for 0.

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.

 

[Ulrich Eckhardt]

Why?

I'd offer another alternative that was pointed out to me in this thread:

p = malloc(s);
if(!p && s!=0)
failure().

The whole point of xmalloc() is that it either succeeds or aborts execution.
Now, since malloc(0) may or may not return NULL, that alone can't be taken
as indicator for failure in that corner case.

However, I think the agreed-on semantics of xmalloc are that you can
access 's' bytes starting at the returned pointer. Now, if 's' is zero, the
actual value that is returned doesn't matter[1], because effectively you
can only access zero bytes starting at that address. Using this allows
pretty uniform code for allocating arrays, no matter what their size is.
Since xmalloc checks against allocation failure and a zero size implicitly
guards you (provided proper use) against accessing this empty array, it
doesn't matter if you return NULL or some dummy value or change the
allocation size to one.

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.


There is one caveat though: IIRC, quite a few implementations choke on
free(NULL), even more than those that choke on malloc(0). This might have
to be taken into account when porting to other platforms. It might be
easiest to change xmalloc() to always return at least one byte there.
Otherwise, you either have to check before calling free() or provide an
additional xfree() which behaves correctly.

The only case I can imagine this to fail is this kind of code:

// old version
p = malloc(s);
if(!p)
return ENOMEM;
// new version
p = xmalloc(s);
assert(p);// make sure xmalloc works

Both versions are in fact broken and their misbehaviour is not an indicator
of a broken xmalloc() implementation. The problem is that they both don't
take the weird malloc(0) behaviour into account. The correct code is this:

// old version
p = malloc(s);
if(!p && s!=0)
return ENOMEM;
// new version
p = xmalloc(s);
assert(p || s==0);// make sure xmalloc works

In my eyes, the assert() should be removed altogether, it only serves to
further clutter the code. Rather, I'd move the assert() into the
implementation of xmalloc().

Uli

[1] I'm aware of the fact that doing anything with uninitialised (random)
pointers is undefined.