[llvm-readelf/obj] - Handle out-of-order PT_LOADs better.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=45698.

Specification says that
"Loadable segment entries in the program header table appear
in ascending order, sorted on the p_vaddr member."

Our toMappedAddr() relies on this condition. This patch
adds a warning when the sorting order of loadable segments is wrong.
In this case we force segments sorting and that allows
toMappedAddr() to work as expected.

[jhenderson]

From the description:
"Out toMappedAddr()" -> "Our toMappedAddr()"

llvm/lib/Object/ELF.cpp

581–587

I know this is what the ELF specification says, but I wonder whether it would be almost as easy to sort the segments here (maybe in addition to a warning, specified via a callback probably, if they are unsorted), rather than just giving up? That would be somewhat more useful than just warning and not doing anything.

Also, this is library code - is this code used elsewhere? It might be more appropriate for the test case to be in the Object tests.

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

419

[grimar]

• Addressed review comments.

llvm/lib/Object/ELF.cpp

581–587

Done.

[grimar]

llvm/lib/Object/ELF.cpp

581–587

Also, this is library code - is this code used elsewhere? It might be more appropriate for the test case to be in the Object tests.

Yes, it is used in lib\InterfaceStub. I've added the unit test.

[jhenderson]

llvm/unittests/Object/ELFObjectFileTest.cpp

377

I think if DataOrErr is false, we need to somehow handle the error, right? Actually maybe this should just be ASSERT_TRUE?

[grimar]

llvm/unittests/Object/ELFObjectFileTest.cpp

377

I can't use ASSERT_TRUE here (or ASSERT_THAT_EXPECTED). Its intention is to return instantly,
and so it calls return internally I think. And so I can't use it in lambda:

Error	C2440	'return': cannot convert from 'void' to 'const uint8_t *'	ObjectTests	D:\Work3\LLVM\llvm-project\llvm\unittests\Object\ELFObjectFileTest.cpp	377

I think if DataOrErr is false, we need to somehow handle the error, right?

I don't think it is very important here? We expect toMappedAddr call to succeed here,
this is what test is wrapped around. When something in a unit test goes wrong,
we can probably just report and fail, no matter how.

[grimar]

llvm/unittests/Object/ELFObjectFileTest.cpp

377

I can consume an error and return nullptr right after EXPECT_TRUE((bool)DataOrErr) probably.
It should be a bit cleaner. Should I?

[jhenderson]

llvm/unittests/Object/ELFObjectFileTest.cpp

377

If you fail to consume the error in one form or other, this test will crash if the DataOrErr check fails, either from the unchecked error or the final dereference of DataOrErr at the end of the function, rather than producing a useful "test failed" type message. I don't think that's desirable. You could always try reporting the error via the << syntax if the check fails before returning. I can't remember the exact syntax, but I believe it's something like EXPECT_TRUE((bool)DataOrErr) << toString(DataOrErr.takeError()); That being said, shouldn't this be EXPECT_THAT_EXPECTED(DataOrErr, Succeeded());? I'm guessing that consumes the error safely (you'll still need to be careful about the dereference).

[grimar]

• Addressed review comment.

llvm/unittests/Object/ELFObjectFileTest.cpp

377

I've used EXPECT_THAT_EXPECTED. Yes, it calls handleAllErrors internally.

[grimar]

• Fixed an issue with the unit test.

llvm/unittests/Object/ELFObjectFileTest.cpp

377

I've found that EXPECT_THAT_EXPECTED doesn't help much here.

The problem is in the line below:

return !DataOrErr ? nullptr : *DataOrErr;

EXPECT_THAT_EXPECTED takes the error from DataOrErr,
but the unchecked bit is then set again by the operator bool:

  explicit operator bool() {
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
    Unchecked = HasError;
#endif
    return !HasError;
  }

I think we should handle an error in a straighforward way here.

[jhenderson]

llvm/unittests/Object/ELFObjectFileTest.cpp

391

Perhaps you could add a single ASSERT_TRUE(Data) (or similar)? You could then make this and the equivalent line below something like EXPECT_EQ(Data[0], 0x11); which would give a nicer message.

[grimar]

• Addressed review comment.

llvm/unittests/Object/ELFObjectFileTest.cpp

391

Done.

[jhenderson]

LGTM.

llvm/include/llvm/Object/ELF.h

public:
   }
 
   bool isMips64EL() const { return isMipsELF64() && isLE(); }
 
   Expected<Elf_Shdr_Range> sections() const;
 
   Expected<Elf_Dyn_Range> dynamicEntries() const;
 
