[llvm-objdump] Change printSymbolVersionDependency to use ELFFile API

[MaskRay]

Summary

When .gnu.version_r is empty (allowed by readelf but warned by objdump),
llvm-objdump -p may decode the next section as .gnu.version_r and may crash due
to out-of-bounds C string reference. ELFFile<ELFT>::getVersionDependencies
handles 0-entry .gnu.version_r gracefully. Just use it.

Fix https://github.com/llvm/llvm-project/issues/57707

[jhenderson]

llvm/include/llvm/Object/ELF.h

1041

Should this be a separate patch?

llvm/test/tools/llvm-objdump/ELF/verneed-invalid.test

2

Similarly, this test seems unrelated to the commit message?

llvm/tools/llvm-objdump/ELFDump.cpp

301–306

Nit: this local variable seems a little bit unnecessary?

352–356

These variables are only used in the else part now. Could we move them?

[MaskRay]

llvm/include/llvm/Object/ELF.h

1041

Without this change llvm-objdump -p output may contain \0 separated strings, which are incorrect.

The only other use llvm/tools/llvm-readobj/ELFDumper.cpp:4706 avoids the problem by using VN.File.data().

llvm/test/tools/llvm-objdump/ELF/verneed-invalid.test

2

I need a file to test reportWarning(toString(V.takeError()), FileName);

The change is like rewriting an existing functionality. Personally I think the needed tests should be like as if we do not have the functionality yet.

llvm/tools/llvm-objdump/ELFDump.cpp

301–306

It avoids repeated outs() call at the cost of one extra line. I think it is worthwhile.

352–356

They can but they aren't so related to this change. We can rewrite printSymbolVersionDefinition to use the llvm/lib/Object API as well then these variables can go away with that change.

[raj.khem]

The patch looks good to me. I have tested it in yocto world builds, it fixes the problem and there are no new regressions.

[jhenderson]

Looks good, thanks!

llvm/test/tools/llvm-objdump/ELF/verneed-invalid.test

2

The change is like rewriting an existing functionality. Personally I think the needed tests should be like as if we do not have the functionality yet.

Agreed. If anything, I'd have added this as a prerequisite patch, but it's fine as-is. Thanks!

llvm/tools/llvm-objdump/ELFDump.cpp

352–356

Sounds reasonable.

[MaskRay]

Thanks. I'll split verneed-invalid.test

llvm/include/llvm/Object/ELF.h

const Elf_Verneed *Verneed =
         reinterpret_cast<const Elf_Verneed *>(VerneedBuf);
 
     VerNeed &VN = *Ret.emplace(Ret.end());
     VN.Version = Verneed->vn_version;
     VN.Cnt = Verneed->vn_cnt;
     VN.Offset = VerneedBuf - Start;
 
     if (Verneed->vn_file < StrTab.size())
-      VN.File = std::string(StrTab.drop_front(Verneed->vn_file));
+      VN.File = std::string(StrTab.data() + Verneed->vn_file);
     else
       VN.File = ("<corrupt vn_file: " + Twine(Verneed->vn_file) + ">").str();
 
     const uint8_t *VernauxBuf = VerneedBuf + Verneed->vn_aux;
     for (unsigned J = 0; J < Verneed->vn_cnt; ++J) {
       if (reinterpret_cast<uintptr_t>(VernauxBuf) % sizeof(uint32_t) != 0)
         return createError("invalid " + describe(*this, Sec) +
                            ": found a misaligned auxiliary entry at offset 0x" +

llvm/test/tools/llvm-objdump/ELF/verneed-invalid.test

 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objdump -p %t 2>&1 | FileCheck %s --check-prefix=BROKEN-AUX -DFILE=%t
 
 # BROKEN-AUX:      Version References:
-# BROKEN-AUX-NEXT:   required from :
-# BROKEN-AUX-NEXT:     0x00000000 0x00 00
+# BROKEN-AUX-NEXT: warning: '[[FILE]]': invalid SHT_GNU_verneed section with index 2: found a misaligned auxiliary entry at offset 0x11
+# BROKEN-AUX-NOT:  {{.}}
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_EXEC
 Sections:
   - Name:    .gnu.version

llvm/test/tools/llvm-objdump/ELF/verneed.test

Dependencies:
         Entries:
           - Name:            v3
             Hash:            4567
             Flags:           12
             Other:           2
 DynamicSymbols:
   - Name:    f1
     Binding: STB_GLOBAL
+
+# RUN: yaml2obj --docnum=2 %s -o %t.empty
+# RUN: llvm-objdump -p %t.empty 2>&1 | FileCheck %s --check-prefix=EMPTY --implicit-check-not=warning:
+
+# EMPTY:     Version References:
+# EMPTY-NOT: {{.}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:            .gnu.version_r
+    Type:            SHT_GNU_verneed
+    Flags:           [ SHF_ALLOC ]
+DynamicSymbols:
+  - Name:    f1
+    Binding: STB_GLOBAL

llvm/tools/llvm-objdump/ELFDump.cpp

outs() << "off    " << format(Fmt, (uint64_t)Phdr.p_offset) << "vaddr "
            << "memsz " << format(Fmt, (uint64_t)Phdr.p_memsz) << "flags "
            << ((Phdr.p_flags & ELF::PF_R) ? "r" : "-")
            << ((Phdr.p_flags & ELF::PF_W) ? "w" : "-")
            << ((Phdr.p_flags & ELF::PF_X) ? "x" : "-") << "\n";
   }
 }
 
 template <class ELFT>
