[libObject] Fix getDesc for Elf_Note_Impl

[jakehehrlich]

Summary

This change fixes a bug in Elf_Note_Impl in which Elf_Word was used where uint8_t should have been used.

[Bigcheese]

Overall this looks fine, but can you add a test of something that this fixes? I'm rather surprised that no tests fail now.

[jakehehrlich]

In D54451#1296292, @Bigcheese wrote:

Overall this looks fine, but can you add a test of something that this fixes? I'm rather surprised that no tests fail now.

Yeah, I think I spotted bug that could have been spotted in an integration test.

As far as I am aware there are no unit tests for libObject. The reason no tests fail makes sense as well. llvm-readobj was the only user of this and everywhere it used it it did something like the following

std::string(reinterpret_cast<const char *>(Words.data()),
                  Words.size())};

Notice that Words.size() is still 16 so this conversion actually works and fixes the faulty bug.

I think the one spot where an issue could have been noticed is in getGNUAbiTag where despite the size being say 1-3 Elf_Word's the size in bytes would have been between 4-12 all of which would not have triggered the error. I think checking for that error in an integration test would suffice but the code could change in a way that made that test stale.

[jakehehrlich]

• Added test that would have caused undefined behavior before but should be solid now.

[Bigcheese]

Ah, the bugs canceled each other out. Adding a unit test is fine. Just add a new file in llvm/unittests/Object/.

[Bigcheese]

lgtm

llvm/include/llvm/Object/ELFTypes.h

public:
   StringRef getName() const {
     if (!Nhdr.n_namesz)
       return StringRef();
     return StringRef(reinterpret_cast<const char *>(&Nhdr) + sizeof(Nhdr),
                      Nhdr.n_namesz - 1);
   }
 
   /// Get the note's descriptor.
-  ArrayRef<Elf_Word> getDesc() const {
+  ArrayRef<uint8_t> getDesc() const {
     if (!Nhdr.n_descsz)
-      return ArrayRef<Elf_Word>();
-    return ArrayRef<Elf_Word>(
-        reinterpret_cast<const Elf_Word *>(
-            reinterpret_cast<const uint8_t *>(&Nhdr) + sizeof(Nhdr) +
-            alignTo<Elf_Nhdr_Impl<ELFT>::Align>(Nhdr.n_namesz)),
+      return ArrayRef<uint8_t>();
+    return ArrayRef<uint8_t>(
+          alignTo<Elf_Nhdr_Impl<ELFT>::Align>(Nhdr.n_namesz),
         Nhdr.n_descsz);
   }
 
   /// Get the note's type.
   Elf_Word getType() const { return Nhdr.n_type; }
 };
 
 template <class ELFT>

llvm/test/tools/llvm-readobj/gnu-note-size.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-readobj -elf-output-style GNU --notes %t | FileCheck %s --check-prefix=GNU
+# RUN: llvm-readobj -elf-output-style LLVM --notes %t | FileCheck %s --check-prefix=LLVM
+
+# GNU:        Owner                 Data size       Description
+# GNU-NEXT:   GNU                   0x00000004      NT_GNU_ABI_TAG (ABI version tag)
+# GNU-NEXT:     <corrupt GNU_ABI_TAG>
+
+# LLVM:      Notes [
+# LLVM-NEXT:   NoteSection {
+# LLVM-NEXT:     Offset:
+# LLVM-NEXT:     Size: 0x14
+# LLVM-NEXT:     Note {
+# LLVM-NEXT:       Owner: GNU
+# LLVM-NEXT:       Data size: 0x4
+# LLVM-NEXT:       Type: NT_GNU_ABI_TAG (ABI version tag)
+# LLVM-NEXT:       ABI: <corrupt GNU_ABI_TAG>
+# LLVM-NEXT:     }
+# LLVM-NEXT:   }
+# LLVM-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .note.ABI-tag
+    Type:            SHT_NOTE
+    AddressAlign:    0x0000000000000004
+    Content:         040000000400000001000000474E550000000000

llvm/tools/llvm-readobj/ELFDumper.cpp

case GNU_PROPERTY_X86_FEATURE_1_AND:
     if (CFProtection)
       OS << format("<unknown flags: 0x%llx>", CFProtection);
     return OS.str();
   }
 }
 
 template <typename ELFT>
 static SmallVector<std::string, 4>