-  Expected<const uint8_t *> toMappedAddr(uint64_t VAddr) const;
+  Expected<const uint8_t *>
+  toMappedAddr(uint64_t VAddr,
+               WarningHandler WarnHandler = &defaultWarningHandler) const;
 
   Expected<Elf_Sym_Range> symbols(const Elf_Shdr *Sec) const {
     if (!Sec)
       return makeArrayRef<Elf_Sym>(nullptr, nullptr);
     return getSectionContentsAsArray<Elf_Sym>(*Sec);
   }
 
   Expected<Elf_Rela_Range> relas(const Elf_Shdr &Sec) const {

llvm/lib/Object/ELF.cpp

Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
   if (Dyn.back().d_tag != ELF::DT_NULL)
     // TODO: this error is untested.
     return createError("dynamic sections must be DT_NULL terminated");
 
   return Dyn;
 }
 
 template <class ELFT>
-Expected<const uint8_t *> ELFFile<ELFT>::toMappedAddr(uint64_t VAddr) const {
+Expected<const uint8_t *>
+ELFFile<ELFT>::toMappedAddr(uint64_t VAddr, WarningHandler WarnHandler) const {
   auto ProgramHeadersOrError = program_headers();
   if (!ProgramHeadersOrError)
     return ProgramHeadersOrError.takeError();
 
   llvm::SmallVector<Elf_Phdr *, 4> LoadSegments;
 
   for (const Elf_Phdr &Phdr : *ProgramHeadersOrError)
     if (Phdr.p_type == ELF::PT_LOAD)
       LoadSegments.push_back(const_cast<Elf_Phdr *>(&Phdr));
 
+  auto SortPred = [](const Elf_Phdr_Impl<ELFT> *A,
+                     const Elf_Phdr_Impl<ELFT> *B) {
+    return A->p_vaddr < B->p_vaddr;
+  };
+  if (!llvm::is_sorted(LoadSegments, SortPred)) {
+    if (Error E =
+            WarnHandler("loadable segments are unsorted by virtual address"))
+      return std::move(E);
+    llvm::stable_sort(LoadSegments, SortPred);
+  }
+
   const Elf_Phdr *const *I =
       std::upper_bound(LoadSegments.begin(), LoadSegments.end(), VAddr,
                        [](uint64_t VAddr, const Elf_Phdr_Impl<ELFT> *Phdr) {
                          return VAddr < Phdr->p_vaddr;
                        });
 
   if (I == LoadSegments.begin())
     return createError("virtual address is not in any segment: 0x" +

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

 # PAST-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}NULL{{[)]?}}      0x0
+
+## Check that we report a warning when we try to map an address, but loadable
+## segments appear unsorted by the p_vaddr member. In this case we are still
+## able to map an address.
+
+# RUN: yaml2obj %s --docnum=7 -o %t10
+# RUN: llvm-readobj --dynamic-table %t10 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10 --implicit-check-not=warning: --check-prefixes=OUT-OF-ORDER,OUT-OF-ORDER-LLVM
+# RUN: llvm-readelf --dynamic-table %t10 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10 --implicit-check-not=warning: --check-prefixes=OUT-OF-ORDER,OUT-OF-ORDER-GNU
+
+# OUT-OF-ORDER:           warning: '[[FILE]]': loadable segments are unsorted by virtual address
+
+# OUT-OF-ORDER-LLVM:      DynamicSection [ (2 entries)
+# OUT-OF-ORDER-LLVM-NEXT:   Tag                Type   Name/Value
+# OUT-OF-ORDER-LLVM-NEXT:   0x0000000000000005 STRTAB 0x1000
+# OUT-OF-ORDER-LLVM-NEXT:   0x0000000000000000 NULL   0x0
+# OUT-OF-ORDER-LLVM-NEXT: ]
+
+# OUT-OF-ORDER-GNU:       Dynamic section at offset 0xe9 contains 2 entries:
+# OUT-OF-ORDER-GNU-NEXT:   Tag                Type     Name/Value
+# OUT-OF-ORDER-GNU-NEXT:   0x0000000000000005 (STRTAB) 0x1000
+# OUT-OF-ORDER-GNU-NEXT:   0x0000000000000000 (NULL)   0x0
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+Sections:
+  - Name:    .dynstr
+    Type:    SHT_STRTAB
+    Address: 0x1000
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Address: 0x1010
+    Entries:
+      - Tag:   DT_STRTAB
+        Value: 0x1000
+      - Tag:   DT_NULL
+        Value: 0
+Symbols: []
+ProgramHeaders:
+  - Type:     PT_LOAD
+    VAddr:    0x1010
+    FirstSec: .dynamic
+    LastSec:  .dynamic
+  - Type:     PT_LOAD
+    VAddr:    0x1000
+    FirstSec: .dynstr
+    LastSec:  .dynstr
+  - Type:     PT_DYNAMIC
+    VAddr:    0x1010
+    FirstSec: .dynamic
+    LastSec:  .dynamic

llvm/tools/llvm-readobj/ELFDumper.cpp

for (const Elf_Shdr &Sec : Sections) {
     }
   }
 
   loadDynamicTable();
 }
 
 template <typename ELFT> void ELFDumper<ELFT>::parseDynamicTable() {
   auto toMappedAddr = [&](uint64_t Tag, uint64_t VAddr) -> const uint8_t * {
-    auto MappedAddrOrError = Obj.toMappedAddr(VAddr);
+    auto MappedAddrOrError = Obj.toMappedAddr(VAddr, [&](const Twine &Msg) {
+      this->reportUniqueWarning(Msg);
+      return Error::success();
+    });
     if (!MappedAddrOrError) {
       this->reportUniqueWarning("unable to parse DT_" +
                                 Obj.getDynamicTagAsString(Tag) + ": " +
                                 llvm::toString(MappedAddrOrError.takeError()));
       return nullptr;
     }
     return MappedAddrOrError.get();
   };

llvm/unittests/Object/ELFObjectFileTest.cpp

Sections:
   - Name:    .symtab_shndx
     Type:    SHT_SYMTAB_SHNDX
     Entries: [ 0 ]
     ShSize: 0xFFFFFFFF
 )");
 
   ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
 }
