[llvm-readelf] - Allow dumping the dynamic symbols when there is no program headers.

[grimar]

Summary

D62179 introduced a regression. llvm-readelf lose the ability to dump the dynamic symbols
when there is .dynamic section with a DT_SYMTAB, but there are no program headers:
https://reviews.llvm.org/D62179#1652778

Below is a program flow before the D62179 change:

1. Find SHT_DYNSYM.
2. Find there is no PT_DYNAMIC => don't try to parse it.
3. Print dynamic symbols using information about them found on step (1).

And after the change it became:

1. Find SHT_DYNSYM.
2. Find there is no PT_DYNAMIC => find SHT_DYNAMIC.
3. Parse dynamic table, but fail to handle the DT_SYMTAB because of the absence of the PT_LOAD. Report the "Virtual address is not in any segment" error.

This patch fixes the issue. For doing this it checks that the value of DT_SYMTAB was mapped to a segment. If not - it ignores it.

[grimar]

tools/llvm-readobj/ELFDumper.cpp

1655 ↗

(On Diff #218356)

for example when we want to dump the dynamic relocations and there is no sections table

This is used and tested by test\Object\relocation-executable.test

[MaskRay]

test/tools/llvm-readobj/dyn-symbols.test

185 ↗

(On Diff #218356)

The test is different from https://reviews.llvm.org/D62179#1652778

In https://reviews.llvm.org/D62179#1652778, there is no program headers (corrupted phdr), so toMappedAddr returns a nullptr. Here, DT_SYMTAB is 0. When the information from DT_SYMTAB and .dynsym conflict, we probably should let DT_DYNTAB win...

tools/llvm-readobj/ELFDumper.cpp

1656 ↗

(On Diff #218356)

We probably should check whether toMappedAddr returns a nullptr - which is the case if there is no program headers. This approach may be better than

if (!DynSymRegion.EntSize) {

[grimar]

test/tools/llvm-readobj/dyn-symbols.test

185 ↗

(On Diff #218356)

The test is different from https://reviews.llvm.org/D62179#1652778

I think not. Here is the same YAML but with the program headers:

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_DYN
  Machine: EM_X86_64
Sections:
  - Name:    .dynamic
    Type:    SHT_DYNAMIC
    Entries:
      - Tag:   DT_SYMTAB
        Value: 0
      - Tag:   DT_NULL
        Value: 0
DynamicSymbols:
  - Name: foo
ProgramHeaders:
  - Type: PT_LOAD
    Flags: [ PF_R ]
    VAddr: 0x0000
    PAddr: 0x0000
    Align: 8
    Sections:
      - Section: .dynsym

The 0 value for DT_SYMTAB is valid. I just removed the ProgramHeaders.

When the information from DT_SYMTAB and .dynsym conflict, we probably should let DT_DYNTAB win...

I do not understand why. Particulary I do not understand why we should take .dynsym size from the section headers table
(it is the only source it seems), but its address from another place.
The much simpler logic is to try to take everything from section header at first and fallback to DT_SYMTAB. This is what this patch does.

If we have different results, then I guess it would be correct to report a warning, but I am not sure why DT_SYMTAB should win.
Doesn't it mean we have a broken object? In this case why should we make any assumptions about which place is correct and which is not?

[MaskRay]

test/tools/llvm-readobj/dyn-symbols.test

185 ↗

(On Diff #218356)

There are two approaches to locate the dynamic symbol table:

• program header -> PT_DYNAMIC -> DT_SYMTAB
• section header -> .dynsym

The first approach is used by the final consumer of an executable/shared object: ld.so or some other interpreter/emulator. They locate the dynamic table (from a program header), parse it, and use DT_SYMTAB to resolve symbol lookups. They ignore the section header. Prioritizing the section header can potentially be cheated by a malicious binary.

// Information in the section header has priority over the information

// in a PT_DYNAMIC header.

So, I think we should probably prioritize the information in PT_DYNAMIC.

In this case, the program header does not exist. I think it is fine to ignore DT_SYMTAB in the first approach.

[jhenderson]

I don't have any particular strong feelings either way on the discussion in the comments, so I've just made a number of suggested minor comment fixes.

test/tools/llvm-readobj/dyn-symbols.test

166 ↗

(On Diff #218356)

dump dynamic symbols table -> dump the dynamic symbol table

192 ↗

(On Diff #218356)

to address -> to an address
of absence -> of the absence
of the corresponding -> of a corresponding

tools/llvm-readobj/ELFDumper.cpp

1648 ↗

(On Diff #218356)

That -> Doing this

1649 ↗

(On Diff #218356)

This -> This behaviour

1650 ↗

(On Diff #218356)

Remove "exist"

1651 ↗

(On Diff #218356)

symbols -> symbol

1652 ↗

(On Diff #218356)

But we -> However, we

1654 ↗

(On Diff #218356)

sections table -> section table

[grimar]

test/tools/llvm-readobj/dyn-symbols.test

185 ↗

(On Diff #218356)

Information in the section header has priority over the information
in a PT_DYNAMIC header.

So, I think we should probably prioritize the information in PT_DYNAMIC.

This is comment taken from here:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1430

And it says that for locating the .dynamic we try to take the information from the sections header first and then fallback to PT_DYNAMIC if have no SHT_DYNAMIC section. That is how llvm-readeobj scans the object now.

So are you saying we should probably reimplement it to take the information from PT_DYNAMIC at first, right?

[MaskRay]

test/tools/llvm-readobj/dyn-symbols.test

185 ↗

(On Diff #218356)

Yes, I think we should try PT_DYNAMIC first. I believe this is one of few things that GNU readelf -D (--use-dynamic) does. The option has been around for more than 20 years (it has been available since the commit 19990502 sourceware import). Fake .dynsym has been used by malware for years. I think we can just always default to -D.

[grimar]

• Reimplemented in according to discussion.
• Added more test cases to verify the new behavior.

tools/llvm-readobj/ELFDumper.cpp

1648 ↗

(On Diff #218356)

I had to rewrite this comment completely because of behavior change...

[jhenderson]

test/tools/llvm-readobj/dyn-symbols.test

220 ↗

(On Diff #218454)

to address -> to an address

251 ↗

(On Diff #218454)

of dynamic -> of the dynamic

274–275 ↗

(On Diff #218454)

Won't this be implicitly generated?

tools/llvm-readobj/ELFDumper.cpp

1647 ↗

(On Diff #218454)

dynamic symbols section -> dynamic symbol table

(technically, if there are no section headers, there's no dynamic symbol section)

1649 ↗

(On Diff #218454)

has a priority -> has priority

1650 ↗

(On Diff #218454)

in runtime -> at runtime

Normally the -> The
(should implies normally)

1652 ↗

(On Diff #218454)

to address -> to an address

1655 ↗

(On Diff #218454)

Using DynSymRegion.EntSize to determine if the dynamic symbol table has been found via a section header feels a bit weird to me. Please at least add a comment.

1659 ↗

(On Diff #218454)

of dynamic -> of the dynamic

[grimar]

• Addressed review comments.

test/tools/llvm-readobj/dyn-symbols.test

274–275 ↗

(On Diff #218454)

Yes, but this is probably not so obvious here? It probably can raise questions,
like: "will the default SHT_DYNSYM be generated if we already have one in the YAML?".
Having 2 described makes this test case pretty straightforward.

[jhenderson]

LGTM.

[MaskRay]

tools/llvm-readobj/ELFDumper.cpp

1660 ↗

(On Diff #218871)

This warning looks good :) People will stay alert when they see this warning.

llvm/trunk/test/tools/llvm-readobj/dyn-symbols.test

-RUN: llvm-readobj --dyn-symbols %p/Inputs/dynamic-table-so.x86 | FileCheck %s
+# RUN: llvm-readobj --dyn-symbols %p/Inputs/dynamic-table-so.x86 | FileCheck %s
 
-# Check the two-letter alias --dt is equivalent to the --dyn-symbols full flag
-# name.
-RUN: llvm-readobj --dt %p/Inputs/dynamic-table-so.x86 > %t.readobj-dt-alias
-RUN: llvm-readobj --dyn-symbols %p/Inputs/dynamic-table-so.x86 > %t.readobj-dt-no-alias
-RUN: diff %t.readobj-dt-alias %t.readobj-dt-no-alias
+## Check the two-letter alias --dt is equivalent to the --dyn-symbols full flag
+## name.
+
+# RUN: llvm-readobj --dt %p/Inputs/dynamic-table-so.x86 > %t.readobj-dt-alias
+# RUN: llvm-readobj --dyn-symbols %p/Inputs/dynamic-table-so.x86 > %t.readobj-dt-no-alias
+# RUN: diff %t.readobj-dt-alias %t.readobj-dt-no-alias
 
 # CHECK:      DynamicSymbols [
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name:
 # CHECK-NEXT:     Value: 0x0
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Local
 # CHECK-NEXT:     Type: None

 # CHECK-NEXT:     Value: 0x7BC
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Global
 # CHECK-NEXT:     Type: Function
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: .fini
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]
+
+## Check that we are able to dump the dynamic symbol table even when we have no program headers.
+## In this case we find the table by it's type (SHT_DYNSYM) and ignore the DT_SYMTAB value.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1.so
+# RUN: llvm-readobj %t1.so --dyn-symbols | FileCheck %s --check-prefix=NOPHDRS
+
+# NOPHDRS: Name: foo
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Entries:
+      - Tag:   DT_SYMTAB
+        Value: 0xffff1234
+      - Tag:   DT_NULL
+        Value: 0
+DynamicSymbols:
+  - Name: foo
+
+## Check we report a warning when there is no SHT_DYNSYM section and we can't map the DT_SYMTAB value
+## to an address because of the absence of a corresponding PT_LOAD program header.
+
+# RUN: yaml2obj --docnum=2 %s -o %t2.so
+# RUN: llvm-readobj %t2.so --dyn-symbols 2>&1 | FileCheck %s -DFILE=%t2.so --check-prefix=NOSHT-DYNSYM
+
+# NOSHT-DYNSYM: warning: '[[FILE]]': Unable to parse DT_SYMTAB: virtual address is not in any segment: 0x0
+# NOSHT-DYNSYM:      DynamicSymbols [
+# NOSHT-DYNSYM-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynsym
+    Type:    SHT_PROGBITS
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Entries:
+      - Tag:   DT_SYMTAB
+        Value: 0
+      - Tag:   DT_NULL
+        Value: 0
+DynamicSymbols:
+  - Name: foo
+
+## Check that when we can't map the value of the DT_SYMTAB tag to an address, we report a warning and
+## use the information in the section header table to locate the dynamic symbol table.
+
+# RUN: yaml2obj --docnum=3 %s -o %t3.so
+# RUN: llvm-readobj %t3.so --dyn-symbols 2>&1 | FileCheck -DFILE=%t3.so %s --check-prefix=BROKEN-DTSYMTAB
+
+# BROKEN-DTSYMTAB: warning: '[[FILE]]': Unable to parse DT_SYMTAB: virtual address is not in any segment: 0xffff1234
+# BROKEN-DTSYMTAB: Name: foo
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Entries:
+      - Tag:   DT_SYMTAB
+        Value: 0xffff1234
+      - Tag:   DT_NULL
+        Value: 0
+DynamicSymbols:
+  - Name: foo
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x0000
+    Sections:
+      - Section: .dynsym
+
+## Check that if we can get the location of the dynamic symbol table using both the DT_SYMTAB value
+## and the section headers table then we prefer the former and report a warning.
+
+# RUN: yaml2obj --docnum=4 %s -o %t4.so
+# RUN: llvm-readobj %t4.so --dyn-symbols 2>&1 | FileCheck -DFILE=%t4.so %s --check-prefix=PREFER-DTSYMTAB
+
+# PREFER-DTSYMTAB: warning: '[[FILE]]': SHT_DYNSYM section header and DT_SYMTAB disagree about the location of the dynamic symbol table
+# PREFER-DTSYMTAB: Name: o
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Entries:
+      - Tag:   DT_SYMTAB
+        Value: 0x0
+      - Tag:   DT_NULL
+        Value: 0
+  - Name: .dynsym
+    Type: SHT_DYNSYM
+  - Name: .mydynsym
+    Type: SHT_DYNSYM
+## The Content describes 2 symbols: zero symbol and symbol with st_name == 3.
+    Content: "000000000000000000000000000000000000000000000000030000000000000000000000000000000000000000000000"
+DynamicSymbols:
+  - Name: foo
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x0000
+    Sections:
+      - Section: .mydynsym

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

case ELF::DT_GNU_HASH:
       break;
     case ELF::DT_STRTAB:
       StringTableBegin = reinterpret_cast<const char *>(
           toMappedAddr(Dyn.getTag(), Dyn.getPtr()));
       break;
     case ELF::DT_STRSZ:
       StringTableSize = Dyn.getVal();
       break;
-    case ELF::DT_SYMTAB:
-      DynSymRegion.Addr = toMappedAddr(Dyn.getTag(), Dyn.getPtr());
+    case ELF::DT_SYMTAB: {
+      // Often we find the information about the dynamic symbol table
+      // location in the SHT_DYNSYM section header. However, the value in
+      // DT_SYMTAB has priority, because it is used by dynamic loaders to
+      // locate .dynsym at runtime. The location we find in the section header
+      // and the location we find here should match. If we can't map the
+      // DT_SYMTAB value to an address (e.g. when there are no program headers), we
+      // ignore its value.
+      if (const uint8_t *VA = toMappedAddr(Dyn.getTag(), Dyn.getPtr())) {
+        // EntSize is non-zero if the dynamic symbol table has been found via a
+        // section header.
+        if (DynSymRegion.EntSize && VA != DynSymRegion.Addr)
+          reportWarning(
+              createError(
+                  "SHT_DYNSYM section header and DT_SYMTAB disagree about "
+                  "the location of the dynamic symbol table"),
+              ObjF->getFileName());
+
+        DynSymRegion.Addr = VA;
         DynSymRegion.EntSize = sizeof(Elf_Sym);
+      }
       break;
+    }
     case ELF::DT_RELA:
       DynRelaRegion.Addr = toMappedAddr(Dyn.getTag(), Dyn.getPtr());
       break;
     case ELF::DT_RELASZ:
       DynRelaRegion.Size = Dyn.getVal();
       break;
     case ELF::DT_RELAENT:
       DynRelaRegion.EntSize = Dyn.getVal();