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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


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


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

https://reviews.llvm.org/D67078