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

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

[Alexander Ivchenko]

Hi,


Currently, when there is an access beyond the end of the merged
section (I faced that situation when linking the object file with
badly-generated debug info), gold will crash with internal error:

>ld.gold --eh-frame-hdr  -shared -m elf_i386 RenderBox.o
>ld.gold: internal error in value_from_output_section, at binutils-2.24/gold/reloc.cc:1485

While gnu ld will warn us about it and finish the job:

>ld.bfd --eh-frame-hdr  -shared -m elf_i386 RenderBox.o
ld.bfd: RenderBox.o: access beyond end of merged section (1997144320)


Internal error is in any way a wrong thing here, because there is
clearly a problem with an object file itself. The following patch adds
an error message when gold sees an incorrect access in merged section,
e.g:

>ld-new  --eh-frame-hdr  -shared -m elf_i386 RenderBox.o
ld-new: error: RenderBox.o: access beyond end of merged section (1997144320)



diff --git a/gold/ChangeLog b/gold/ChangeLog
index 264f127..0b617ca 100644
--- a/gold/ChangeLog
+++ b/gold/ChangeLog
@@ -1,3 +1,13 @@
+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.
+
 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..18f7420 100644
--- a/gold/merge.cc
+++ b/gold/merge.cc
@@ -145,7 +145,8 @@ bool
 Object_merge_map::get_output_offset(const Merge_map* merge_map,
     unsigned int shndx,
     section_offset_type input_offset,
-    section_offset_type* output_offset)
+    section_offset_type* output_offset,
+    const Relobj* relobj)
 {
   Input_merge_map* map = this->get_input_merge_map(shndx);
   if (map == NULL
@@ -174,7 +175,13 @@ Object_merge_map::get_output_offset(const
Merge_map* merge_map,

   if (input_offset - p->input_offset
       >= static_cast<section_offset_type>(p->length))
-    return false;
+    {
+      if (relobj == NULL)
+ return false;
+      else
+ relobj->error(_("access beyond end of merged section (%ld)"),
+      input_offset);
+    }

   *output_offset = p->output_offset;
   if (*output_offset != -1)
@@ -268,7 +275,7 @@ Merge_map::get_output_offset(const Relobj* object,
unsigned int shndx,
   if (object_merge_map == NULL)
     return false;
   return object_merge_map->get_output_offset(this, shndx, offset,
-     output_offset);
+     output_offset, NULL);
 }

 // Return whether this is the merge section for SHNDX in OBJECT.
diff --git a/gold/merge.h b/gold/merge.h
index b4fd8e1..6df9879 100644
--- a/gold/merge.h
+++ b/gold/merge.h
@@ -66,11 +66,15 @@ class Object_merge_map
   // 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
-  // the start of the merged data in the output section.
+  // the start of the merged data in the output section. If RELOBJ
+  // is not NULL, then failure to find the output offset is fatal, but in
+  // some cases we can gracefully finish the job by providing the error
+  // information.
   bool
   get_output_offset(const Merge_map*, unsigned int shndx,
     section_offset_type offset,
-    section_offset_type* output_offset);
+    section_offset_type* output_offset,
+    const Relobj* relobj);

   // Return whether this is the merge map for section SHNDX.
   bool
diff --git a/gold/reloc.cc b/gold/reloc.cc
index 115ab37..8f09fef 100644
--- a/gold/reloc.cc
+++ b/gold/reloc.cc
@@ -1476,7 +1476,8 @@ Merged_symbol_value<size>::value_from_output_section(
   section_offset_type output_offset;
   bool found = object->merge_map()->get_output_offset(NULL, input_shndx,
       input_offset,
-      &output_offset);
+      &output_offset,
+      object);

   // If this assertion fails, it means that some relocation was
   // against a portion of an input merge section which we didn't map




 Is it ok?

 --Alexander

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

[Cary Coutant]

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

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

[Alexander Ivchenko]

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

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

[Alexander Ivchenko]

*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

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

[Alexander Ivchenko]

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

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

[Cary Coutant]

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

I don't think that Object_merge_map::get_output_offset should assert
on these conditions -- there are other callers that may legitimately
expect a false return for either of these reasons. In the case of
Merged_symbol_value::value_from_output_section(), I think that we've
gone long enough without ever hitting that assert for either of
reasons (1) or (2) that it's safe to assume that a false return is due
to reason (3).


> There is an unfortunate necessity of const_cast, if we want to print
> out the name of the section.
> What do you think?

We really should be able to call section_name() without a const_cast.
I'll commit a patch to fix that shortly, then you can update this
patch.

Thanks, and sorry for the delay. I was on vacation, and am catching up.

-cary

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

[Alexander Ivchenko]

Thanks, Cary :)

Okey, then I'll follow your suggestions and write the comment that we
expect "false" only in case (3).

Will update the patch after your commit.

--Alexander

2014-08-05 21:13 GMT+04:00 Cary Coutant <ccoutant@google.com>:
>> 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.
>
> I don't think that Object_merge_map::get_output_offset should assert
> on these conditions -- there are other callers that may legitimately
> expect a false return for either of these reasons. In the case of
> Merged_symbol_value::value_from_output_section(), I think that we've
> gone long enough without ever hitting that assert for either of
> reasons (1) or (2) that it's safe to assume that a false return is due
> to reason (3).
>
>
>> There is an unfortunate necessity of const_cast, if we want to print
>> out the name of the section.
>> What do you think?
>
> We really should be able to call section_name() without a const_cast.
> I'll commit a patch to fix that shortly, then you can update this
> patch.
>
> Thanks, and sorry for the delay. I was on vacation, and am catching up.
>
> -cary