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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: test/tools/llvm-readobj/dyn-symbols.test:185
+      - Tag:   DT_SYMTAB
+        Value: 0
+      - Tag:   DT_NULL
----------------
grimar wrote:
> MaskRay wrote:
> > 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...
> > 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67078/new/

https://reviews.llvm.org/D67078