[PATCH] elfcompress: Handle shstrtab/symtab names more consistently

[Aaron Merey]

Hi Mark,

On Tue, May 26, 2026 at 1:09 PM Mark Wielaard <mark@klomp.org> wrote:
>
> We track the shstrtab and symtab section names in case they need to be
> renamed. These names are sometimes assigned a static string (from the
> shstrtab string pool), sometimes created as a copy of an existing
> string name and sometimes created by constructing a new string. This
> means they might leak because they are never freed.
>
> Fix this leak by defining the variables early (as NULL) before looping
> over all sections, making sure to always assign a allocated (copy) of
> the (new) name and explicitly free them at cleanup (run both on
> success and failure).
>
> This makes it easier to reason about allocation status and string
> "ownership" making it easy to see there are no memory issues even in
> the rare case of a .[z]debug section being used as a symtab or
> shstrtab table (which is why there is no testcase, because it is
> questionable whether such a file is actually valid).
>
>         * src/elfcompress.c (process_file): Define shstrtab_name,
>         shstrtab_newname, symtab_name and symtab_newname
>         early. xstrdup shstrtab_name and symtab_name if they were
>         assigned to a static string. Free all four strings at
>         cleanup.
>
> Signed-off-by: Mark Wielaard <mark@klomp.org>

LGTM.

Aaron

> ---
>  src/elfcompress.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/src/elfcompress.c b/src/elfcompress.c
> index 789bcb383786..6f21e7c330b8 100644
> --- a/src/elfcompress.c
> +++ b/src/elfcompress.c
> @@ -357,6 +357,12 @@ process_file (const char *fname)
>    /* Which sections match and need to be (un)compressed.  */
>    unsigned int *sections = NULL;
>
> +  /* Specific section names when renaming shstrtab or symtab.  */
> +  char *shstrtab_name = NULL;
> +  char *shstrtab_newname = NULL;
> +  char *symtab_name = NULL;
> +  char *symtab_newname = NULL;
> +
>    /* How many sections are we talking about?  */
>    size_t shnum = 0;
>    int res = 1;
> @@ -703,12 +709,8 @@ process_file (const char *fname)
>       difference later when we do compress.  */
>    enum ch_type shstrtab_compressed = UNSET;
>    size_t shstrtab_size = 0;
> -  char *shstrtab_name = NULL;
> -  char *shstrtab_newname = NULL;
>    enum ch_type symtab_compressed = UNSET;
>    size_t symtab_size = 0;
> -  char *symtab_name = NULL;
> -  char *symtab_newname = NULL;
>
>    /* Collection pass.  Copy over the sections, (de)compresses matching
>       sections, collect names of sections and symbol table if
> @@ -1141,6 +1143,7 @@ process_file (const char *fname)
>                      shdrstrndx);
>               goto cleanup;
>             }
> +         shstrtab_name = xstrdup (shstrtab_name);
>
>           shstrtab_size = shdr->sh_size;
>           if ((shdr->sh_flags & SHF_COMPRESSED) != 0)
> @@ -1282,6 +1285,7 @@ process_file (const char *fname)
>                              symtabndx);
>                       goto cleanup;
>                     }
> +                 symtab_name = xstrdup (symtab_name);
>
>                   symtab_size = shdr->sh_size;
>                   if ((shdr->sh_flags & SHF_COMPRESSED) != 0)
> @@ -1396,6 +1400,10 @@ cleanup:
>      }
>
>    free (sections);
> +  free (shstrtab_name);
> +  free (shstrtab_newname);
> +  free (symtab_name);
> +  free (symtab_newname);
>    return res;
>  }
>
> --
> 2.54.0
>