[PATCH] bfd: Avoid signed overflow for new_size adjustment

["H.J. Lu" <hjl.tools@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6314 bytes --]

On Mon, Dec 5, 2022 at 9:04 PM Alan Modra via Binutils
<binutils@sourceware.org> wrote:
>
> SEC_ELF_RENAME is a flag used to effect section name changes when
> compressing/decompressing zlib-gnu debug sections.  This can be
> accomplished more directly in one of the objcopy specific bfd
> functions.  Renaming for ld input is simplified too.  Ld input object
> files always have BFD_DECOMPRESS set.
>
> bfd/
>         * compress.c (bfd_convert_section_size): Rename to..
>         (bfd_convert_section_setup): ..this.  Handle objcopy renaming
>         of compressed/decompressed debug sections.
>         * elf.c (_bfd_elf_make_section_from_shdr): Only rename zdebug
>         input for linker.
>         (elf_fake_sections): Don't handle renaming of debug sections for
>         objcopy here.
>         * section.c (SEC_ELF_RENAME): Delete.
>         * bfd-in2.h: Regenerate.
> binutils/
>         * objcopy.c (setup_section): Call bfd_convert_section_setup.
>         Don't call bfd_convert_section_size.
>
> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index 24f9305c47c..d983268563d 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -932,10 +932,6 @@ typedef struct bfd_section
>       TMS320C54X only.  */
>  #define SEC_TIC54X_BLOCK           0x10000000
>
> -  /* This section should be renamed.  This is for ELF linker
> -     internal use only.  */
> -#define SEC_ELF_RENAME             0x10000000
> -
>    /* Conditionally link this section; do not link if there are no
>       references found to any symbol in the section.  This is for TI
>       TMS320C54X only.  */
> @@ -7982,8 +7978,9 @@ void bfd_update_compression_header
>
>  int bfd_get_compression_header_size (bfd *abfd, asection *sec);
>
> -bfd_size_type bfd_convert_section_size
> -   (bfd *ibfd, asection *isec, bfd *obfd, bfd_size_type size);
> +bool bfd_convert_section_setup
> +   (bfd *ibfd, asection *isec, bfd *obfd,
> +    const char **new_name, bfd_size_type *new_size);
>
>  bool bfd_convert_section_contents
>     (bfd *ibfd, asection *isec, bfd *obfd,
> diff --git a/bfd/compress.c b/bfd/compress.c
> index a4e6a8ee7b5..bb55a6ec0ac 100644
> --- a/bfd/compress.c
> +++ b/bfd/compress.c
> @@ -225,53 +225,89 @@ bfd_get_compression_header_size (bfd *abfd, asection *sec)
>
>  /*
>  FUNCTION
> -       bfd_convert_section_size
> +       bfd_convert_section_setup
>
>  SYNOPSIS
> -       bfd_size_type bfd_convert_section_size
> -         (bfd *ibfd, asection *isec, bfd *obfd, bfd_size_type size);
> +       bool bfd_convert_section_setup
> +         (bfd *ibfd, asection *isec, bfd *obfd,
> +          const char **new_name, bfd_size_type *new_size);
>
>  DESCRIPTION
> -       Convert the size @var{size} of the section @var{isec} in input
> -       BFD @var{ibfd} to the section size in output BFD @var{obfd}.
> +       Do early setup for objcopy, when copying @var{isec} in input
> +       BFD @var{ibfd} to output BFD @var{obfd}.  Returns the name and
> +       size of the output section.
>  */
>
> -bfd_size_type
> -bfd_convert_section_size (bfd *ibfd, sec_ptr isec, bfd *obfd,
> -                         bfd_size_type size)
> +bool
> +bfd_convert_section_setup (bfd *ibfd, asection *isec, bfd *obfd,
> +                          const char **new_name, bfd_size_type *new_size)
>  {
>    bfd_size_type hdr_size;
>
> +  if ((isec->flags & SEC_DEBUGGING) != 0
> +      && (isec->flags & SEC_HAS_CONTENTS) != 0)
> +    {
> +      const char *name = *new_name;
> +
> +      if ((ibfd->flags & (BFD_DECOMPRESS | BFD_COMPRESS_GABI)) != 0)
> +       {
> +         /* When we decompress or compress with SHF_COMPRESSED,
> +            convert section name from .zdebug_* to .debug_*.  */
> +         if (startswith (name, ".zdebug_"))
> +           {
> +             name = bfd_zdebug_name_to_debug (obfd, name);
> +             if (name == NULL)
> +               return false;
> +           }
> +       }
> +
> +      /* PR binutils/18087: Compression does not always make a
> +        section smaller.  So only rename the section when
> +        compression has actually taken place.  If input section
> +        name is .zdebug_*, we should never compress it again.  */
> +      else if (isec->compress_status == COMPRESS_SECTION_DONE
> +              && startswith (name, ".debug_"))
> +       {
> +         name = bfd_debug_name_to_zdebug (obfd, name);
> +         if (name == NULL)
> +           return false;
> +       }
> +      *new_name = name;
> +    }
> +  *new_size = bfd_section_size (isec);
> +
>    /* Do nothing if either input or output aren't ELF.  */
>    if (bfd_get_flavour (ibfd) != bfd_target_elf_flavour
>        || bfd_get_flavour (obfd) != bfd_target_elf_flavour)
> -    return size;
> +    return true;
>
>    /* Do nothing if ELF classes of input and output are the same. */
>    if (get_elf_backend_data (ibfd)->s->elfclass
>        == get_elf_backend_data (obfd)->s->elfclass)
> -    return size;
> +    return true;
>
>    /* Convert GNU property size.  */
>    if (startswith (isec->name, NOTE_GNU_PROPERTY_SECTION_NAME))
> -    return _bfd_elf_convert_gnu_property_size (ibfd, obfd);
> +    {
> +      *new_size = _bfd_elf_convert_gnu_property_size (ibfd, obfd);
> +      return true;
> +    }
>
>    /* Do nothing if input file will be decompressed.  */
>    if ((ibfd->flags & BFD_DECOMPRESS))
> -    return size;
> +    return true;
>
>    /* Do nothing if the input section isn't a SHF_COMPRESSED section. */
>    hdr_size = bfd_get_compression_header_size (ibfd, isec);
>    if (hdr_size == 0)
> -    return size;
> +    return true;
>
>    /* Adjust the size of the output SHF_COMPRESSED section.  */
>    if (hdr_size == sizeof (Elf32_External_Chdr))
> -    return (size - sizeof (Elf32_External_Chdr)
> -           + sizeof (Elf64_External_Chdr));
> +    *new_size += sizeof (Elf64_External_Chdr) - sizeof (Elf32_External_Chdr);
>    else
> -    return (size - sizeof (Elf64_External_Chdr)
> -           + sizeof (Elf32_External_Chdr));
> +    *new_size += sizeof (Elf32_External_Chdr) - sizeof (Elf64_External_Chdr);

This doesn't work for 32-bit program since

 sizeof (Elf32_External_Chdr) - sizeof (Elf64_External_Chdr);

is negative and will overflow.

We should use

*new_size -= sizeof (Elf64_External_Chdr) - sizeof (Elf32_External_Chdr);

instead.

OK for master?

-- 
H.J.

[-- Attachment #2: 0001-bfd-Avoid-signed-overflow-for-new_size-adjustment.patch --]
[-- Type: application/x-patch, Size: 1229 bytes --]