-static void printSymbolVersionDependency(ArrayRef<uint8_t> Contents,
-                                         StringRef StrTab) {
+static void printSymbolVersionDependency(StringRef FileName,
+                                         const ELFFile<ELFT> &Obj,
+                                         const typename ELFT::Shdr &Sec) {
   outs() << "\nVersion References:\n";
 
-  const uint8_t *Buf = Contents.data();
-  while (Buf) {
-    auto *Verneed = reinterpret_cast<const typename ELFT::Verneed *>(Buf);
-    outs() << "  required from "
-           << StringRef(StrTab.drop_front(Verneed->vn_file).data()) << ":\n";
-
-    const uint8_t *BufAux = Buf + Verneed->vn_aux;
-    while (BufAux) {
-      auto *Vernaux = reinterpret_cast<const typename ELFT::Vernaux *>(BufAux);
-      outs() << "    "
-             << format("0x%08" PRIx32 " ", (uint32_t)Vernaux->vna_hash)
-             << format("0x%02" PRIx16 " ", (uint16_t)Vernaux->vna_flags)
-             << format("%02" PRIu16 " ", (uint16_t)Vernaux->vna_other)
-             << StringRef(StrTab.drop_front(Vernaux->vna_name).data()) << '\n';
-      BufAux = Vernaux->vna_next ? BufAux + Vernaux->vna_next : nullptr;
-    }
-    Buf = Verneed->vn_next ? Buf + Verneed->vn_next : nullptr;
+  auto WarningHandler = [&](const Twine &Msg) {
+    reportWarning(Msg, FileName);
+    return Error::success();
+  };
+  Expected<std::vector<VerNeed>> V =
+      Obj.getVersionDependencies(Sec, WarningHandler);
+  if (!V) {
+    reportWarning(toString(V.takeError()), FileName);
+    return;
+
+  raw_fd_ostream &OS = outs();
+  for (const VerNeed &VN : *V) {
+    OS << "  required from " << VN.File << ":\n";
+    for (const VernAux &Aux : VN.AuxV)
+      OS << format("    0x%08x 0x%02x %02u %s\n", Aux.Hash, Aux.Flags,
+                   Aux.Other, Aux.Name.c_str());
   }
 }
 
 template <class ELFT>
 static void printSymbolVersionDefinition(const typename ELFT::Shdr &Shdr,
                                          ArrayRef<uint8_t> Contents,
                                          StringRef StrTab) {
   outs() << "\nVersion definitions:\n";
static void printSymbolVersionInfo(const ELFFile<ELFT> &Elf,
                                    StringRef FileName) {
   ArrayRef<typename ELFT::Shdr> Sections =
       unwrapOrError(Elf.sections(), FileName);
   for (const typename ELFT::Shdr &Shdr : Sections) {
     if (Shdr.sh_type != ELF::SHT_GNU_verneed &&
         Shdr.sh_type != ELF::SHT_GNU_verdef)
       continue;
 
     ArrayRef<uint8_t> Contents =
         unwrapOrError(Elf.getSectionContents(Shdr), FileName);
     const typename ELFT::Shdr *StrTabSec =
         unwrapOrError(Elf.getSection(Shdr.sh_link), FileName);
     StringRef StrTab = unwrapOrError(Elf.getStringTable(*StrTabSec), FileName);
 
     if (Shdr.sh_type == ELF::SHT_GNU_verneed)
-      printSymbolVersionDependency<ELFT>(Contents, StrTab);
+      printSymbolVersionDependency<ELFT>(FileName, Elf, Shdr);
     else
       printSymbolVersionDefinition<ELFT>(Shdr, Contents, StrTab);
   }
 }
 
 void objdump::printELFFileHeader(const object::ObjectFile *Obj) {
   if (const auto *ELFObj = dyn_cast<ELF32LEObjectFile>(Obj))
     printProgramHeaders(ELFObj->getELFFile(), Obj->getFileName());