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

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

On Thu, Apr 9, 2015 at 10:14 AM, Doug Evans <dje@google.com> wrote:
> 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

Yes.

> 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

My patch removes all unnecessary free_debug_section calls.
I don't know if it will run out of memory.

> 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.

We need to update sh_size to get readelf to work right.
Feel free to try and make sure that "make check" passes.


-- 
H.J.