When to check the return value of malloc

[BruceS]

[...]

I agree about fclose().  I would be very unlikely to use that feature,
but to my way of thinking it would still be a better interface if it
worked that way.  Then again, I'd also prefer an fclose() that NULLed
the FILE * [i.e. fclose(&myfileptr); ], and a free that took a pointer
to the pointer and set it NULL after releasing the memory.  Then you'd
only have to be careful of secondary pointers.  I could just do

myptr = realloc(myptr, 0);

instead of

free(myptr);
myptr = NULL;

but that seems a bit contrived and "cute", and more likely to confuse
than to communicate.

There are two problems with having free() take a pointer-to-pointer
argument.  One is that, though void* is a generic pointer type, there is
no generic pointer-to-pointer type.  You can't write a myfree() function
for which both of these calls:

    int    *iptr = malloc(sizeof *iptr);
    double *dptr = malloc(sizeof *dptr);
    myfree(&iptr);
    myfree(&dptr);

are legal.

See, that's what I get for not attempting to implement my improved
free()---I would have run into that.

Second, it doesn't solve the whole problem:

    int *p = malloc(sizeof *p);
    int *copy_of_p = p;
    free(p);
    p = NULL;
    /* copy_of_p is still a dangling pointer */

That's what I meant by secondary pointers. Any time you have >1
pointer to the same piece of memory, you have extra work to manage
them. I can't offhand think of a reasonable way to get around this.

Perhaps a partial solution is better than none at all -- but see the
first point.

One solid hit does the trick. The second just wings, but it looks
like the target is down anyway.

 

[BruceS]

BruceS wrote:

) I agree about fclose().  I would be very unlikely to use that feature,
) but to my way of thinking it would still be a better interface if it
) worked that way.  Then again, I'd also prefer an fclose() that NULLed
) the FILE * [i.e. fclose(&myfileptr); ], and a free that took a pointer
) to the pointer and set it NULL after releasing the memory.  Then you'd
) only have to be careful of secondary pointers.  I could just do
)
) myptr = realloc(myptr, 0);
)
) instead of
)
) free(myptr);
) myptr = NULL;
)
) but that seems a bit contrived and "cute", and more likely to confuse
) than to communicate.

How about making free() always return NULL ?

Then you can simply do:  myptr = free(myptr);
Which is nicely parallel to the realloc() version.

I like that. The same could be done with fclose(), except that it
loses the ability to indicate an error by returning EOF. Of course,
this can be done with a simple macro without changing the language.

 

[Moi]

Okay, assignment isn't the proper word. A value is still being returned
here and it is still being used; it just isn't being assigned to a
varable.

My question would be why p hasn't already been checked against a NULL
condition? One would that that this would be done when it was first
allocated; before one would assume any of its contents would have any
sanity. Notice that this isn't all that descriptive either. Your
testing two non-dependant condtions at the same time. This alone raises
a flag in my mind.

Well, it is a matter of taste. Some people sometimes prefer grouping
*all the possibilities* into one big switch. It avoids extra nesting
(and "ugly tails"), such as:

if (p) {
switch (p->status) {
...
...
... 30 lines of blabla...
...
...
}
}else {

... oops there was no p ...
}

That's all,
AvK

 

[Ben Pfaff]

Sorry, poor wording. I don't always speak the best English. What I was
referring to is the fact that ?: implies an assignment (ie, the first
argument).

Here's a counterexample from some code of mine:

if (row->old
? !ovsdb_datum_equals(&row->old[idx], &row->new[idx], &column->type)
: !ovsdb_datum_is_default(&row->new[idx], &column->type)) {
...
}

 

[Keith Thompson]

Okay, assignment isn't the proper word. A value is still being returned
here and it is still being used; it just isn't being assigned to a varable.

Then I'm not sure what you're trying to say.

My question would be why p hasn't already been checked against a NULL
condition? One would that that this would be done when it was first
allocated; before one would assume any of its contents would have any
sanity. Notice that this isn't all that descriptive either. Your testing
two non-dependant condtions at the same time. This alone raises a flag in
my mind.

Certainly you'd check whether p == NULL immediately after allocating it
(assuming it points to allocated memory; it could point to anything).
But a null pointer might still be a valid state for p.

In this case, p appears to be a pointer to a structure that has
information about a printer. If you don't have a printer (or
haven't selected one, or whatever), then p will be null; it's not
necessarily an error. The "case -1:" code might ask the user
to configure a printer, for example.

 

[Moi]

Certainly you'd check whether p == NULL immediately after allocating it
(assuming it points to allocated memory; it could point to anything).
But a null pointer might still be a valid state for p.

