Skip to content

[llvm-objdump] Rework .gnu.version_d dumping #128434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Feb 23, 2025

and fix crash when vd_aux is invalid (#86611).

vd_version, vd_flags, vd_ndx, and vd_cnt in Elf{32,64}_Verdef are
16-bit. Change VerDef to use uint16_t instead.

vda_name specifies a NUL-terminated string. Update getVersionDefinitions
to remove some .c_str().

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Member

llvmbot commented Feb 23, 2025

@llvm/pr-subscribers-objectyaml

@llvm/pr-subscribers-llvm-binary-utilities

Author: Fangrui Song (MaskRay)

Changes

and fix crash when vd_aux is invalid (#86611).

vd_version, vd_flags, vd_ndx, and vd_cnt in Elf{32,64}_Verdef are
16-bit. Change VerDef to use uint16_t instead.

vda_name specifies a NUL-terminated string. Update getVersionDefinitions
to remove some .c_str().


Full diff: https://github.com/llvm/llvm-project/pull/128434.diff

8 Files Affected:

  • (modified) llvm/include/llvm/Object/ELF.h (+6-6)
  • (modified) llvm/test/tools/llvm-objdump/ELF/private-headers.test (+1)
  • (added) llvm/test/tools/llvm-objdump/ELF/verdef-invalid.test (+50)
  • (modified) llvm/test/tools/llvm-objdump/ELF/verdef.test (+19-9)
  • (modified) llvm/tools/llvm-objdump/ELFDump.cpp (+19-35)
  • (modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+1-1)
  • (modified) llvm/tools/llvm-objdump/llvm-objdump.h (+1)
  • (modified) llvm/tools/llvm-readobj/ELFDumper.cpp (+1-1)
diff --git a/llvm/include/llvm/Object/ELF.h b/llvm/include/llvm/Object/ELF.h
index 3aa1d7864fcb7..57a6db6c4e5aa 100644
--- a/llvm/include/llvm/Object/ELF.h
+++ b/llvm/include/llvm/Object/ELF.h
@@ -41,10 +41,10 @@ struct VerdAux {
 
 struct VerDef {
   unsigned Offset;
-  unsigned Version;
-  unsigned Flags;
-  unsigned Ndx;
-  unsigned Cnt;
+  uint16_t Version;
+  uint16_t Flags;
+  uint16_t Ndx;
+  uint16_t Cnt;
   unsigned Hash;
   std::string Name;
   std::vector<VerdAux> AuxV;
@@ -1057,8 +1057,8 @@ ELFFile<ELFT>::getVersionDefinitions(const Elf_Shdr &Sec) const {
 
     VerdAux Aux;
     Aux.Offset = VerdauxBuf - Start;
-    if (Verdaux->vda_name <= StrTabOrErr->size())
-      Aux.Name = std::string(StrTabOrErr->drop_front(Verdaux->vda_name));
+    if (Verdaux->vda_name < StrTabOrErr->size())
+      Aux.Name = std::string(StrTabOrErr->drop_front(Verdaux->vda_name).data());
     else
       Aux.Name = ("<invalid vda_name: " + Twine(Verdaux->vda_name) + ">").str();
     return Aux;
diff --git a/llvm/test/tools/llvm-objdump/ELF/private-headers.test b/llvm/test/tools/llvm-objdump/ELF/private-headers.test
index eefdc8440385c..15a721895525b 100644
--- a/llvm/test/tools/llvm-objdump/ELF/private-headers.test
+++ b/llvm/test/tools/llvm-objdump/ELF/private-headers.test
@@ -37,6 +37,7 @@ Sections:
        Value: 0x0
   - Name:            .gnu.version_d
     Type:            SHT_GNU_verdef
+    AddressAlign:    4
     Entries:
       - Version:         1
         Flags:           1
diff --git a/llvm/test/tools/llvm-objdump/ELF/verdef-invalid.test b/llvm/test/tools/llvm-objdump/ELF/verdef-invalid.test
new file mode 100644
index 0000000000000..8d0199244752a
--- /dev/null
+++ b/llvm/test/tools/llvm-objdump/ELF/verdef-invalid.test
@@ -0,0 +1,50 @@
+## Adapted from test/llvm-readobj/ELF/verdef-invalid.test
+## Check that we report a warning when a SHT_GNU_verdef section contains a version definition
+## that refers to an auxiliary entry that goes past the end of the section.
+
+# RUN: yaml2obj %s -o %t5
+# RUN: llvm-objdump -p %t5 2>&1 | FileCheck %s --check-prefix=AUX-PAST-END -DFILE=%t5
+# RUN: llvm-objdump -p %t5 2>&1 | FileCheck %s --check-prefix=AUX-PAST-END -DFILE=%t5
+
+# AUX-PAST-END: warning: '[[FILE]]': invalid SHT_GNU_verdef section with index 1: version definition 1 refers to an auxiliary entry that goes past the end of the section
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_DYN
+Sections:
+  - Name: .gnu.version_d
+    Type: SHT_GNU_verdef
+    Entries:
+      - Names:
+          - FOO
+    ShSize: 21
+DynamicSymbols:
+  - Name: foo
+
+## Check we report a warning when a version definition is not correctly aligned in memory.
+
+# RUN: yaml2obj %s --docnum=2 -o %t7
+# RUN: llvm-objdump -p %t7 2>&1 | FileCheck %s --check-prefix=MISALIGNED-DEF -DFILE=%t7
+# RUN: llvm-objdump -p %t7 2>&1 | FileCheck %s --check-prefix=MISALIGNED-DEF -DFILE=%t7
+
+# MISALIGNED-DEF: warning: '[[FILE]]': invalid SHT_GNU_verdef section with index 1: found a misaligned version definition entry at offset 0x0
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_DYN
+Sections:
+  - Type: Fill
+    Size: 0x1
+  - Name: .gnu.version_d
+    Type: SHT_GNU_verdef
+    Link: .dynstr
+    Info: 0x1
+    Entries:
+      - Names:
+          - FOO
+DynamicSymbols:
+  - Name: foo
diff --git a/llvm/test/tools/llvm-objdump/ELF/verdef.test b/llvm/test/tools/llvm-objdump/ELF/verdef.test
index e4ae33853deb4..dbb10bf87cbea 100644
--- a/llvm/test/tools/llvm-objdump/ELF/verdef.test
+++ b/llvm/test/tools/llvm-objdump/ELF/verdef.test
@@ -1,12 +1,14 @@
 # RUN: yaml2obj %s -o %t
-# RUN: llvm-objdump -p %t | FileCheck --strict-whitespace %s
+# RUN: llvm-objdump -p %t | FileCheck --match-full-lines --strict-whitespace %s
 
-# CHECK:      Dynamic Section:
-# CHECK-EMPTY:
-# CHECK-NEXT: Version definitions:
-# CHECK-NEXT: 1 0x01 0x075bcd15 foo
-# CHECK-NEXT: 2 0x02 0x3ade68b1 VERSION_1
-# CHECK-NEXT: 	                VERSION_2 
+#      CHECK:Dynamic Section:
+#CHECK-EMPTY:
+# CHECK-NEXT:Version definitions:
+# CHECK-NEXT:2 0x01 0x075bcd15 foo
+# CHECK-NEXT:3 0x02 0x3ade68b1 VERSION_1
+# CHECK-NEXT:	VERSION_2
+# CHECK-NEXT:4 0x00 0x0000007b VERSION_3
+# CHECK-NEXT:	VERSION_4 VERSION_5
 
 --- !ELF
 FileHeader:
@@ -24,17 +26,25 @@ Sections:
     Entries:
       - Version:         1
         Flags:           1
-        VersionNdx:      1
+        VersionNdx:      2
         Hash:            123456789
         Names:
           - foo
       - Version:         1
         Flags:           2
-        VersionNdx:      2
+        VersionNdx:      3
         Hash:            987654321
         Names:
           - VERSION_1
           - VERSION_2
+      - Version:         1
+        Flags:           0
+        VersionNdx:      4
+        Hash:            123
+        Names:
+          - VERSION_3
+          - VERSION_4
+          - VERSION_5
 DynamicSymbols:
   - Name:    bar
     Binding: STB_GLOBAL
diff --git a/llvm/tools/llvm-objdump/ELFDump.cpp b/llvm/tools/llvm-objdump/ELFDump.cpp
index e9e5b059f1786..0c9b1f3479f83 100644
--- a/llvm/tools/llvm-objdump/ELFDump.cpp
+++ b/llvm/tools/llvm-objdump/ELFDump.cpp
@@ -378,38 +378,6 @@ void ELFDumper<ELFT>::printSymbolVersionDependency(
   }
 }
 
-template <class ELFT>
-static void printSymbolVersionDefinition(const typename ELFT::Shdr &Shdr,
-                                         ArrayRef<uint8_t> Contents,
-                                         StringRef StrTab) {
-  outs() << "\nVersion definitions:\n";
-
-  const uint8_t *Buf = Contents.data();
-  uint32_t VerdefIndex = 1;
-  // sh_info contains the number of entries in the SHT_GNU_verdef section. To
-  // make the index column have consistent width, we should insert blank spaces
-  // according to sh_info.
-  uint16_t VerdefIndexWidth = std::to_string(Shdr.sh_info).size();
-  while (Buf) {
-    auto *Verdef = reinterpret_cast<const typename ELFT::Verdef *>(Buf);
-    outs() << format_decimal(VerdefIndex++, VerdefIndexWidth) << " "
-           << format("0x%02" PRIx16 " ", (uint16_t)Verdef->vd_flags)
-           << format("0x%08" PRIx32 " ", (uint32_t)Verdef->vd_hash);
-
-    const uint8_t *BufAux = Buf + Verdef->vd_aux;
-    uint16_t VerdauxIndex = 0;
-    while (BufAux) {
-      auto *Verdaux = reinterpret_cast<const typename ELFT::Verdaux *>(BufAux);
-      if (VerdauxIndex)
-        outs() << std::string(VerdefIndexWidth + 17, ' ');
-      outs() << StringRef(StrTab.drop_front(Verdaux->vda_name).data()) << '\n';
-      BufAux = Verdaux->vda_next ? BufAux + Verdaux->vda_next : nullptr;
-      ++VerdauxIndex;
-    }
-    Buf = Verdef->vd_next ? Buf + Verdef->vd_next : nullptr;
-  }
-}
-
 template <class ELFT> void ELFDumper<ELFT>::printSymbolVersion() {
   const ELFFile<ELFT> &Elf = getELFFile();
   StringRef FileName = Obj.getFileName();
@@ -426,10 +394,26 @@ template <class ELFT> void ELFDumper<ELFT>::printSymbolVersion() {
         unwrapOrError(Elf.getSection(Shdr.sh_link), FileName);
     StringRef StrTab = unwrapOrError(Elf.getStringTable(*StrTabSec), FileName);
 
-    if (Shdr.sh_type == ELF::SHT_GNU_verneed)
+    if (Shdr.sh_type == ELF::SHT_GNU_verneed) {
       printSymbolVersionDependency(Shdr);
-    else
-      printSymbolVersionDefinition<ELFT>(Shdr, Contents, StrTab);
+    } else {
+      OS << "\nVersion definitions:\n";
+      Expected<std::vector<VerDef>> V =
+          getELFFile().getVersionDefinitions(Shdr);
+      if (!V) {
+        this->reportUniqueWarning(V.takeError());
+        continue;
+      }
+      for (const VerDef &Def : *V) {
+        OS << Def.Ndx << ' ' << format_hex(Def.Flags, 4) << ' '
+           << format_hex(Def.Hash, 10) << ' ' << Def.Name << '\n';
+        if (!Def.AuxV.empty()) {
+          for (auto [I, Aux] : enumerate(Def.AuxV))
+            OS << (I ? ' ' : '\t') << Aux.Name;
+          OS << '\n';
+        }
+      }
+    }
   }
 }
 
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 99e0440dce78d..115f04a4df778 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -360,7 +360,7 @@ static StringRef ToolName;
 
 std::unique_ptr<BuildIDFetcher> BIDFetcher;
 
-Dumper::Dumper(const object::ObjectFile &O) : O(O) {
+Dumper::Dumper(const object::ObjectFile &O) : O(O), OS(outs()) {
   WarningHandler = [this](const Twine &Msg) {
     if (Warnings.insert(Msg.str()).second)
       reportWarning(Msg, this->O.getFileName());
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.h b/llvm/tools/llvm-objdump/llvm-objdump.h
index 7253cc3f4d91b..25d9c1e106a6c 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.h
+++ b/llvm/tools/llvm-objdump/llvm-objdump.h
@@ -77,6 +77,7 @@ class Dumper {
   StringSet<> Warnings;
 
 protected:
+  llvm::raw_ostream &OS;
   std::function<Error(const Twine &Msg)> WarningHandler;
 
 public:
diff --git a/llvm/tools/llvm-readobj/ELFDumper.cpp b/llvm/tools/llvm-readobj/ELFDumper.cpp
index fdae09ac767e6..e7825419ef9ec 100644
--- a/llvm/tools/llvm-readobj/ELFDumper.cpp
+++ b/llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -7668,7 +7668,7 @@ void LLVMELFDumper<ELFT>::printVersionDefinitionSection(const Elf_Shdr *Sec) {
     W.printFlags("Flags", D.Flags, ArrayRef(SymVersionFlags));
     W.printNumber("Index", D.Ndx);
     W.printNumber("Hash", D.Hash);
-    W.printString("Name", D.Name.c_str());
+    W.printString("Name", D.Name);
     W.printList(
         "Predecessors", D.AuxV,
         [](raw_ostream &OS, const VerdAux &Aux) { OS << Aux.Name.c_str(); });

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically looks fine to me. Essentially just some nits at this point.

Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
@MaskRay MaskRay requested a review from jh7370 February 25, 2025 18:01
Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to include the verdef-section.yaml changes? If so, the test is failing.

Everything else LGTM, so I'm assuming it was an accidental addition and am approving without it. If it is intended, please let me know and I'll take another look (after it's fixed).

@MaskRay
Copy link
Member Author

MaskRay commented Feb 28, 2025

Did you mean to include the verdef-section.yaml changes? If so, the test is failing.

Everything else LGTM, so I'm assuming it was an accidental addition and am approving without it. If it is intended, please let me know and I'll take another look (after it's fixed).

Thanks! llvm/test/tools/obj2yaml/ELF/verdef-section.yaml was unintended. Dropped.

@MaskRay MaskRay merged commit 7c26356 into main Feb 28, 2025
6 of 10 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/llvm-objdump-rework-gnuversion_d-dumping branch February 28, 2025 17:38
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Feb 28, 2025
and fix crash when vd_aux is invalid (#86611).

vd_version, vd_flags, vd_ndx, and vd_cnt in Elf{32,64}_Verdef are
16-bit. Change VerDef to use uint16_t instead.

vda_name specifies a NUL-terminated string. Update getVersionDefinitions
to remove some `.c_str()`.

Pull Request: llvm/llvm-project#128434
cheezeburglar pushed a commit to cheezeburglar/llvm-project that referenced this pull request Feb 28, 2025
and fix crash when vd_aux is invalid (llvm#86611).

vd_version, vd_flags, vd_ndx, and vd_cnt in Elf{32,64}_Verdef are
16-bit. Change VerDef to use uint16_t instead.

vda_name specifies a NUL-terminated string. Update getVersionDefinitions
to remove some `.c_str()`.

Pull Request: llvm#128434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants