[v2] locale: fix memory leaks in write_locales and write_charmaps

  Fix multiple memory leaks in the locale program:

1. PUT (xstrdup (...)) leaks when tsearch finds a duplicate entry,
   since tsearch returns the existing node and the newly allocated
   string is orphaned.  Introduce PUT_UNIQUE, which looks the name up
   with GET first and only allocates when it is actually inserted.

2. String literals "POSIX" and "C" passed to PUT cannot be freed by
   tdestroy.  They now go through PUT_UNIQUE, which duplicates them,
   so tdestroy (all_data, free) is safe.

3. Add tdestroy (all_data, free) at the end of write_locales and
   write_charmaps to free the search trees.

4. Free dirents[cnt] entries in the scandir loop (only the dirents
   array pointer was freed, not the individual entries).

5. Free alias_path allocated by argz_create_sep in write_locales.

Before this change "locale -a" leaked 74 bytes in 3 blocks directly
and 835 bytes in 49 blocks indirectly, and "locale -m" leaked 2190
bytes in 227 blocks.  Both are valgrind-clean afterwards.

These leaks were reported by Arjun Shankar via GCC -fanalyzer
(OpenScanHub/Fedora).

Resolves: BZ #33972
Signed-off-by: Ruslan Valiyev <linuxoid@gmail.com>
---
 locale/programs/locale.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)


base-commit: a1e333203ac2d9f90bf9fbc09b4f43f1d16efef5
  

Hi Ruslan,

> Fix multiple memory leaks in the locale program:
>
> 1. PUT (xstrdup (...)) leaks when tsearch finds a duplicate entry,
>    since tsearch returns the existing node and the newly allocated
>    string is orphaned.  Introduce PUT_UNIQUE, which looks the name up
>    with GET first and only allocates when it is actually inserted.
>
> 2. String literals "POSIX" and "C" passed to PUT cannot be freed by
>    tdestroy.  They now go through PUT_UNIQUE, which duplicates them,
>    so tdestroy (all_data, free) is safe.
>
> 3. Add tdestroy (all_data, free) at the end of write_locales and
>    write_charmaps to free the search trees.
>
> 4. Free dirents[cnt] entries in the scandir loop (only the dirents
>    array pointer was freed, not the individual entries).
>
> 5. Free alias_path allocated by argz_create_sep in write_locales.
>
> Before this change "locale -a" leaked 74 bytes in 3 blocks directly
> and 835 bytes in 49 blocks indirectly, and "locale -m" leaked 2190
> bytes in 227 blocks.  Both are valgrind-clean afterwards.
>
> These leaks were reported by Arjun Shankar via GCC -fanalyzer
> (OpenScanHub/Fedora).
>
> Resolves: BZ #33972
> Signed-off-by: Ruslan Valiyev <linuxoid@gmail.com>

Thanks for the v2! This looks good to me.
Reviewed-by: Arjun Shankar <arjun@redhat.com>

> ---
>  locale/programs/locale.c | 36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/locale/programs/locale.c b/locale/programs/locale.c
> index 15f109f3..2e47b14f 100644
> --- a/locale/programs/locale.c
> +++ b/locale/programs/locale.c
> @@ -429,10 +429,21 @@ write_locales (void)
>  #define GET(name) tfind (name, &all_data, \
>                            (int (*) (const void *, const void *)) strcoll)
>
> +/* Insert a copy of NAME into the tree, unless an equal string is already
> +   present.  Only allocates when the name is actually new.  */
> +#define PUT_UNIQUE(name) \
> +  do \
> +    { \
> +      const char *put_name_ = (name); \
> +      if (GET (put_name_) == NULL) \
> +       PUT (xstrdup (put_name_)); \
> +    } \
> +  while (0)

OK. Do the xstrdup inside. I need to check below if every invocation
now *skips* the dup.

> +
>    /* `POSIX' locale is always available (POSIX.2 4.34.3).  */
> -  PUT ("POSIX");
> +  PUT_UNIQUE ("POSIX");
>    /* And so is the "C" locale.  */
> -  PUT ("C");
> +  PUT_UNIQUE ("C");

OK. This will dup the static strings, but the destructor will also
correctly deallocate the dupe.

>
>    memset (linebuf, '-', sizeof (linebuf) - 1);
>    linebuf[sizeof (linebuf) - 1] = '\0';
> @@ -510,8 +521,9 @@ write_locales (void)
>
>           /* If the verbose format is not selected we simply
>              collect the names.  */
> -         PUT (xstrdup (dirents[cnt]->d_name));
> +         PUT_UNIQUE (dirents[cnt]->d_name);
>         }
> +      free (dirents[cnt]);

OK.

>      }
>    if (ndirents > 0)
>      free (dirents);
> @@ -591,7 +603,7 @@ write_locales (void)
>
>                   /* Add the alias.  */
>                   if (! verbose && GET (value) != NULL)
> -                   PUT (xstrdup (alias));
> +                   PUT_UNIQUE (alias);
>                 }
>             }
>
> @@ -610,10 +622,14 @@ write_locales (void)
>        fclose (fp);
>      }
>
> +  free (alias_path);
> +

OK.

>    if (! verbose)
>      {
>        twalk (all_data, print_names);
>      }
> +
> +  tdestroy (all_data, free);

OK.

>  }
>
>
> @@ -669,7 +685,7 @@ write_archive_locales (void **all_datap, char *linebuf)
>        for (cnt = 0; cnt < head->namehash_size; ++cnt)
>         if (namehashtab[cnt].locrec_offset != 0)
>           {
> -           PUT (xstrdup (addr + namehashtab[cnt].name_offset));
> +           PUT_UNIQUE (addr + namehashtab[cnt].name_offset);
>             ++ret;
>           }
>      }
> @@ -694,7 +710,7 @@ write_archive_locales (void **all_datap, char *linebuf)
>         {
>           struct locrecent *locrec;
>
> -         PUT (xstrdup (names[cnt].name));
> +         PUT_UNIQUE (names[cnt].name);
>
>           if (cnt)
>             putchar_unlocked ('\n');
> @@ -744,19 +760,19 @@ write_charmaps (void)
>        char **aliases;
>        char **p;
>
> -      PUT (xstrdup (dirent));
> +      PUT_UNIQUE (dirent);
>
>        aliases = charmap_aliases (CHARMAP_PATH, dirent);
>
>  #if 0
>        /* Add the code_set_name and the aliases.  */
>        for (p = aliases; *p; p++)
> -       PUT (xstrdup (*p));
> +       PUT_UNIQUE (*p);
>  #else
>        /* Add the code_set_name only.  Most aliases are obsolete.  */
>        p = aliases;
>        if (*p)
> -       PUT (xstrdup (*p));
> +       PUT_UNIQUE (*p);
>  #endif
>
>        charmap_free_aliases (aliases);
> @@ -765,6 +781,8 @@ write_charmaps (void)
>    charmap_closedir (dir);
>
>    twalk (all_data, print_names);
> +
> +  tdestroy (all_data, free);

OK.

All PUT calls replaced by PUT_UNIQUE.

>  }
>
>  /* Print a properly quoted assignment of NAME with VAL, using double
>
> base-commit: a1e333203ac2d9f90bf9fbc09b4f43f1d16efef5
> --
> 2.43.0
>