In this case, p appears to be a pointer to a structure that has
information about a printer. If you don't have a printer (or haven't
selected one, or whatever), then p will be null; it's not necessarily an
error. The "case -1:" code might ask the user to configure a printer,
for example.

again:
switch (p? p->status : -1) {
case -1:
p = configure_printer(...);
if (p) goto again;
break;
case STATUS_OUT_OF_PAPER:
...
break;
case FILE_NOT_FOUND:
...
break;
case WATER_IN_DRIVE_A:
start_pumping();
break;
...
}


/cross-topic

FTFY ;-)
AvK

 

[Ben Bacarisse]

This is where I'd do it quite differently, more like:

char *p = malloc(...);
if ( !p )
/* error */
else
{
int *q = malloc(...);
if ( !q )
/* error */
else
{
mystruct *r = malloc(...);
if ( !r )
/* error */
else
{
char *s = malloc(...);
if ( !s )
/* error */
else
{
/* ok */
free(s);
} //allocated s
free(r);
} //allocated r
free(q);
} //allocated q
free(p);
} //allocated p

Well, each to their own, but this is much too much for my taste. It can
be tricky making the "no error" path clear and obvious in a language
like C with in-band error signalling, but it is particularly buried in
your version. Also, the error code needs to be written four times.

<snip>

 

[Tim Harig]

Sorry, poor wording. I don't always speak the best English. What I was
referring to is the fact that ?: implies an assignment (ie, the first
argument).

Here's a counterexample from some code of mine:

if (row->old
? !ovsdb_datum_equals(&row->old[idx], &row->new[idx], &column->type)
: !ovsdb_datum_is_default(&row->new[idx], &column->type)) {
...
}

As I have already mentioned to Moi, in an almost identical example, while
yes you are not actually assigning the value returned by the ?: statement,
you are still using it. You don't often see it used in isolation without
making use of its value, ie:

