Re: When to check the return value of malloc

Malcolm McLean wrote:


"Eric Sosman" <esosman@xxxxxxxxxxxxxxxxxxxx> wrote in message
Malcolm McLean wrote:

"Eric Sosman" <esosman@xxxxxxxxxxxxxxxxxxxx> wrote in message
1) The strategy of having a low-level routine decide the
fate of the entire program seldom works except for smallish
programs. Once the program gets sophisticated enough to be
maintaining a significant amount of state, it may well want
to do things like writing the internal state to a file before
exiting -- but if xmalloc() or xfopen() or something has
unilaterally decided to exit on its own initiative, Too Bad.

If you are caling malloc() then usually it means that the state is
currently in a corrupt or partially constructed situation. So you have
to handle that on the dump. Which means that the whole program has to be
written in a very careful, sophisticated manner.

Writing carefully is a Good Thing, certainly. But I take
issue with the notion that doing so is "sophisticated;" going
further back, I reject the "corrupt" piece. Observe that the
memory that wasn't allocated is not yet linked into any of the
program's data structures (how could it be?), so the state
dumper or state repairer or whatever will not encounter it. If
you're extremely sloppy there might be an uninitialized pointer
somewhere, but that's the fault of the programmer and not of the
malloc() failure. More typically there's just a NULL somewhere
in the data structure, and the state massager knows enough (ought
to know enough) not to follow NULLs.

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

.

Relevant Pages