-getGNUPropertyList(ArrayRef<typename ELFT::Word> Words) {
+getGNUPropertyList(ArrayRef<uint8_t> Arr) {
   using Elf_Word = typename ELFT::Word;
 
   SmallVector<std::string, 4> Properties;
-  ArrayRef<uint8_t> Arr(reinterpret_cast<const uint8_t *>(Words.data()),
-                        Words.size());
   while (Arr.size() >= 8) {
     uint32_t Type = *reinterpret_cast<const Elf_Word *>(Arr.data());
     uint32_t DataSize = *reinterpret_cast<const Elf_Word *>(Arr.data() + 4);
     Arr = Arr.drop_front(8);
 
     // Take padding size into account if present.
     uint64_t PaddedSize = alignTo(DataSize, sizeof(typename ELFT::uint));
     std::string str;

 
 struct GNUAbiTag {
   std::string OSName;
   std::string ABI;
   bool IsValid;
 };
 
 template <typename ELFT>
-static GNUAbiTag getGNUAbiTag(ArrayRef<typename ELFT::Word> Words) {
+static GNUAbiTag getGNUAbiTag(ArrayRef<uint8_t> Desc) {
+  typedef typename ELFT::Word Elf_Word;
+
+  ArrayRef<Elf_Word> Words(reinterpret_cast<const Elf_Word*>(Desc.begin()),
+                           reinterpret_cast<const Elf_Word*>(Desc.end()));
+
   if (Words.size() < 4)
     return {"", "", /*IsValid=*/false};
 
   static const char *OSNames[] = {
       "Linux", "Hurd", "Solaris", "FreeBSD", "NetBSD", "Syllable", "NaCl",
   };
   StringRef OSName = "Unknown";
   if (Words[0] < array_lengthof(OSNames))
     OSName = OSNames[Words[0]];
   uint32_t Major = Words[1], Minor = Words[2], Patch = Words[3];
   std::string str;
   raw_string_ostream ABI(str);
   ABI << Major << "." << Minor << "." << Patch;
   return {OSName, ABI.str(), /*IsValid=*/true};
 }
 
-template <typename ELFT>
-static std::string getGNUBuildId(ArrayRef<typename ELFT::Word> Words) {
+static std::string getGNUBuildId(ArrayRef<uint8_t> Desc) {
   std::string str;
   raw_string_ostream OS(str);
-  ArrayRef<uint8_t> ID(reinterpret_cast<const uint8_t *>(Words.data()),
-                       Words.size());
-  for (const auto &B : ID)
+  for (const auto &B : Desc)
     OS << format_hex_no_prefix(B, 2);
   return OS.str();
 }
 
-template <typename ELFT>
-static StringRef getGNUGoldVersion(ArrayRef<typename ELFT::Word> Words) {
-  return StringRef(reinterpret_cast<const char *>(Words.data()), Words.size());
+static StringRef getGNUGoldVersion(ArrayRef<uint8_t> Desc) {
+  return StringRef(reinterpret_cast<const char *>(Desc.data()), Desc.size());
 }
 
 template <typename ELFT>
 static void printGNUNote(raw_ostream &OS, uint32_t NoteType,
-                         ArrayRef<typename ELFT::Word> Words) {
+                         ArrayRef<uint8_t> Desc) {
   switch (NoteType) {
   default:
     return;
   case ELF::NT_GNU_ABI_TAG: {
-    const GNUAbiTag &AbiTag = getGNUAbiTag<ELFT>(Words);
+    const GNUAbiTag &AbiTag = getGNUAbiTag<ELFT>(Desc);
     if (!AbiTag.IsValid)
       OS << "    <corrupt GNU_ABI_TAG>";
     else
       OS << "    OS: " << AbiTag.OSName << ", ABI: " << AbiTag.ABI;
     break;
   }
   case ELF::NT_GNU_BUILD_ID: {
-    OS << "    Build ID: " << getGNUBuildId<ELFT>(Words);
+    OS << "    Build ID: " << getGNUBuildId(Desc);
     break;
   }
   case ELF::NT_GNU_GOLD_VERSION:
-    OS << "    Version: " << getGNUGoldVersion<ELFT>(Words);
+    OS << "    Version: " << getGNUGoldVersion(Desc);
     break;
   case ELF::NT_GNU_PROPERTY_TYPE_0:
     OS << "    Properties:";
-    for (const auto &Property : getGNUPropertyList<ELFT>(Words))
+    for (const auto &Property : getGNUPropertyList<ELFT>(Desc))
       OS << "    " << Property << "\n";
     break;
   }
   OS << '\n';
 }
 
 struct AMDGPUNote {
   std::string type;
   std::string value;
 };
 
 template <typename ELFT>
 static AMDGPUNote getAMDGPUNote(uint32_t NoteType,
-                                ArrayRef<typename ELFT::Word> Words) {
+                                ArrayRef<uint8_t> Desc) {
   switch (NoteType) {
   default:
     return {"", ""};
   case ELF::NT_AMD_AMDGPU_HSA_METADATA:
     return {"HSA Metadata",
-            std::string(reinterpret_cast<const char *>(Words.data()),
-                        Words.size())};
+            std::string(reinterpret_cast<const char *>(Desc.data()),
+                        Desc.size())};
   case ELF::NT_AMD_AMDGPU_ISA:
     return {"ISA Version",
-            std::string(reinterpret_cast<const char *>(Words.data()),
-                        Words.size())};
+            std::string(reinterpret_cast<const char *>(Desc.data()),
+                        Desc.size())};
   case ELF::NT_AMD_AMDGPU_PAL_METADATA:
     const uint32_t *PALMetadataBegin =
-        reinterpret_cast<const uint32_t *>(Words.data());
-    const uint32_t *PALMetadataEnd = PALMetadataBegin + Words.size();
+        reinterpret_cast<const uint32_t *>(Desc.data());
+    const uint32_t *PALMetadataEnd = PALMetadataBegin + Desc.size();
     std::vector<uint32_t> PALMetadata(PALMetadataBegin, PALMetadataEnd);
     std::string PALMetadataString;
     auto Error = AMDGPU::PALMD::toString(PALMetadata, PALMetadataString);
     if (Error) {
       return {"PAL Metadata", "Invalid"};
     }
     return {"PAL Metadata", PALMetadataString};
   }
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printNotes(const ELFFile<ELFT> *Obj) {
   const Elf_Ehdr *e = Obj->getHeader();
   bool IsCore = e->e_type == ELF::ET_CORE;
 
   auto PrintHeader = [&](const typename ELFT::Off Offset,
                          const typename ELFT::Addr Size) {
     OS << "Displaying notes found at file offset " << format_hex(Offset, 10)
        << " with length " << format_hex(Size, 10) << ":\n"
        << "  Owner                 Data size\tDescription\n";
   };
 
   auto ProcessNote = [&](const Elf_Note &Note) {
     StringRef Name = Note.getName();
-    ArrayRef<Elf_Word> Descriptor = Note.getDesc();
+    ArrayRef<uint8_t> Descriptor = Note.getDesc();
     Elf_Word Type = Note.getType();
 
     OS << "  " << Name << std::string(22 - Name.size(), ' ')
        << format_hex(Descriptor.size(), 10) << '\t';
 
     if (Name == "GNU") {
       OS << getGNUNoteTypeName(Type) << '\n';
       printGNUNote<ELFT>(OS, Type, Descriptor);
while (Cur != End) {
     W.printNumber("Sym", this->dumper()->getStaticSymbolName(SymIndex),
                   SymIndex);
     Cur += Size;
   }
 }
 
 template <typename ELFT>
 static void printGNUNoteLLVMStyle(uint32_t NoteType,
-                                  ArrayRef<typename ELFT::Word> Words,
+                                  ArrayRef<uint8_t> Desc,
                                   ScopedPrinter &W) {
   switch (NoteType) {
   default:
     return;
   case ELF::NT_GNU_ABI_TAG: {
-    const GNUAbiTag &AbiTag = getGNUAbiTag<ELFT>(Words);
+    const GNUAbiTag &AbiTag = getGNUAbiTag<ELFT>(Desc);
     if (!AbiTag.IsValid) {
       W.printString("ABI", "<corrupt GNU_ABI_TAG>");
     } else {
       W.printString("OS", AbiTag.OSName);
       W.printString("ABI", AbiTag.ABI);
     }
     break;
   }
   case ELF::NT_GNU_BUILD_ID: {
-    W.printString("Build ID", getGNUBuildId<ELFT>(Words));
+    W.printString("Build ID", getGNUBuildId(Desc));
     break;
   }
   case ELF::NT_GNU_GOLD_VERSION:
-    W.printString("Version", getGNUGoldVersion<ELFT>(Words));
+    W.printString("Version", getGNUGoldVersion(Desc));
     break;
   case ELF::NT_GNU_PROPERTY_TYPE_0:
     ListScope D(W, "Property");
-    for (const auto &Property : getGNUPropertyList<ELFT>(Words))
+    for (const auto &Property : getGNUPropertyList<ELFT>(Desc))
       W.printString(Property);
     break;
   }
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printNotes(const ELFFile<ELFT> *Obj) {
   ListScope L(W, "Notes");
   const Elf_Ehdr *e = Obj->getHeader();
   bool IsCore = e->e_type == ELF::ET_CORE;
 
   auto PrintHeader = [&](const typename ELFT::Off Offset,
                          const typename ELFT::Addr Size) {
     W.printHex("Offset", Offset);
     W.printHex("Size", Size);
   };
 
   auto ProcessNote = [&](const Elf_Note &Note) {
     DictScope D2(W, "Note");
     StringRef Name = Note.getName();
-    ArrayRef<Elf_Word> Descriptor = Note.getDesc();
+    ArrayRef<uint8_t> Descriptor = Note.getDesc();
     Elf_Word Type = Note.getType();
 
     W.printString("Owner", Name);
     W.printHex("Data size", Descriptor.size());
     if (Name == "GNU") {
       W.printString("Type", getGNUNoteTypeName(Type));
       printGNUNoteLLVMStyle<ELFT>(Type, Descriptor, W);
     } else if (Name == "FreeBSD") {