+
+// Test that we are able to create an ELFObjectFile even when loadable segments
+// are unsorted by virtual address.
+// Test that ELFFile<ELFT>::toMappedAddr works properly in this case.
+
+TEST(ELFObjectFileTest, InvalidLoadSegmentsOrderTest) {
+  SmallString<0> Storage;
+  Expected<ELFObjectFile<ELF64LE>> ExpectedFile = toBinary<ELF64LE>(Storage, R"(
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+Sections:
+  - Name:         .foo
+    Type:         SHT_PROGBITS
+    Address:      0x1000
+    Offset:       0x3000
+    ContentArray: [ 0x11 ]
+  - Name:         .bar
+    Type:         SHT_PROGBITS
+    Address:      0x2000
+    Offset:       0x4000
+    ContentArray: [ 0x99 ]
+ProgramHeaders:
+  - Type:     PT_LOAD
+    VAddr:    0x2000
+    FirstSec: .bar
+    LastSec:  .bar
+  - Type:     PT_LOAD
+    VAddr:    0x1000
+    FirstSec: .foo
+    LastSec:  .foo
+)");
+
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+
+  std::string WarnString;
+  auto ToMappedAddr = [&](uint64_t Addr) -> const uint8_t * {
+    Expected<const uint8_t *> DataOrErr =
+        ExpectedFile->getELFFile().toMappedAddr(Addr, [&](const Twine &Msg) {
+          EXPECT_TRUE(WarnString.empty());
+          WarnString = Msg.str();
+          return Error::success();
+        });
+
+    if (!DataOrErr) {
+      ADD_FAILURE() << toString(DataOrErr.takeError());
+      return nullptr;
+    }
+
+    EXPECT_TRUE(WarnString ==
+                "loadable segments are unsorted by virtual address");
+    WarnString = "";
+    return *DataOrErr;
+  };
+
+  const uint8_t *Data = ToMappedAddr(0x1000);
+  ASSERT_TRUE(Data);
+  MemoryBufferRef Buf = ExpectedFile->getMemoryBufferRef();
+  EXPECT_EQ((const char *)Data - Buf.getBufferStart(), 0x3000);
+  EXPECT_EQ(Data[0], 0x11);
+
+  Data = ToMappedAddr(0x2000);
+  ASSERT_TRUE(Data);
+  Buf = ExpectedFile->getMemoryBufferRef();
+  EXPECT_EQ((const char *)Data - Buf.getBufferStart(), 0x4000);
+  EXPECT_EQ(Data[0], 0x99);
+}