Path: ...!eternal-september.org!feeder3.eternal-september.org!news.eternal-september.org!.POSTED!not-for-mail
From: Kaz Kylheku <643-408-1753@kylheku.com>
Newsgroups: comp.lang.c
Subject: Re: Fixing a sample from K&R book using cake static analyser
Date: Sun, 23 Jun 2024 22:36:50 -0000 (UTC)
Organization: A noiseless patient Spider
Lines: 123
Message-ID: <20240623151344.758@kylheku.com>
References: <v53sl1$35qt7$1@dont-email.me> <v558hv$3dskb$1@dont-email.me>
 <20240623034624.135@kylheku.com> <87wmmfq4if.fsf@bsb.me.uk>
Injection-Date: Mon, 24 Jun 2024 00:36:50 +0200 (CEST)
Injection-Info: dont-email.me; posting-host="0a91c74ce60a0e44ab75e91b4c3a6a2f";
	logging-data="567149"; mail-complaints-to="abuse@eternal-september.org";	posting-account="U2FsdGVkX19/+ZiA7tAMKzNde2wL8Ua+xBVbbaMbHhc="
User-Agent: slrn/pre1.0.4-9 (Linux)
Cancel-Lock: sha1:8Q3iA2OyR78tgps8UwqLC0cW92s=
Bytes: 4830

On 2024-06-23, Ben Bacarisse <ben@bsb.me.uk> wrote:
> Kaz Kylheku <643-408-1753@kylheku.com> writes:
>
>>> On Fri, 21 Jun 2024 09:45:21 -0300, Thiago Adams wrote:
>>>
>>>> Page 145, The C programming Language 2 Edition
>>>> 
>>>> /* install:  put (name, defn) in hashtab */
>>>> struct nlist *install(char *name, char *defn)
>>>> {
>>>>      struct nlist *np;
>>>>      unsigned hashval;
>>>> 
>>>>      if ((np = lookup(name)) == NULL) {  /* not found */
>>>>          np = (struct nlist *) malloc(sizeof(*np));
>>>>          if (np == NULL || (np->name = strdup(name)) == NULL)
>>>>              return NULL;
>>>>          hashval = hash(name);
>>>>          np->next = hashtab[hashval];
>>>>          hashtab[hashval] = np;
>>>>      } else      /* already there */
>>>>          free((void *) np->defn);  /* free previous defn */
>>>> 
>>>>      if ((np->defn = strdup(defn)) == NULL)
>>>>          return NULL;
>>>>      return np;
>>>> }
>
> [snip attempts at tidying up...]
>> Watch and learn:
>>
>>   struct nlist *install(char *name, char *defn)
>>   {
>>     struct nlist *existing = lookup(name);
>>
>>     if (existing) {
>>       return existing;
>>     } else {
>>       struct nlist *np = calloc(1, sizeof (struct nlist));
>>       char *dupname = strdup(name);
>>       char *dupdefn = strdup(defn);
>>       unsigned hashval = hash(name);
>>
>>       if (np && dupname && dupdefn) {
>>         np->name = dupname;
>>         np->defn = dupdefn;
>>         np->next = hashtab[hashval];
>>         hashtab[hashval] = np;
>>         return np;
>>       }
>>
>>       free(dupdefn);
>>       free(dupname);
>>       free(np);
>>
>>       return NULL;
>>     }
>>   }
>
> You've over-simplified.  The function needs to replace the definition
> with a strdup'd string (introduction another way to fail) when the name
> is found by lookup. 

I couldn't see that requirement at a glance from the way the other
code is written, but in my code I made it very clear what requirement
is being followed. All we need is to add the replacement logic
to the "existing" branch. Also, I regret not using sizeof *np:

  struct nlist *install(char *name, char *defn)
  {
    struct nlist *existing = lookup(name);

    if (existing) {
      char *dupdefn = strdup(defn);

      if (dupdefn) {
        free(existing->defn);
        existing->defn = dupdefn;
        return existing;
      }

      free(dupdefn);
    } else {
      struct nlist *np = calloc(1, sizeof (struct nlist));
      char *dupname = strdup(name);
      char *dupdefn = strdup(defn);
      unsigned hashval = hash(name);

      if (np && dupname && dupdefn) {
        np->name = dupname;
        np->defn = dupdefn;
        np->next = hashtab[hashval];
        hashtab[hashval] = np;
        return np;
      }

      free(dupdefn);
      free(dupname);
      free(np);
    }

    return NULL;
  }

The free(dupdefn) in the first case is defensive. We know that
since there is only one resource being allocated, that's the one
that is null, so this need not be called. But if the code expands
to multiple resources, it could help remind the maintainer that
there is a cleanup block that needs to be touched.

Freeing the old definition before allocating the new one is
a mistake. Though we return null to the caller to indicate that
the ooperation failed, in doing so, we have damaged the existing
data store. There is now an entry with a null pointer definition,
that the program has to defend against.

Functions like this should try not to mutate anything existing if they
are not able to allocate the resources they need in order to succeed.

-- 
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca