[PATCH] D116769: [ifs] Allow llvm-ifs to generate text stub from elf stub

[Haowei Wu via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:538
   // Get pointer to in-memory location of .dynstr section.
-  Expected<const uint8_t *> DynStrPtr = ElfFile.toMappedAddr(DynEnt.StrTabAddr);
-  if (!DynStrPtr)
-    return appendToError(DynStrPtr.takeError(),
-                         "when locating .dynstr section contents");
-
-  StringRef DynStr(reinterpret_cast<const char *>(DynStrPtr.get()),
-                   DynEnt.StrSize);
+  const uint8_t *DynStrPtr = ElfFile.base() + DynEnt.StrTabAddr;
+  if (HasPhdr) {
----------------
mcgrathr wrote:
> This value cannot be safely used at all.  The DT_STRTAB address is not something that can be presumed to be an offset into the file, even if that's often true.
> 
> The behavior I'd expect here is to first look for section headers.  If there are section headers at all, then purely look for the SHT_DYNSYM section and follow its sh_link to its SHT_STRTAB section and use its sh_offset and sh_size.  If there are section headers at all, but no SHT_DYNSYM section or its sh_link doesn't point to an SHT_STRTAB section, then you don't need to handle this input file.  It's invalid or empty.
> 
> Only if there are no section headers at all should you fall back to decoding based on phdrs.  This should be the same for finding the SHT_DYNAMIC section in the first place, only falling back to PT_DYNAMIC if there are no section headers at all.
> 
> Also note that for DT_NEEDED et al, strings pointed to by SHT_DYNAMIC entries, technically it's the sh_link of the SHT_DYNAMIC that tells you what string table section to use.  In practice, it's always the same one as the one that SHT_DYNSYM's sh_link points to, but by the spec they can be separate when dealing with sections.
> 
I have corrected the pointers to the .dynstr and .dynsym according to your suggestions.

I will submit another patch to correct the DT_NEEDED as it is out of the scope of this patch. Thanks for pointing that out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769