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

Hi Ruslan,

Thank you for working on this.

> 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 macro that checks with
>    GET (tfind) before inserting, freeing the duplicate if it already
>    exists.
>
> 2. String literals "POSIX" and "C" passed to PUT cannot be freed
>    by tdestroy. Wrap them in xstrdup 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 individual entries).
>
> 5. Free alias_path allocated by argz_create_sep in write_locales.
>
> These leaks were reported by Arjun Shankar via GCC -fanalyzer
> (OpenScanHub/Fedora) and confirmed with valgrind.
>
> Resolves: BZ #33972
> Signed-off-by: Ruslan Valiyev <[email protected]>

This looks mostly good to me. I have a couple of comments below.

Cheers!

> ---
>  locale/programs/locale.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/locale/programs/locale.c b/locale/programs/locale.c
> index 15f109f3..396821a1 100644
> --- a/locale/programs/locale.c
> +++ b/locale/programs/locale.c
> @@ -429,10 +429,20 @@ write_locales (void)
>  #define GET(name) tfind (name, &all_data, \
>                            (int (*) (const void *, const void *)) strcoll)
>
> +  /* Insert NAME into the tree, freeing it if a duplicate exists.  */
> +#define PUT_UNIQUE(name) \
> +  do {\
> +    char *put_name_ = (name);\
> +    if (GET (put_name_) != NULL)\
> +      free (put_name_);\
> +    else\
> +      PUT (put_name_);\
> +  } while (0)
> +

Since every invocation of this macro uses the result of an xstrdup, I
think it would be cleaner to pass in the un-duplicated string, perform
a GET using it, and then xstrdup only when necessary, right before
PUT. We will eliminate the allocation and subsequent free when
encountering duplicates, and it will also be a bit easier to read.

While at it, there's also a whitespace issue: a missing space before
the "\" at the end of each line.

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

This is needed so we can call "tdestroy" later.

I see that it's correct to use PUT here since we are just starting,
but it might be worth using PUT_UNIQUE everywhere just for
consistency. I'm OK either way but I just wanted to mention it.

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

OK.

>         }
> +      free (dirents[cnt]);

OK.

>      }
>    if (ndirents > 0)
>      free (dirents);
> @@ -591,7 +602,7 @@ write_locales (void)
>
>                   /* Add the alias.  */
>                   if (! verbose && GET (value) != NULL)
> -                   PUT (xstrdup (alias));
> +                   PUT_UNIQUE (xstrdup (alias));

OK.

>                 }
>             }
>
> @@ -610,10 +621,14 @@ write_locales (void)
>        fclose (fp);
>      }
>
> +  free (alias_path);
> +

OK.

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

OK.

>  }
>
>
> @@ -669,7 +684,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 (xstrdup (addr + namehashtab[cnt].name_offset));
>             ++ret;
>           }
>      }
> @@ -694,7 +709,7 @@ write_archive_locales (void **all_datap, char *linebuf)
>         {
>           struct locrecent *locrec;
>
> -         PUT (xstrdup (names[cnt].name));
> +         PUT_UNIQUE (xstrdup (names[cnt].name));

OK.

>
>           if (cnt)
>             putchar_unlocked ('\n');
> @@ -744,19 +759,19 @@ write_charmaps (void)
>        char **aliases;
>        char **p;
>
> -      PUT (xstrdup (dirent));
> +      PUT_UNIQUE (xstrdup (dirent));

OK.

>
>        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 (xstrdup (*p));

OK.

>  #else
>        /* Add the code_set_name only.  Most aliases are obsolete.  */
>        p = aliases;
>        if (*p)
> -       PUT (xstrdup (*p));
> +       PUT_UNIQUE (xstrdup (*p));

OK.

I confirmed that apart from the duplication of "POSIX" and "C" at the
start, every other insertion now uses PUT_UNIQUE.

>  #endif
>
>        charmap_free_aliases (aliases);
> @@ -765,6 +780,8 @@ write_charmaps (void)
>    charmap_closedir (dir);
>
>    twalk (all_data, print_names);
> +
> +  tdestroy (all_data, free);

OK. We can use "free" since everything inside is now the result of an
allocation.


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