[llvm] r260488 - [readobj] Handle ELF files with no section table or with no program headers.

[Rafael Espíndola via llvm-commits]

On 16 February 2016 at 15:29, Michael Spencer <bigcheesegs at gmail.com> wrote:
> On Tue, Feb 16, 2016 at 11:36 AM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>> It may be fine to emit a warning, but there's no need to abort unless
>>> the user wanted to do something that can't (or is unreasonable to) be
>>> done. Other tests caught actual uses of invalid data.
>>
>> Correct. What I mean is that if that test no longer hits a particular
>> error, you should not just delete the error checking like the patch
>> did:
>>
>>      if (I == LoadSegments.begin())
>> -      report_fatal_error("Virtual address is not in any segment");
>> +      return nullptr;
>>
>> You should instead add a test where the LoadSegments are needed and
>> you can't get them because the file is corrupted. If in no case that
>> information is needed, this is now dead code and a lot more should be
>> deleted.
>
> Ah, I thought I had modified a test to expose this, but apparently not.

OK, I have extracted every independent improvement out of those patches.

Looking at what is left, it seems you actually implemented two
independent features in one patch. I have split it in the two attached
patches.

The first feature is using a .dynamic section instead of a PT_DYNAMIC.
This needs a few clarifications and tests:

* Is it expected that the ELF has program headers and is just missing
the PT_DYNAMIC entry? If that is not what is expected, should we
error?
* If it is expected that the ELF has no PT_LOAD, how are the addresses
in the dynamic section interpreted? How do we map them to a position
in the file? Note that this is a new way to map, it should be
implemented in *addition* to what we have in toMappedAddr and should
not delete the error checking for the case where the PT_DYNAMIC refers
to addresses in PT_LOAD.

The second patch is stranger. The comment talks about the "dynamic
section", but the code is actually working on the dynamic symbol
table. The code is trying to guess the end of the dynamic symbols.
That seems extremely odd and brittle. Why is it needed?

Cheers,
Rafael
-------------- next part --------------
diff --git a/test/Object/Inputs/invalid-sh_entsize.elf b/test/Object/Inputs/invalid-sh_entsize.elf
index ed50131..2c85a00 100755
Binary files a/test/Object/Inputs/invalid-sh_entsize.elf and b/test/Object/Inputs/invalid-sh_entsize.elf differ
diff --git a/tools/llvm-readobj/ELFDumper.cpp b/tools/llvm-readobj/ELFDumper.cpp
index e15a7eb..4ec7c89 100644
--- a/tools/llvm-readobj/ELFDumper.cpp
+++ b/tools/llvm-readobj/ELFDumper.cpp
@@ -1011,6 +1011,13 @@ ELFDumper<ELFT>::ELFDumper(const ELFFile<ELFT> *Obj, StreamWriter &Writer)
         reportError("Multilpe SHT_SYMTAB");
       DotSymtabSec = &Sec;
       break;
+    case ELF::SHT_DYNAMIC: {
+      if (DynamicTable.Addr == nullptr)
+        DynamicTable = createDRIFrom(&Sec);
+      const Elf_Shdr *DynStrSec = unwrapOrError(Obj->getSection(Sec.sh_link));
+      DynamicStringTable = unwrapOrError(Obj->getStringTable(DynStrSec));
+      break;
+    }
     case ELF::SHT_DYNSYM:
       if (DynSymRegion.Size)
         reportError("Multilpe SHT_DYNSYM");
-------------- next part --------------
diff --git a/tools/llvm-readobj/ELFDumper.cpp b/tools/llvm-readobj/ELFDumper.cpp
index 4ec7c89..63fc5a0 100644
--- a/tools/llvm-readobj/ELFDumper.cpp
+++ b/tools/llvm-readobj/ELFDumper.cpp
@@ -1134,6 +1134,50 @@ void ELFDumper<ELFT>::parseDynamicTable(
     DynamicStringTable = StringRef(StringTableBegin, StringTableSize);
   if (SONameOffset)
     SOName = getDynamicString(SONameOffset);
+  if (DynSymRegion.Addr && !DynSymRegion.Size) {
+    // There was no section table entry for the dynamic section, and there is
+    // no DT entry describing its size, so attempt to guess at its size.
+    // Initally guess that it ends at the end of the file.
+    const void *Start = DynSymRegion.Addr;
+    const void *End = Obj->base() + Obj->getBufSize();
+
+    // Check all the sections we know about.
+    for (const Elf_Shdr &Sec : Obj->sections()) {
+      const void *Addr = Obj->base() + Sec.sh_offset;
+      if (Addr >= Start && Addr < End)
+        End = Addr;
+    }
+
+    // Check all the dynamic regions we know about.
+    auto CheckDRI = [&](DynRegionInfo DRI) {
+      if (DRI.Addr >= Start && DRI.Addr < End)
+        End = DRI.Addr;
+    };
+
+    CheckDRI(DynamicTable);
+    CheckDRI(DynRelRegion);
+    CheckDRI(DynRelaRegion);
+    CheckDRI(DynPLTRelRegion);
+
+    if (DynamicStringTable.data() >= Start && DynamicStringTable.data() < End)
+      End = DynamicStringTable.data();
+
+    // Scan to the first invalid symbol.
+    auto SymI = reinterpret_cast<const Elf_Sym *>(Start);
+    for (; ((const char *)SymI + sizeof(Elf_Sym)) <= End; ++SymI) {
+      uint32_t NameOffset = SymI->st_name;
+      if (SymI > Start && !NameOffset)
+        break;
+      if (NameOffset >= DynamicStringTable.size())
+        break;
+      uint16_t SectionIndex = SymI->st_shndx;
+      if ((Obj->getNumSections() && SectionIndex >= Obj->getNumSections()) &&
+          SectionIndex < SHN_LORESERVE)
+        break;
+    }
+    End = SymI;
+    DynSymRegion.Size = (const char *)End - (const char *)Start;
+  }
 }
 
 template <typename ELFT>