(row->old
? !ovsdb_datum_equals(&row->old[idx], &row->new[idx], &column->type)
: !ovsdb_datum_is_default(&row->new[idx], &column->type)

Since the it tends to imply a this or that quantity rather then a general
if/else, you would tend to wonder why the value is not being used. In that
sense, using it in this way is somewhat misleading.

 

[Tim Harig]

Then I'm not sure what you're trying to say.

This really is a small point among the rest that I have made.

What I mean is that you do not normally see ?: operators where no value
from the statements are used:

(a?b:c)

As the the ?: construct was designed as an explicit this or that value
construct, you don't see it used in isolation as above. If you did, you
would wonder why the value it returns is not being used. It is in this way
misleading. If/else doesn't necessarily have this limitation:

if (condition)
/* do something */
else
/* do something else */

It doesn't stand to reason that any value changes are actually preserved
(though they might) with the if/else blocks.

Certainly you'd check whether p == NULL immediately after allocating it
(assuming it points to allocated memory; it could point to anything).
But a null pointer might still be a valid state for p.

So you are essentially overloading p. It points to a print_dev struct if
one is available or NULL if it is not.

In this case, p appears to be a pointer to a structure that has
information about a printer. If you don't have a printer (or
haven't selected one, or whatever), then p will be null; it's not
necessarily an error. The "case -1:" code might ask the user
to configure a printer, for example.

Okay, I can see now where Moi is coming from; but, in my opinion, if
there are no printers then *no* printer handling code should be called in
the first place. As Moi has it, the printing codes all must be able to
handle the possibility that the printer may not exist. This is a
requirement for *everything* that attempts to dereference p. Everybody who
works with the code must be aware of this fact.

I would much rather handle a missing printer as soon as the printer is
allocated or selected. NULL might be okay to pass back as a flag from the
create_printer() function; but, once it has failed I want a more explicit
indication of it If a valid printer is saved to p then a printer_count
variable is incremented or possibly the printer is placed into some
kind of collection where the length can be counted. I would prevent
any printer handling code from running at all if p is not a valid
printer_dev struct.

Even using NULL as a flag for a valid printer , I feel that checking
whether or not the printer exists is logically distinct from what
errors a valid printer may have. In both cases, I would have formated
it as two blocks. One check that a valid printer is present, either by
checking the printer count or in Moi's case whether p was NULL, and the
other to check for the status of a valid printer struct. This seems more
explicit to me.

 

[Tim Harig]

Alright, Keith Thompson let me in on a little bit of your thinking. You
are effectively overloading p to act both as a valid struct or as NULL if
was not valid. Returning NULL from the function that creates the struct is
sufficient; but, after that I would want a more explicit indication that
creating the stuct failed. I would not allow any printer handling code,
including your switch/case statement execute unless as valid struct was
created.

Well, it is a matter of taste. Some people sometimes prefer grouping
*all the possibilities* into one big switch. It avoids extra nesting

Separating the blocks into distinct functional units does have the
potential to create an extra nest; but, in practice I would expect the code
that creates your struct object to be in a distinct function from your
switch/case statement that is checking for its status.

(and "ugly tails"), such as:

I would not have permitted such an "ugly tail." In my code, none of your
switch case statement would be allowed to execute unless a valid p
structure had been created. If and only if struct p had been created would
I worry about checking the status of p. In any case, testing whether p is
valid and the status of a valid p are, to me, two functionally distinct
operations and I would have placed them in separate blocks.

 

[Seebs]

What I mean is that you do not normally see ?: operators where no value
from the statements are used:

"expressions", not "statements", but I think this is probably true.
You similarly don't tend to see many addition operators where their value
isn't used. In general, operators are used only when we want their
values, except when they have side effects.

So you are essentially overloading p. It points to a print_dev struct if
one is available or NULL if it is not.

That's not really "overloading".

Okay, I can see now where Moi is coming from; but, in my opinion, if
there are no printers then *no* printer handling code should be called in
the first place.

That seems much, much, harder to arrange.

As Moi has it, the printing codes all must be able to
handle the possibility that the printer may not exist. This is a
requirement for *everything* that attempts to dereference p. Everybody who
works with the code must be aware of this fact.

No, it's only a requirement for all the places at which you could enter
the code. If the only interface provided is
do_print(printer *p, struct print_me *data);
then only do_print has to handle this.

I would much rather handle a missing printer as soon as the printer is
allocated or selected.

Yes, but the way you handle it is by storing a null pointer to indicate
that there isn't anything to point to.

NULL might be okay to pass back as a flag from the
create_printer() function; but, once it has failed I want a more explicit
indication of it If a valid printer is saved to p then a printer_count
variable is incremented or possibly the printer is placed into some
kind of collection where the length can be counted. I would prevent
any printer handling code from running at all if p is not a valid
printer_dev struct.

That seems a LOT more difficult. You then have to have every single place
that might want to ask that someone do something involving a printer check
that, which is more work.

Which is easier?

Case #1:
int do_print(printer *p, struct print_me *data) {
if (!p)
return -1;
/* code using p */
}

...
do_print(default_printer, file_data);
...
do_print(alternate_printer, file_data);
...
do_print(color_printer, stored_entries);

Case #2:
int do_print(printer *p, struct print_me *data) {
/* code using p */
}
...
if (default_printer)
do_print(default_printer, file_data);
...
if (alternate_printer)
do_print(alternate_printer, file_data);
...
if (color_printer)
do_print(color_printer, stored_entries);

I think the former makes more sense, and is also more robust.

Even using NULL as a flag for a valid printer , I feel that checking
whether or not the printer exists is logically distinct from what
errors a valid printer may have.

That it might be, but it is easy to imagine a case where the
simplest thing to do is to merge the cases since "no printer exists"
is clearly very much like errors a printer could have.

In both cases, I would have formated
it as two blocks. One check that a valid printer is present, either by
checking the printer count or in Moi's case whether p was NULL, and the
other to check for the status of a valid printer struct. This seems more
explicit to me.

It might be, but if the handling for error conditions is reasonably elaborate,
being able to route both cases through a single hunk of code may be easier
to maintain. Minimize special cases early and often!

-s

 

[Nick Keighley]

Sorry, poor wording.  I don't always speak the best English.  What I was
referring to is the fact that ?: implies an assignment (ie, the first
argument).

no it doesn't. Maybe you'd drop your predjudice about ?: if you worked
out what it did... In what sense is ?: different from any other
operator?

<snip>

 

[Nick Keighley]

Here's a counterexample from some code of mine:

      if (row->old
          ? !ovsdb_datum_equals(&row->old[idx], &row->new[idx], &column->type)
          : !ovsdb_datum_is_default(&row->new[idx], &column->type)) {
          ...
      }

As I have already mentioned to Moi, in an almost identical example, while
yes you are not actually assigning the value returned by the ?: statement,
you are still using it.  You don't often see it used in isolation without
making use of its value, ie:

you don't often see + used in isolation without making use of its
value

        (row->old
           ? !ovsdb_datum_equals(&row->old[idx], &row->new[idx], &column->type)
           : !ovsdb_datum_is_default(&row->new[idx], &column->type)

Since the it tends to imply a this or that quantity rather then a general
if/else, you would tend to wonder why the value is not being used.  In that
sense, using it in this way is somewhat misleading.

not really. In fact not at all

 

[Phil Carmody]

Sorry, poor wording. I don't always speak the best English. What I was
referring to is the fact that ?: implies an assignment (ie, the first
argument).

I have written many a printk that contains conditional terms. But
it works well for arbitrary function's parameters.
prepare_mem(p->buf, p->is_big ? BIG : SMALL);
So again, I dispute your so-called facts, no matter how they are
worded.

I wish I was young

I wish I hadn't found out yesterday that one of my managers has only
half the work experience that I have. And he's in a role where he is
supposed to make decisions about what I work on.

Your above assertion does support the hypothesis that you've simply
not seen enough code. So if you're trying to swing things back in
favour of just being naive, you're making progress.

Would a crack pipe help diminish my growing gut? Where can I get one!

I can't say I've never known any crack users, but from what I've seen
in the trashy media, weight loss may occur. Beer, on the other hand,
can be healthy (so you want it live and not too heavily filtered), so
will only give gut-friendly, but of course gut-unfriendly, advice.

Phil

 

[Keith Thompson]

I have written many a printk that contains conditional terms. But
it works well for arbitrary function's parameters.
prepare_mem(p->buf, p->is_big ? BIG : SMALL);
So again, I dispute your so-called facts, no matter how they are
worded.

What Tim meant, as he clarified in a later followup, is that the
use of ?: implies, or should imply, that the resulting value is
going to be used, i.e., not discarded. I tend to agree; if you're
going to discard the result, you might as well use an if statement.
Obviously "assignment" wasn't the righ word for this, but one can
very loosely think of the result of an expression being "assigned"
to the enclosing context.

 

[Nisse Engström]

Sorry, poor wording. I don't always speak the best English. What I was
referring to is the fact that ?: implies an assignment (ie, the first
argument).

Here's a counterexample from some code of mine:

if (row->old
? !ovsdb_datum_equals(&row->old[idx], &row->new[idx], &column->type)
: !ovsdb_datum_is_default(&row->new[idx], &column->type)) {
...
}

Interesting. I both like and dislike this one at the same
time. Counter-example from some old code of mine:

(offLmod == offRmod ? ror_d0 :
minlen <= BITS ? ror_short :
offLmod < offRmod ? ror_long_dn :
ror_long_up )
(bsL->data+offLdiv, bsR->data+offRdiv, offLmod, offRmod, minlen);


/Nisse

 

[Tim Harig]

you don't often see + used in isolation without making use of its
value

And that is exaclty the point. The same is not true for if/else. It is
entirely conceivable that you would write an if/else without any
assignments that persist after the if/else.

if (condition)
/* enable bus */
else
/* print error */

not really. In fact not at all

?: is more properly written as a this or that statement. While it doesn't
require the value that it returns to be used, that *is* the way that it was
designed to be used and the expectation that many programmers are going to
have when they see it. The first thing I am going to wonder if I see an
isolated ?: is why they forgot to use the value. An if/else does not
violate these expectations.

 

[Tim Harig]

I have written many a printk that contains conditional terms. But
it works well for arbitrary function's parameters.
prepare_mem(p->buf, p->is_big ? BIG : SMALL);
So again, I dispute your so-called facts, no matter how they are
worded.

1. I have already said that I don't mind as much when used in an output
statement because it doesn't alter the control flow structure of
the program.

2. Your example is non-sequator to the argumentation that followed it.

3. It would seem obvious to me to either change the function declaration so
that it uses the same values as p->is_big or modify the struct
definition so that it uses the same values expected by the
function. Personally, p->is_big should have been an enumeration
in the first place. This isn't a ?: problem, it is a design
problem.

 

[Tim Harig]

What Tim meant, as he clarified in a later followup, is that the
use of ?: implies, or should imply, that the resulting value is
going to be used, i.e., not discarded. I tend to agree; if you're
going to discard the result, you might as well use an if statement.
Obviously "assignment" wasn't the righ word for this, but one can
very loosely think of the result of an expression being "assigned"
to the enclosing context.

Thank-you.

 

[Tim Harig]

"expressions", not "statements", but I think this is probably true.
You similarly don't tend to see many addition operators where their value
isn't used. In general, operators are used only when we want their
values, except when they have side effects.

Exactly. The same is not however true for if/else statements. It is quite
reasonable to see an if/else statement where no value is assigned that
persists outside of the if/else block.

if (condition)
/* enable bus */
else
/* print error */

That's not really "overloading".

No, I didn't have a more exact term to describe it, and since C does not
allow overloading, I thought that it might be descriptive in this case.

The fact is that you are using p in two different ways to send two
different kinds of information. This isn't really all that different from
the ways many standard functions like fgetc() return an int for what is
expected to be a char value so that they can also send error information
along with any values. This is questionable practice that tends to cause
lots of trouble (in this case particularly to newcomers.)

In general, it is a much better practice to explicitly separate the data
outputs. The structure is sent in channel while the error code is sent
separately out of channel. If I was going to write a function that creates
p, I often use something like this:

typedef enum fulfillment_type {SUCCESS, FAILURE} fulfillment;

/*
creates a valid printer_dev at the buffer pointed to by printer -- returns
SUCCESS or FAILURE depending on whether a valid pring_dev could actually be
created
*/
fulfillment create_printer(printer_dev* printer);

In this way, I have an explicitly differentiated the error information from
the data itself.

Furthermore, I have allowed the possibility of later expanding the
interface to include values other then success or failure which may help
with error handling down the road. As somebody pointed out, there are
different reasons why the printer may not be available. NULL only
allows you to say that it isn't available; but, provides no information as
to why. It could be that the printer is not physically available or that
there are printers available but the user has not choosen which to use for
this operation. By using an external error indicator, I might have my
error handing code call a printer select dialog if printers are available
but none have been chosen.

That seems much, much, harder to arrange.

Not really. There may be several layers on top of the extra printer
handling functionality. In this example, one would expect to see code
which confirms that there is valid data to print, converts the data into a
format that the printer understands, then it actually calls the printer
handling code to send data to the printer. If the code checks for a valid
printer when a printing operation is requested, then non of this needs to
be called.

No, it's only a requirement for all the places at which you could enter
the code. If the only interface provided is
do_print(printer *p, struct print_me *data);
then only do_print has to handle this.

Agreed, but the code that calls do_printer still must be able to handle the
differing ways in which do_printer might fail.

Yes, but the way you handle it is by storing a null pointer to indicate
that there isn't anything to point to.

As I have already said, a NULL pointer indicates that there is an error.
It doesn't explain why. Even with an absence of requirements that different
errors handled differently, it is much more descriptive to say:

if (is_printer_available())
/* ... */
else
/* error code */

then:

if (printer != NULL)
/* ... */
else
/* error code */

The second only tells me that there is a problem. The second tells me that
there is an error because no printer is available.

That seems a LOT more difficult. You then have to have every single place
that might want to ask that someone do something involving a printer check
that, which is more work.

Which is easier?

Case #1:
int do_print(printer *p, struct print_me *data) {
if (!p)
return -1;
/* code using p */
}

...
do_print(default_printer, file_data);
...
do_print(alternate_printer, file_data);
...
do_print(color_printer, stored_entries);

Case #2:
int do_print(printer *p, struct print_me *data) {
/* code using p */
}
...
if (default_printer)
do_print(default_printer, file_data);
...
if (alternate_printer)
do_print(alternate_printer, file_data);
...
if (color_printer)
do_print(color_printer, stored_entries);

You mistaken in my reasoning. Likely, the test will be performed much
higher up before the system even attempts to generate information to send
to the printer. It makes no sense to do this much if there is not going to
be an printer to send it to. Secondly, the test will likely only need to
be in a single place inside of the function interface that actually
initiates any printer operations.

I think the former makes more sense, and is also more robust.

First see above, since test only needs to be called at the first common
entry into the printing system it is no less robust.

Second, while allowing an error to propogate through the code, there are
many more places that it could fail. Allowing a NULL printer value not
only creates problems in the actual printer handling code but could cause
issues where the program has to decide what output to send to the printer
etc.

In my model, it is illegal to call print handling code if the printer is
not valid in the first place. This would be enforced by assertions to make
sure the code is never used in this way. Since the code is never even
called for an invalid printer, there is no way that any piece of code which
was not properly designed to handle the missing printer could cause any
issues. I call this more robust.

That it might be, but it is easy to imagine a case where the
simplest thing to do is to merge the cases since "no printer exists"
is clearly very much like errors a printer could have.

The simplest thing to do isn't always the best thing to do.

It might be, but if the handling for error conditions is reasonably elaborate,
being able to route both cases through a single hunk of code may be easier
to maintain. Minimize special cases early and often!

Congratulations, this is the first decent argument that I have seen used in
this sub-thread. I do however disagree that this is a problem.

1. I would not even enounter this problem because I have made sure this
incident never arises as in my model, this code never would have
been called.

2. I don't think this really adds any significant complication and omits
the "special case" far earlier so that the same bit of code is not
complicated in having to use a value in two different ways.