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

[Arjun Shankar]

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
>


-- 
Arjun Shankar
he/him/his