Re: [PATCH] PR binutils/18218: bad handling of .debug_str_offsets section

[Doug Evans <dje at google dot com>]

On Thu, Apr 9, 2015 at 6:59 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> We cache debug section by storing ELF section pointer in dwarf_section.
> OK for master?
>
> H.J.
>         PR binutils/18218
>         * readelf.c (load_specific_debug_section): Call free_debug_section
>         if ELF section pointer or size doesn't match.  Store ELF section
>         pointer.
>         (load_debug_section): Don't call free_debug_section here.
>         (display_debug_section): Likewise.
> ---
>  binutils/readelf.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/binutils/readelf.c b/binutils/readelf.c
> index db69b5d..ccb901b 100644
> --- a/binutils/readelf.c
> +++ b/binutils/readelf.c
> @@ -12076,11 +12076,15 @@ load_specific_debug_section (enum dwarf_section_display_enum debug,
>
>    /* If it is already loaded, do nothing.  */
>    if (section->start != NULL)
> -    return 1;
> +    {
> +      if (section->user_data == sec && section->size == sec->sh_size)
> +       return 1;
> +      free_debug_section (debug);
> +    }
>
>    snprintf (buf, sizeof (buf), _("%s section data"), section->name);
>    section->address = sec->sh_addr;
> -  section->user_data = NULL;
> +  section->user_data = sec;
>    section->start = (unsigned char *) get_data (NULL, (FILE *) file,
>                                                 sec->sh_offset, 1,
>                                                 sec->sh_size, buf);
> @@ -12146,12 +12150,6 @@ load_debug_section (enum dwarf_section_display_enum debug, void * file)
>    if (sec == NULL)
>      return 0;
>
> -  /* If we're loading from a subset of sections, and we've loaded
> -     a section matching this name before, it's likely that it's a
> -     different one.  */
> -  if (section_subset != NULL)
> -    free_debug_section (debug);
> -
>    return load_specific_debug_section (debug, sec, (FILE *) file);
>  }
>
> @@ -12205,10 +12203,6 @@ display_debug_section (int shndx, Elf_Internal_Shdr * section, FILE * file)
>          || streq (debug_displays[i].section.compressed_name, name))
>        {
>         struct dwarf_section * sec = &debug_displays [i].section;
> -       int secondary = (section != find_section (name));
> -
> -       if (secondary)
> -         free_debug_section ((enum dwarf_section_display_enum) i);
>
>         if (i == line && const_strneq (name, ".debug_line."))
>           sec->name = name;
> @@ -12226,9 +12220,6 @@ display_debug_section (int shndx, Elf_Internal_Shdr * section, FILE * file)
>             result &= debug_displays[i].display (sec, file);
>
>             section_subset = NULL;
> -
> -           if (secondary || (i != info && i != abbrev))
> -             free_debug_section ((enum dwarf_section_display_enum) i);
>           }
>
>         break;

Hi.

The above patch avoids the problem IIUC by not freeing the section so
there's no need to reload it (right?).  I'm guessing the reason it was
freed was to conserve space if possible (I can't think of another
reason).  If that's the case I think the free_debug_section calls make
more sense where they were (could be wrong though - I'm not completely
familiar with this code). Question: Why free any sections at all if
we're not going to free this particular set?  Is this just papering
over the problem? At the least, more comments should be required to
explain Why Things Are The Way They Are: I think it's not obvious that
this part:

>    /* If it is already loaded, do nothing.  */
>    if (section->start != NULL)
> -    return 1;
> +    {
> +      if (section->user_data == sec && section->size == sec->sh_size)
> +       return 1;
> +      free_debug_section (debug);
> +    }

is to avoid 18218: Namely that load_specific_debug_section has a
side-effect of modifying sec->sh_size.

Even better, treat the "sec" argument to load_specific_debug_section
as "const Elf_Internal_Shdr * sec".
Modifying sec->sh_size to be the size of the now-uncompressed section
is fragile and what led to this bug. Leave this data be the
representation of what's actually on disk, and build on it, not modify
it.