Re: Fixing a sample from K&R book using cake static analyser

Anton Shepelev <anton.txt@gmail.moc> writes:

Ben Bacarisse:
>

I don't get why the goto crowd want to complicate it so
much.

>
Will someone post a goto-less that fixes what I perceive as
bugs in the original:
>
  a.  the failure to free() a newly allocated nlist in case
      of a later error,
>
  b.  the failure to free() a newly allocated np->name in
      case of a later error,
>
  c.  the failure to keep np->defn unchaged if the
      allocation of the new defn value failed.
>
And my perception be wrong, let us first establish the
actual bug(s).

With the usual trepidation that this is untested (though I did compile
it), I'd write something like:

struct nlist *install(const char *name, const char *defn)
{
     struct nlist *node = lookup(name);
     if (node) {
          char *new_defn = strdup(defn);
          if (new_defn) {
               free(node->defn);
               node->defn = new_defn;
               return node;
          }
          else return NULL;
     }
     else {
          struct nlist *new_node = malloc(sizeof *new_node);
          char *new_name = strdup(name), *new_defn = strdup(defn);
          if (new_node && new_name && new_defn) {
               unsigned hashval = hash(name);
               new_node->name = new_name;
               new_node->defn = new_defn;
               new_node->next = hashtab[hashval];
               return hashtab[hashval] = new_node;
          }
          else {
               free(new_defn);
               free(new_name);
               free(new_node);
               return NULL;
          }
     }
}

We have four cases:

  node with the name found
     new definition allocated
     new definition not allocated
  node with the name not found
     new node, name and definition all allocated
     not all of new node, name and definition allocated.

We can very simply reason about all of these situations.  For example,
when is storage freed?  Just before returning NULL because the name was
not in the table and one or more of the required allocations failed.
Are the calls to free legal?  Yes, all are initialised with the return
from malloc-like functions and are never assigned.  Pretty much anything
that you want to check can be checked by simple reasoning.  In
particular, your (a), (b) and (c) can be checked quite easily.

(I've written out all the else clauses because I want to show the "fully
structured" version.  I admit that I might sometimes save three lines by
putting "return NULL;" at the very end and allowing two cases to fall
through.)

--
Ben.