Re: [gold] Error out when there is an access beyond the end of the merged section

[Alexander Ivchenko <aivchenk at gmail dot com>]

*kind ping*


--Alexander

2014-07-02 16:57 GMT+04:00 Alexander Ivchenko <aivchenk@gmail.com>:
> Hi Cary,
>
> I think you are right that it would be better to give an error from
> Merged_symbol_value::value_from_output_section, but I think that just
> replacing gold_assert is wrong:
>
> Here is how "found" is initialized:
> bool found = object->merge_map()->get_output_offset(NULL, input_shndx,
>                                                     input_offset,
>                                                     &output_offset,
>                                                     object);
>
> And Object_merge_map::get_output_offset can return false in three cases:
>
> 1)
>   if (map == NULL
>       || (merge_map != NULL && map->merge_map != merge_map))
>     return false;
>
> 2)
>   std::vector<Input_merge_entry>::const_iterator p =
>     std::lower_bound(map->entries.begin(), map->entries.end(),
>                      entry, Input_merge_compare());
>   if (p == map->entries.end() || p->input_offset > input_offset)
>     {
>       if (p == map->entries.begin())
>         return false;
>       --p;
>       gold_assert(p->input_offset <= input_offset);
>     }
>
> and
>
> 3)
>   if (input_offset - p->input_offset
>       >= static_cast<section_offset_type>(p->length))
>        return false;
>
> AFAIU, for cases 1) and 2) the gold_assert in
> Merged_symbol_value::value_from_output_section should really be hit,
> and only 3) should give us an error with "access beyond end of merged
> section".
>
> So, I think, we should move gold_assert to 1), 2) in order to catch
> that situation. Something like that:
>
>
> diff --git a/gold/ChangeLog b/gold/ChangeLog
> index 264f127..8608aaa 100644
> --- a/gold/ChangeLog
> +++ b/gold/ChangeLog
> @@ -1,3 +1,12 @@
> +2014-07-02  Alexander Ivchenko  <alexander.ivchenko@intel.com>
> +
> + * reloc.cc (Merged_symbol_value<size>::value_from_output_section):
> + error out (instead of gold_assert) when we see that there is an
> + access beyond the end of the merged section.
> + * merge.cc (Object_merge_map::get_output_offset): Add two additional
> + asserts.
> + * merge.h (Object_merge_map::get_output_offset): Adjust the comment.
> +
>  2014-06-27  Alan Modra  <amodra@gmail.com>
>
>   * symtab.cc (Symbol::should_add_dynsym_entry): Don't make inline.
> diff --git a/gold/merge.cc b/gold/merge.cc
> index 6d444e6..8c91b20 100644
> --- a/gold/merge.cc
> +++ b/gold/merge.cc
> @@ -148,6 +148,14 @@ Object_merge_map::get_output_offset(const
> Merge_map* merge_map,
>      section_offset_type* output_offset)
>  {
>    Input_merge_map* map = this->get_input_merge_map(shndx);
> +
> +  // If this assertion fails, then it means that we are called from
> +  // Merged_symbol_value<size>::value_from_output_section and some
> +  // relocation was against a portion of an input merge section which
> +  // we didn't map to the output file and we didn't explicitly discard.
> +  // We should always map all portions of input merge sections.
> +  gold_assert(merge_map != NULL || map != NULL);
> +
>    if (map == NULL
>        || (merge_map != NULL && map->merge_map != merge_map))
>      return false;
> @@ -166,6 +174,11 @@ Object_merge_map::get_output_offset(const
> Merge_map* merge_map,
>       entry, Input_merge_compare());
>    if (p == map->entries.end() || p->input_offset > input_offset)
>      {
> +      // Just like in the assert in the beginning of this method: we are
> +      // called from Merged_symbol_value<size>::value_from_output_section
> +      // and failed to map some portion of some input merge section.
> +      gold_assert(merge_map != NULL || p != map->entries.begin());
> +
>        if (p == map->entries.begin())
>   return false;
>        --p;
> diff --git a/gold/merge.h b/gold/merge.h
> index b4fd8e1..ff4f3af 100644
> --- a/gold/merge.h
> +++ b/gold/merge.h
> @@ -61,8 +61,9 @@ class Object_merge_map
>        section_size_type length, section_offset_type output_offset);
>
>    // Get the output offset for an input address.  MERGE_MAP is the map
> -  // we are looking for, or NULL if we don't care.  The input address
> -  // is at offset OFFSET in section SHNDX.  This sets *OUTPUT_OFFSET
> +  // we are looking for, or NULL if we don't care - in this case, we
> +  // assume that failing to find the output offset is fatal.  The input
> +  // address is at offset OFFSET in section SHNDX.  This sets *OUTPUT_OFFSET
>    // to the offset in the output section; this will be -1 if the bytes
>    // are not being copied to the output.  This returns true if the
>    // mapping is known, false otherwise.  *OUTPUT_OFFSET is relative to
> diff --git a/gold/reloc.cc b/gold/reloc.cc
> index 115ab37..dd86a08 100644
> --- a/gold/reloc.cc
> +++ b/gold/reloc.cc
> @@ -1478,11 +1478,14 @@ Merged_symbol_value<size>::value_from_output_section(
>        input_offset,
>        &output_offset);
>
> -  // If this assertion fails, it means that some relocation was
> -  // against a portion of an input merge section which we didn't map
> -  // to the output file and we didn't explicitly discard.  We should
> -  // always map all portions of input merge sections.
> -  gold_assert(found);
> +  if (!found)
> +    {
> +      // FIXME: const_cast is ugly..
> +      object->error(_("access beyond end of merged section (%s+0x%lx)"),
> +    const_cast<Relobj*>(object)->section_name(input_shndx).c_str(),
> +    static_cast<long>(input_offset));
> +      return 0;
> +    }
>
>    if (output_offset == -1)
>      return 0;
>
>
>
> There is an unfortunate necessity of const_cast, if we want to print
> out the name of the section.
> What do you think?
>
>
> --Alexander
>
> 2014-07-02 0:18 GMT+04:00 Cary Coutant <ccoutant@google.com>:
>>> +2014-07-01  Alexander Ivchenko  <alexander.ivchenko@intel.com>
>>> +
>>> + * merge.cc (Object_merge_map::get_output_offset): error out when we see
>>> + that there is an access beyond the end of the merged section.
>>> + * merge.h (Object_merge_map::get_output_offset): Add new argument.
>>> + (Merge_map::get_output_offset): Adjust
>>> + Object_merge_map::get_output_offset call with additional argument.
>>> + * reloc.cc (Merged_symbol_value<size>::value_from_output_section):
>>> + Ditto.
>>
>> I don't think Object_merge_map or Merge_map should be printing this
>> error. Instead, I'd prefer to issue the error from
>> Merged_symbol_value::value_from_output_section in place of the
>> gold_assert. Everything needed to print the error message is readily
>> available there.
>>
>> It would be nice to print the section name as well as the offset. You
>> should just be able to replace gold_assert with this:
>>
>>   if (!found)
>>     {
>>       object->error(_("access beyond end of merged section (%s+%ld)"),
>>                     object->section_name(input_shndx),
>>                     static_cast<long>(input_offset));
>>       return 0;
>>     }
>>
>> Thanks for looking at this!
>>
>> -cary