This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [gold] Error out when there is an access beyond the end of the merged section
- From: Alexander Ivchenko <aivchenk at gmail dot com>
- To: Cary Coutant <ccoutant at google dot com>
- Cc: binutils <binutils at sourceware dot org>
- Date: Mon, 28 Jul 2014 13:20:15 +0400
- Subject: Re: [gold] Error out when there is an access beyond the end of the merged section
- Authentication-results: sourceware.org; auth=none
- References: <CACysShj9wK3e1eC56TffaOQknD4A8XF3NVcyYUNAKEPOux2E8w at mail dot gmail dot com> <CAHACq4rvh5Uv_tXqwgNTrir5zBB_Vi+sO1tYhLYWkWsQdi+-Sg at mail dot gmail dot com> <CACysShgkDLZWzZZtZFwandLGztSTqNkZEniXcyQDWNxE4Cj2Vg at mail dot gmail dot com> <CACysShhj92Oxyv_q-wFAX_9xi5hFh5w_318XWue3mD7_YGReKg at mail dot gmail dot com>
Hi Cary, could you please take a look at the patch.
--Alexander
2014-07-11 14:07 GMT+04:00 Alexander Ivchenko <aivchenk@gmail.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