Path: ...!eternal-september.org!feeder3.eternal-september.org!news.eternal-september.org!.POSTED!not-for-mail
From: Ben Bacarisse <ben@bsb.me.uk>
Newsgroups: comp.lang.c
Subject: Re: Fixing a sample from K&R book using cake static analyser
Date: Mon, 24 Jun 2024 00:25:56 +0100
Organization: A noiseless patient Spider
Lines: 80
Message-ID: <87zfrbnsvv.fsf@bsb.me.uk>
References: <v53sl1$35qt7$1@dont-email.me> <v558hv$3dskb$1@dont-email.me>
	<20240623034624.135@kylheku.com> <87wmmfq4if.fsf@bsb.me.uk>
	<20240624012527.8bbe16b96f5bfca10feadb5c@gmail.moc>
MIME-Version: 1.0
Content-Type: text/plain
Injection-Date: Mon, 24 Jun 2024 01:25:58 +0200 (CEST)
Injection-Info: dont-email.me; posting-host="2fe2b5b522b691828527b639c59887b1";
	logging-data="606629"; mail-complaints-to="abuse@eternal-september.org";	posting-account="U2FsdGVkX18CCM3rS6CHXfKvcUAjeFPbPcJgrIAJgxw="
User-Agent: Gnus/5.13 (Gnus v5.13)
Cancel-Lock: sha1:coDJIxweILGGNC9d7sIUmJBMiaQ=
	sha1:BUO0vPTlHTdLA/9z0l/MhNS9wus=
X-BSB-Auth: 1.85b25b1546e2c6f5c96f.20240624002556BST.87zfrbnsvv.fsf@bsb.me.uk
Bytes: 3791

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.