[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:
> > 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.
> >> // 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?
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`.


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

https://reviews.llvm.org/D67078