[llvm-readobj] - Change how we create DynRegionInfo objects. NFCI.

[grimar]

Summary

Currently we have checkDRI and two createDRIFrom methods which
are used to create DynRegionInfo objects.

And we have an issue: constructions like:
ObjF->getELFFile()->base() + P->p_offset
that are used in createDRIFrom functions might overflow.

I had to revert D85519 which triggered such UBSan failure.

To resolve it, I am suggesting this NFC, which simplifies and
generalizes how we create DynRegionInfo objects.

It will allow us to introduce more/better validation checks in a single place.
It also will allow to change createDRI to return Expected<> so
that we will be able to stop using the reportError, which
is used inside currently, and have a warning instead.

[grimar]

This is a phab issue, btw: "grimar added a reverted change: D85519: [llvm-readobj/elf] - Refine the code for broken PT_DYNAMIC segment diagnostic.."
I've just edited the diff description. Seems using the "revert" word and diff ID in a same sentence was not the best idea..

[jhenderson]

Seems reasonable to me.

llvm/tools/llvm-readobj/ELFDumper.cpp

public:
 
   const object::ELFObjectFile<ELFT> *getElfObject() const { return ObjF; };
 
 private:
   std::unique_ptr<DumpStyle<ELFT>> ELFDumperStyle;
 
   TYPEDEF_ELF_TYPES(ELFT)
 
-  DynRegionInfo checkDRI(DynRegionInfo DRI) {
+  DynRegionInfo createDRI(uint64_t Offset, uint64_t Size, uint64_t EntSize) {
     const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-    if (DRI.Addr < Obj->base() ||
-        reinterpret_cast<const uint8_t *>(DRI.Addr) + DRI.Size >
-            Obj->base() + Obj->getBufSize())
+    const uint8_t *Addr = Obj->base() + Offset;
+    if (Addr < Obj->base() || Addr + Size > Obj->base() + Obj->getBufSize())
       reportError(errorCodeToError(llvm::object::object_error::parse_failed),
                   ObjF->getFileName());
-    return DRI;
-  }
-
-  DynRegionInfo createDRIFrom(const Elf_Phdr *P, uintX_t EntSize) {
-    return checkDRI({ObjF->getELFFile()->base() + P->p_offset, P->p_filesz,
-                     EntSize, ObjF->getFileName()});
-  }
 
-  DynRegionInfo createDRIFrom(const Elf_Shdr *S) {
-    return checkDRI({ObjF->getELFFile()->base() + S->sh_offset, S->sh_size,
-                     S->sh_entsize, ObjF->getFileName()});
+    return {Addr, Size, EntSize, ObjF->getFileName()};
   }
 
   void printAttributes();
   void printMipsReginfo();
   void printMipsOptions();
 
   std::pair<const Elf_Phdr *, const Elf_Shdr *>
   findDynamic(const ELFFile<ELFT> *Obj);
void ELFDumper<ELFT>::loadDynamicTable(const ELFFile<ELFT> *Obj) {
   const Elf_Shdr *DynamicSec;
   std::tie(DynamicPhdr, DynamicSec) = findDynamic(Obj);
   if (!DynamicPhdr && !DynamicSec)
     return;
 
   DynRegionInfo FromPhdr(ObjF->getFileName());
   bool IsPhdrTableValid = false;
   if (DynamicPhdr) {
-    FromPhdr = createDRIFrom(DynamicPhdr, sizeof(Elf_Dyn));
+    FromPhdr = createDRI(DynamicPhdr->p_offset, DynamicPhdr->p_filesz,
+                         sizeof(Elf_Dyn));
     FromPhdr.SizePrintName = "PT_DYNAMIC size";
     FromPhdr.EntSizePrintName = "";
 
     IsPhdrTableValid = !FromPhdr.getAsArrayRef<Elf_Dyn>().empty();
   }
 
   // Locate the dynamic table described in a section header.
   // Ignore sh_entsize and use the expected value for entry size explicitly.
   // This allows us to dump dynamic sections with a broken sh_entsize
   // field.
   DynRegionInfo FromSec(ObjF->getFileName());
   bool IsSecTableValid = false;
   if (DynamicSec) {
     FromSec =
-        checkDRI({ObjF->getELFFile()->base() + DynamicSec->sh_offset,
-                  DynamicSec->sh_size, sizeof(Elf_Dyn), ObjF->getFileName()});
+        createDRI(DynamicSec->sh_offset, DynamicSec->sh_size, sizeof(Elf_Dyn));
     FromSec.Context = describe(*DynamicSec);
     FromSec.EntSizePrintName = "";
 
     IsSecTableValid = !FromSec.getAsArrayRef<Elf_Dyn>().empty();
   }
 
   // When we only have information from one of the SHT_DYNAMIC section header or
   // PT_DYNAMIC program header, just use that.
case ELF::SHT_SYMTAB:
       if (!DotSymtabSec)
         DotSymtabSec = &Sec;
       break;
     case ELF::SHT_DYNSYM:
       if (!DotDynsymSec)
         DotDynsymSec = &Sec;
 
       if (!DynSymRegion) {
-        DynSymRegion = createDRIFrom(&Sec);
+        DynSymRegion = createDRI(Sec.sh_offset, Sec.sh_size, Sec.sh_entsize);
         DynSymRegion->Context = describe(Sec);
 
         if (Expected<StringRef> E = Obj->getStringTableForSymtab(Sec))
           DynamicStringTable = *E;
         else
           reportWarning(E.takeError(), ObjF->getFileName());
       }
       break;