[llvm-readelf] - Rework how we parse the .dynamic section.

[grimar]

Summary

This is a result of what I found during my work on https://bugs.llvm.org/show_bug.cgi?id=41679.

Previously LLVM readelf took the information about .dynamic section from its PT_DYNAMIC segment only.
GNU tools have a bit different logic. They also use the information from the .dynamic section header if it is available.
This patch changes the code to improve the compatibility with the GNU Binutils.

I also think it should resolve the problems from PR mentioned.

[jhenderson]

test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test

2 ↗

(On Diff #199572)

which is not in -> when it is not in a

8 ↗

(On Diff #199572)

Does this warning come out with GNU? Same question with the other warnings elsewhere.

test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test

5 ↗

(On Diff #199572)

either ". We check that we print a warning for this case" or ". We check that we warn about this case".

tools/llvm-readobj/ELFDumper.cpp

206–207 ↗

(On Diff #199572)

I feel like a cleaner interface would be for findDynamicTable to return a start/end offset or even an ArrayRef of the contents, and pass that into parseDynamicTable.

1330–1332 ↗

(On Diff #199572)

This feels like a weird behaviour. If it can dump the .dynamic section when it's not in a segment, why does it need the segment to exist?

1343 ↗

(On Diff #199572)

has a priority under -> has priority over

tools/llvm-readobj/llvm-readobj.cpp

377 ↗

(On Diff #199572)

Do GNU readelf warnings definitely get printed on stdout?

[jakehehrlich]

I have the same question as James with respect to requiring PT_DYNAMIC but otherwise the code looks good to me. I'm still looking at the tests.

[grimar]

• Addressed review comments.
• Fixed one more test: test/Object/corrupt.test

test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test

8 ↗

(On Diff #199572)

Yes, I think so:
(I rewrote it a bit, but idea of the message is the same).

umb@ubuntu:~/tests/9$ readelf --dynamic 1.o
readelf: Warning: the .dynamic section is not contained within the dynamic segment

Dynamic section at offset 0x1000 contains 2 entries:
  Tag        Type                         Name/Value
 0x0000000000000018 (BIND_NOW)           
 0x0000000000000000 (NULL)               0x0

umb@ubuntu:~/tests/9$ readelf -v
GNU readelf (GNU Binutils for Ubuntu) 2.31.1

test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test

11 ↗

(On Diff #199572)

GNU shows:
readelf: Warning: the .dynamic section is not the first section in the dynamic segment.

I think my version is better because:

1. I think the different start address does not mean there is a section there. It is a normal assumption, but still I have a fantazy to imagine there can be just a sequence of data before the .dynamic and no section in the section header table. Perhaps does not sound as a real case, but probably possible? Perhaps it is possible to create such test case with yaml2obj even.
2. I can imagine that SHT_DYNAMIC has name different from .dynamic. GNU code just hardoces the name. Doesn't seem as a good choice.

tools/llvm-readobj/ELFDumper.cpp

206–207 ↗

(On Diff #199572)

Hmmm. In the current design we store the tables and use them later, see:

DynRegionInfo DynRelRegion;
DynRegionInfo DynRelaRegion;
DynRegionInfo DynRelrRegion;
DynRegionInfo DynPLTRelRegion;
DynRegionInfo DynSymRegion;
DynRegionInfo DynamicTable;

When I wrote this place, I thought about the following scenario:

1. Rename the method to findAndParseDynamicTable or alike.
2. Call the parseDynamicTable from there.

(Honestly I did not try, but seems it should work).

What do you think?

1330–1332 ↗

(On Diff #199572)

Indeed. But I am following GNU behavior here. We have a test case which is failing without that lines: elf-dynamic-no-pt-dynamic.test

I.e. GNU seems to be a bit strange here, but I thought it is important to follow it for the consistency here.
Should we just check the section header and then a dynamic header (if not found) to lookup the .dynamic table?
(I am not sure so I would suggest to land this and do such change in a follow up so that we could revert it easily if that will not work for some reason).

tools/llvm-readobj/llvm-readobj.cpp

377 ↗

(On Diff #199572)

Hmm. No. Looks like I tested that it does not change the return code (and does not exit after warning), i.e.
like in my code and somehow tested where do it prints the output wrong.

After retesting I see it prints warnings to stderr, indeed. Fixed, thanks!

[jhenderson]

test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test

11 ↗

(On Diff #199572)

I agree with your comments here. It is possible to create such a test case with yaml2obj, with something like this:

Sections:
  - Name: .dynamic
    Type: SHT_DYNAMIC
    Address: 0x1000
    AddressAlign: 0x1000
    Entries:
      # ...
ProgramHeaders:
 - Type: PT_DYNAMIC
    Offset: 0x900
    Sections:
      - Section: .dynamic

Maybe it would be good for the type and name of the section to be printed (e.g. "warning: the SHT_DYNAMIC section '.dynamic' ...")

tools/llvm-readobj/ELFDumper.cpp

206–207 ↗

(On Diff #199572)

Okay. How about loadDynamicTable instead of findDynamicTable? That could parse as well, but I'm okay if it doesn't.

1330–1332 ↗

(On Diff #199572)

I agree with doing it as a separate change, thanks. I think there is no logic in being able to dump a section not in a segment, but requiring the segment to be present. Of course, it is weird having a .dynamic section without a PT_DYNAMIC segment at all, but if we handle one case, we might as well handle the whole behaviour.

[grimar]

• Addressed review comments.

test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test

11 ↗

(On Diff #199572)

I updated the warnings to include the dynamic section name.

tools/llvm-readobj/ELFDumper.cpp

206–207 ↗

(On Diff #199572)

Done. (I also moved parseDynamicTable. Not sure it is much better, but isolating it in one place is also perhaps can be reasonable)

[jhenderson]

LGTM, with one minor additional comment.

test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test

1 ↗

(On Diff #200278)

Not sure what happened, but this file is currently showing up twice in the diff!

tools/llvm-readobj/ELFDumper.cpp

1342 ↗

(On Diff #200278)

Maybe worth extending this comment with. "This matches GNU's behavior."

[grimar]

Thanks for the review, James!

test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test

1 ↗

(On Diff #200278)

It is just a 2 different tests with a similar names:
(or I do not observe this bug)

A M test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test (47 lines)
A M test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test (92 lines)

[jhenderson]

test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test

1 ↗

(On Diff #200278)

Oh, right, thanks!

test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test

1 ↗

(On Diff #200278)

Perhaps worth naming this elf-non-dynamic-in-pt-dynamic.test

[grimar]

In D61937#1505366, @jakehehrlich wrote:

I have the same question as James with respect to requiring PT_DYNAMIC but otherwise the code looks good to me. I'm still looking at the tests.

I suddenly realized that forgot about this comment when committed. Sorry, Jake, if you have any post-commit review comments about the tests I'll be happy to address them.
(I am working on the follow-up patch for this, btw, which was suggested in this comment: https://reviews.llvm.org/D61937?id=199572#inline-552013. Some tests will be changed there.)

llvm/trunk/test/Object/corrupt.test

 
 DYN-TABLE-SIZE:  Invalid entity size
 
 
 RUN: not llvm-readobj --dyn-relocations \
 RUN:   %p/Inputs/corrupt-invalid-dynamic-table-offset.elf.x86-64 2>&1 | \
 RUN:   FileCheck --check-prefix=DYN-TABLE-OFFSET %s
 
-DYN-TABLE-OFFSET: error: Invalid data was encountered while parsing the file
+DYN-TABLE-OFFSET: error: PT_DYNAMIC segment offset + size exceeds the size of the file
 
 
 RUN: not llvm-readobj --dyn-relocations \
 RUN:   %p/Inputs/corrupt-invalid-dynamic-table-too-large.elf.x86-64 2>&1 | \
 RUN:   FileCheck --check-prefix=DYN-TABLE-TOO-LARGE %s
 
-DYN-TABLE-TOO-LARGE: error: Invalid data was encountered while parsing the file
+DYN-TABLE-TOO-LARGE: error: PT_DYNAMIC segment offset + size exceeds the size of the file

llvm/trunk/test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test

+## Show that llvm-readobj/llvm-readelf tools can dump the .dynamic
+## section when it is not in a PT_DYNAMIC segment.
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj --dynamic-table %t.o 2>&1 | FileCheck %s
+# RUN: llvm-readelf --dynamic-table %t.o 2>&1 | FileCheck %s
+
+# CHECK:      warning: The SHT_DYNAMIC section '.dynamic' is not contained within the PT_DYNAMIC segment
+# CHECK:      DynamicSection [ (2 entries)
+# CHECK-NEXT:   Tag                Type     Name/Value
+# CHECK-NEXT:   0x0000000000000018 BIND_NOW 0x1
+# CHECK-NEXT:   0x0000000000000000 NULL     0x0
+# CHECK-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .dynamic
+    Type: SHT_DYNAMIC
+    Flags: [SHF_ALLOC]
+    Address: 0x1000
+    AddressAlign: 0x1000
+    Entries:
+      - Tag:   DT_BIND_NOW
+        Value: 0x1
+      - Tag:   DT_NULL
+        Value: 0x0
+  - Name: .text
+    Type: SHT_PROGBITS
+    Flags: [SHF_ALLOC]
+    Address: 0x1100
+    AddressAlign: 0x100
+    Content: "00"
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x1000
+    Sections:
+      - Section: .dynamic
+      - Section: .text
+  - Type: PT_DYNAMIC
+    VAddr: 0x1000
+    Sections:
+      - Section: .text

llvm/trunk/test/tools/llvm-readobj/elf-malformed-pt-dynamic.test

 # If the offset and/or size fields of the PT_DYNAMIC field become corrupted,
-# it will be impossible to read the dynamic segment validly. This test shows
-# that a sensible error message is given in this situation.
+# we should report a sensible error message.
 
 # Creating such a malformed file is hard. The easiest way to simulate it is to
 # truncate the file. Note that the section headers must first be stripped or
 # llvm-readobj will fail to parse the file due to the section header table
 # offset pointing outside the file.
 
 # RUN: yaml2obj %s -o %t.base
 # RUN: llvm-objcopy --strip-sections %t.base %t.stripped
 
 # Test case where the size is too large to fit in the file, but the start is
 # within the file.
 # RUN: cp %t.stripped %t.truncated1
 # RUN: %python -c "with open(r'%t.truncated1', 'r+') as f: f.truncate(0x1001)"
 # RUN: not llvm-readobj %t.truncated1 --dynamic-table 2>&1 | FileCheck %s
 
 # Test case where the offset is too large to be in the file.
 # RUN: cp %t.stripped %t.truncated2
 # RUN: %python -c "with open(r'%t.truncated2', 'r+') as f: f.truncate(0xFFF)"
 # RUN: not llvm-readobj %t.truncated2 --dynamic-table 2>&1 | FileCheck %s
 
-# CHECK: error: Invalid data was encountered while parsing the file
+# CHECK: error: PT_DYNAMIC segment offset + size exceeds the size of the file
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

llvm/trunk/test/tools/llvm-readobj/elf-non-dynamic-in-pt-dynamic.test

+## Show that llvm-readobj/llvm-readelf tools can dump the .dynamic section which
+## is not alone in PT_DYNAMIC segment.
+
+## In the first case .text is placed before .dynamic.
+## We check that we warn about this case.
+
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-readobj --dynamic-table %t.o 2>&1 | FileCheck %s --check-prefixes=WARNING,CHECK
+# RUN: llvm-readelf --dynamic-table %t.o 2>&1 | FileCheck %s --check-prefixes=WARNING,CHECK
+
+# WARNING:    warning: The SHT_DYNAMIC section '.dynamic' is not at the start of PT_DYNAMIC segment
+# CHECK:      DynamicSection [ (2 entries)
+# CHECK-NEXT:   Tag                Type     Name/Value
+# CHECK-NEXT:   0x0000000000000018 BIND_NOW 0x1
+# CHECK-NEXT:   0x0000000000000000 NULL     0x0
+# CHECK-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
+    Flags: [SHF_ALLOC]
+    Address: 0x1000
+    AddressAlign: 0x100
+    Content: "00"
+  - Name: .dynamic
+    Type: SHT_DYNAMIC
+    Flags: [SHF_ALLOC]
+    Address: 0x1100
+    AddressAlign: 0x1000
+    Entries:
+      - Tag:   DT_BIND_NOW
+        Value: 0x1
+      - Tag:   DT_NULL
+        Value: 0x0
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x1000
+    Sections:
+      - Section: .text
+      - Section: .dynamic
+  - Type: PT_DYNAMIC
+    VAddr: 0x1000
+    Sections:
+      - Section: .text
+      - Section: .dynamic
+
+## In the second case .text goes after .dynamic and we don't display any warnings.
+
+# RUN: yaml2obj --docnum=2 %s -o %t.o
+# RUN: llvm-readobj --dynamic-table %t.o | FileCheck %s --implicit-check-not="warning"
+# RUN: llvm-readelf --dynamic-table %t.o | FileCheck %s --implicit-check-not="warning"
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .dynamic
+    Type: SHT_DYNAMIC
+    Flags: [SHF_ALLOC]
+    Address: 0x1000
+    AddressAlign: 0x1000
+    Entries:
+      - Tag:   DT_BIND_NOW
+        Value: 0x1
+      - Tag:   DT_NULL
+        Value: 0x0
+  - Name: .text
+    Type: SHT_PROGBITS
+    Flags: [SHF_ALLOC]
+    Address: 0x1100
+    AddressAlign: 0x100
+    Content: "00"
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x1000
+    Sections:
+      - Section: .dynamic
+      - Section: .text
+  - Type: PT_DYNAMIC
+    VAddr: 0x1000
+    Sections:
+      - Section: .dynamic
+      - Section: .text

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

return checkDRI(
         {ObjF->getELFFile()->base() + P->p_offset, P->p_filesz, EntSize});
   }
 
   DynRegionInfo createDRIFrom(const Elf_Shdr *S) {
     return checkDRI(
         {ObjF->getELFFile()->base() + S->sh_offset, S->sh_size, S->sh_entsize});
   }
 
-  void parseDynamicTable(ArrayRef<const Elf_Phdr *> LoadSegments);
+  void loadDynamicTable(const ELFFile<ELFT> *Obj);
+  void parseDynamicTable();
 
   void printValue(uint64_t Type, uint64_t Value);
 
   StringRef getDynamicString(uint64_t Offset) const;
   StringRef getSymbolVersion(StringRef StrTab, const Elf_Sym *symb,
                              bool &IsDefault) const;
   void LoadVersionMap() const;
   void LoadVersionNeeds(const Elf_Shdr *ec) const;
static const char *getElfMipsOptionsOdkType(unsigned Odk) {
   LLVM_READOBJ_ENUM_CASE(ELF, ODK_IDENT);
   LLVM_READOBJ_ENUM_CASE(ELF, ODK_PAGESIZE);
   default:
     return "Unknown";
   }
 }
 
 template <typename ELFT>
-ELFDumper<ELFT>::ELFDumper(const object::ELFObjectFile<ELFT> *ObjF,
-    ScopedPrinter &Writer)
-    : ObjDumper(Writer), ObjF(ObjF) {
-  SmallVector<const Elf_Phdr *, 4> LoadSegments;
-  const ELFFile<ELFT> *Obj = ObjF->getELFFile();
+void ELFDumper<ELFT>::loadDynamicTable(const ELFFile<ELFT> *Obj) {
+  const Elf_Phdr *DynamicPhdr = nullptr;
   for (const Elf_Phdr &Phdr : unwrapOrError(Obj->program_headers())) {
-    if (Phdr.p_type == ELF::PT_DYNAMIC) {
-      DynamicTable = createDRIFrom(&Phdr, sizeof(Elf_Dyn));
+    if (Phdr.p_type != ELF::PT_DYNAMIC)
       continue;
-    }
-    if (Phdr.p_type != ELF::PT_LOAD || Phdr.p_filesz == 0)
+    DynamicPhdr = &Phdr;
+    break;
+
+  // We do not want to dump dynamic section if we have no PT_DYNAMIC header.
+  // This matches GNU's behavior.
+  if (!DynamicPhdr)
+    return;
+
+  // Try to locate the .dynamic section in the sections header table.
+  const Elf_Shdr *DynamicSec = nullptr;
+  for (const Elf_Shdr &Sec : unwrapOrError(Obj->sections())) {
+    if (Sec.sh_type != ELF::SHT_DYNAMIC)
       continue;
-    LoadSegments.push_back(&Phdr);
-  }
+    DynamicSec = &Sec;
+    break;
+  }
+
+  // Information in the section header has priority over the information
+  // in a PT_DYNAMIC header.
+  // Ignore sh_entsize and use the expected value for entry size explicitly.
+  // This allows us to dump the dynamic sections with a broken sh_entsize
+  // field.
+  if (DynamicSec)
+    DynamicTable = checkDRI({ObjF->getELFFile()->base() + DynamicSec->sh_offset,
+                             DynamicSec->sh_size, sizeof(Elf_Dyn)});
+
+  if (DynamicPhdr->p_offset + DynamicPhdr->p_filesz >
+      ObjF->getMemoryBufferRef().getBufferSize())
+    reportError(
+        "PT_DYNAMIC segment offset + size exceeds the size of the file");
+
+  if (!DynamicSec) {
+    DynamicTable = createDRIFrom(DynamicPhdr, sizeof(Elf_Dyn));
+    parseDynamicTable();
+    return;
+  }
+
+  StringRef Name = unwrapOrError(Obj->getSectionName(DynamicSec));
+
+  if (DynamicSec->sh_addr + DynamicSec->sh_size >
+          DynamicPhdr->p_vaddr + DynamicPhdr->p_memsz ||
+      DynamicSec->sh_addr < DynamicPhdr->p_vaddr)
+    reportWarning("The SHT_DYNAMIC section '" + Name +
+                  "' is not contained within the "
+                  "PT_DYNAMIC segment");
+
+  if (DynamicSec->sh_addr != DynamicPhdr->p_vaddr)
+    reportWarning("The SHT_DYNAMIC section '" + Name +
+                  "' is not at the start of "
+                  "PT_DYNAMIC segment");
+
+  parseDynamicTable();
 
+template <typename ELFT>
+ELFDumper<ELFT>::ELFDumper(const object::ELFObjectFile<ELFT> *ObjF,
+    ScopedPrinter &Writer)
+    : ObjDumper(Writer), ObjF(ObjF) {
+  const ELFFile<ELFT> *Obj = ObjF->getELFFile();
+
   for (const Elf_Shdr &Sec : unwrapOrError(Obj->sections())) {
     switch (Sec.sh_type) {
     case ELF::SHT_SYMTAB:
       if (DotSymtabSec != nullptr)
         reportError("Multiple SHT_SYMTAB");
       DotSymtabSec = &Sec;
       break;
     case ELF::SHT_DYNSYM:
for (const Elf_Shdr &Sec : unwrapOrError(Obj->sections())) {
     case ELF::SHT_LLVM_ADDRSIG:
       if (DotAddrsigSec != nullptr)
         reportError("Multiple .llvm_addrsig");
       DotAddrsigSec = &Sec;
       break;
     }
   }
 
-  parseDynamicTable(LoadSegments);
+  loadDynamicTable(Obj);
 
   if (opts::Output == opts::GNU)
     ELFDumperStyle.reset(new GNUStyle<ELFT>(Writer, this));
   else
     ELFDumperStyle.reset(new LLVMStyle<ELFT>(Writer, this));
 }
 
-template <typename ELFT>
-void ELFDumper<ELFT>::parseDynamicTable(
-    ArrayRef<const Elf_Phdr *> LoadSegments) {
+template <typename ELFT> void ELFDumper<ELFT>::parseDynamicTable() {
   auto toMappedAddr = [&](uint64_t VAddr) -> const uint8_t * {
     auto MappedAddrOrError = ObjF->getELFFile()->toMappedAddr(VAddr);
     if (!MappedAddrOrError)
       report_fatal_error(MappedAddrOrError.takeError());
     return MappedAddrOrError.get();
   };
 
   uint64_t SONameOffset = 0;

llvm/trunk/tools/llvm-readobj/llvm-readobj.h

 
 namespace llvm {
   namespace object {
     class RelocationRef;
   }
 
   // Various helper functions.
   LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg);
+  void reportWarning(Twine Msg);
   void error(std::error_code EC);
   void error(llvm::Error EC);
   template <typename T> T error(llvm::Expected<T> &&E) {
     error(E.takeError());
     return std::move(*E);
   }
 
   template <class T> T unwrapOrError(ErrorOr<T> EO) {

llvm/trunk/tools/llvm-readobj/llvm-readobj.cpp

 namespace llvm {
 
 LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg) {
   errs() << "\n";
   WithColor::error(errs()) << Msg << "\n";
   exit(1);
 }
 
+void reportWarning(Twine Msg) {
+  errs() << "\n";
+  WithColor::warning(errs()) << Msg << "\n";
+}
+
 void error(Error EC) {
   if (!EC)
     return;
   handleAllErrors(std::move(EC),
                   [&](const ErrorInfoBase &EI) { reportError(EI.message()); });
 }
 
 void error(std::error_code EC) {