Why do so many functions return NULL if a memory allocation fails?

[krader1961]

Please ponder why this function returns NULL if the memory allocation fails (from src/cmd/ksh93/sh/nvdisc.c):

static_fn void *newnode(const char *name) {
    size_t s;
    Namval_t *np = newof(0, Namval_t, 1, s = strlen(name) + 1);

    if (np) {
        np->nvname = (char *)np + sizeof(Namval_t);
        memcpy(np->nvname, name, s);
    }
    return (void *)np;
}

Also ponder the first assignment. Hint: it contains another assignment embedded within the ostensible function call but which is actually a macro. And because it is a macro the expression might be executed more than once. But that is a different rant than the one I want to focus on in this issue.

The question is whether these explicit returns of a null pointer on a memory allocation failure make any sense. My opinion is hell no in general. There may be instances where this makes sense. But in this specific instance the two call sites require a non-NULL return value. Thus the explicit test for a non-NULL value is pointless and only delays the failure. And AFAICT this is true for 99% of the equivalent situations in this project.

[krader1961]

FWIW, There are many bogosities in this project. But two really stand out:

1. Wrapping every return expression in parentheses. Which leads to such bogosities as return (0);.

2. The explicit if (!(ptr = (malloc(1)) return 0;, or equivalent, statements. In every single instance I've examined that just delays the failure. I have yet to see an instance where ksh would do something useful if the malloc/calloc/realloc failed and the called function short circuited as a result.

[krader1961]

Gah! Consider this block of code from sh_argprocsub() in src/cmd/ksh93/sh/args.c:

    if (fd) {
        if (!shp->procsub) {
            shp->procsub = procsub = newof(0, pid_t, shp->nprocsub = 4, 0);
        } else {

Notice anything odd about the assignment?

It has another, semi-hidden, assignment within that statement. The assignment might be mistaken for an equality test. As in so many other places where this pattern is employed. And in most of those places the compiler and lint tools like oclint warn us about the (needless) ambiguity.

[krader1961]

The code is also not consistent about dealing with dynamic allocations that fail. For example, this block silently ignores the second strdup() failure rather than return -1. And note that it also leaks the previous strdup() buffer. Lastly, the first strdup() isn't even necessary unless the path contains a / character.

[krader1961]

Another great example for why this pattern needs to be changed. See nv_clone_disc() in src/cmd/ksh93/sh/nvdisc.c. It returns NULL if the allocation fails but every caller blindly assumes the pointer is not NULL. I have yet to find a single place in the ksh code that does anything useful if a function it calls fails due to a dynamic allocation failure. That is, any place that does not immediately report a failure via errormsg() or similar mechanism when an allocation fails.

[krader1961]

See also the discussion in issue #1397. Even within the libast code the handling of failed mallocs is inconsistent. For example, in src/lib/libast/misc/optget.c there is no explicit test for a failed calloc() in multiple places. Starting with this one:

https://github.com/att/ast/blob/2828fd98a2db434fd8826265f56955a1102ea2e3/src/lib/libast/misc/optget.c#L639-L640