Re: [PATCH] PR gold/18695

[Andrew Senkevich <andrew dot n dot senkevich at gmail dot com>]

> Shouldn't there be generic address overflow checks which can be used
> by all targets?

As for me I am not sure it is really needed.

Here is new version of the patch.

2015-08-27  Andrew Senkevich  <andrew.senkevich@intel.com>

gold/
        PR gold/18695
        * x86_64.cc (Target_x86_64::Relocate::relocate): Added overflow checks
        for 32-bit relocations.
        (x86_64_overflow_check): New class.

diff --git a/gold/x86_64.cc b/gold/x86_64.cc
old mode 100644
new mode 100755
index 007af1d..0d0928f
--- a/gold/x86_64.cc
+++ b/gold/x86_64.cc
@@ -3320,6 +3320,32 @@ Target_x86_64<size>::do_finalize_sections(
     }
 }

+template<int size, int valsize>
+class x86_64_overflow_check
+{
+public:
+  typedef typename elfcpp::Elf_types<size>::Elf_Addr Address;
+
+  static inline bool
+  has_overflow_signed(Address value)
+  {
+    // limit = 1 << (valsize - 1) without shift count exceeding size of type
+    Address limit = static_cast<Address>(1) << ((valsize - 1) >> 1);
+    limit <<= ((valsize - 1) >> 1);
+    limit <<= ((valsize - 1) - 2 * ((valsize - 1) >> 1));
+    return value + limit > (limit << 1) - 1;
+  }
+
+  static inline bool
+  has_overflow_unsigned(Address value)
+  {
+    Address limit = static_cast<Address>(1) << ((valsize - 1) >> 1);
+    limit <<= ((valsize - 1) >> 1);
+    limit <<= ((valsize - 1) - 2 * ((valsize - 1) >> 1));
+    return value > (limit << 1) - 1;
+  }
+};
+
 // Perform a relocation.

 template<int size>
@@ -3431,24 +3457,38 @@ Target_x86_64<size>::Relocate::relocate(
       break;

     case elfcpp::R_X86_64_32:
-      // FIXME: we need to verify that value + addend fits into 32 bits:
-      //    uint64_t x = value + addend;
-      //    x == static_cast<uint64_t>(static_cast<uint32_t>(x))
-      // Likewise for other <=32-bit relocations (but see R_X86_64_32S).
-      Relocate_functions<size, false>::rela32(view, object, psymval, addend);
+      {
+ Relocate_functions<size, false>::rela32(view, object, psymval, addend);
+ typename elfcpp::Elf_types<size>::Elf_Addr value;
+ value = psymval->value(object, addend) + addend;
+ if (x86_64_overflow_check<size, 32>::has_overflow_unsigned(value))
+  gold_error_at_location(relinfo, relnum, rela.get_r_offset(),
+ _("relocation overflow"));
+      }
       break;

     case elfcpp::R_X86_64_32S:
-      // FIXME: we need to verify that value + addend fits into 32 bits:
-      //    int64_t x = value + addend;   // note this quantity is signed!
-      //    x == static_cast<int64_t>(static_cast<int32_t>(x))
-      Relocate_functions<size, false>::rela32(view, object, psymval, addend);
+      {
+ Relocate_functions<size, false>::rela32(view, object, psymval, addend);
+ typename elfcpp::Elf_types<size>::Elf_Addr value;
+ value = psymval->value(object, addend) + addend;
+ if (x86_64_overflow_check<size, 32>::has_overflow_signed(value))
+  gold_error_at_location(relinfo, relnum, rela.get_r_offset(),
+ _("relocation overflow"));
+      }
       break;

     case elfcpp::R_X86_64_PC32:
     case elfcpp::R_X86_64_PC32_BND:
-      Relocate_functions<size, false>::pcrela32(view, object, psymval, addend,
- address);
+      {
+ Relocate_functions<size, false>::pcrela32(view, object, psymval, addend,
+  address);
+ typename elfcpp::Elf_types<size>::Elf_Addr value;
+ value = psymval->value(object, addend) - address;
+ if (x86_64_overflow_check<size, 32>::has_overflow_signed(value))
+  gold_error_at_location(relinfo, relnum, rela.get_r_offset(),
+ _("relocation overflow"));
+      }
       break;

     case elfcpp::R_X86_64_16:
@@ -3471,17 +3511,24 @@ Target_x86_64<size>::Relocate::relocate(

     case elfcpp::R_X86_64_PLT32:
     case elfcpp::R_X86_64_PLT32_BND:
-      gold_assert(gsym == NULL
-  || gsym->has_plt_offset()
-  || gsym->final_value_is_known()
-  || (gsym->is_defined()
-      && !gsym->is_from_dynobj()
-      && !gsym->is_preemptible()));
-      // Note: while this code looks the same as for R_X86_64_PC32, it
-      // behaves differently because psymval was set to point to
-      // the PLT entry, rather than the symbol, in Scan::global().
-      Relocate_functions<size, false>::pcrela32(view, object, psymval, addend,
- address);
+      {
+ gold_assert(gsym == NULL
+    || gsym->has_plt_offset()
+    || gsym->final_value_is_known()
+    || (gsym->is_defined()
+ && !gsym->is_from_dynobj()
+ && !gsym->is_preemptible()));
+ // Note: while this code looks the same as for R_X86_64_PC32, it
+ // behaves differently because psymval was set to point to
+ // the PLT entry, rather than the symbol, in Scan::global().
+ Relocate_functions<size, false>::pcrela32(view, object, psymval, addend,
+  address);
+ typename elfcpp::Elf_types<size>::Elf_Addr value;
+ value = psymval->value(object, addend) - address;
+ if (x86_64_overflow_check<size, 32>::has_overflow_signed(value))
+  gold_error_at_location(relinfo, relnum, rela.get_r_offset(),
+ _("relocation overflow"));
+      }
       break;

     case elfcpp::R_X86_64_PLTOFF64:


--
WBR,
Andrew