[llvm-objcopy] Fix latent bug that allowed some Sections to be improperly cast to StringTableSections

[jakehehrlich]

Summary

If a Section had Type SHT_STRTAB (which could happen if you had a .dynstr section) it was possible to cast Section to StringTableSection and get away with any operation that was supported by SectionBase without it being noticed. This change makes this bug easier to notice and fixes it where it occurred. It also made me realize that there was some duplication of efforts in the loop that calls ::initialize. These issues are all fixed by this change.

[jhenderson]

So, from what I can see, it is no longer a requirement for a dynamic section (or indeed any "SectionWithStrTab" instance) to refer to a section of type SHT_STRTAB? Is this desirable, because it sounds a bit odd to me? I could support a check in SectionWithStrtab::initialize that checks the actual type of the string table. Otherwise, this change looks reasonable to me.

tools/llvm-objcopy/Object.h

125

As this class is now only usable for non-alloc string table sections, I think that we need to explicitly say so in the comment, along with the reason why.

[jakehehrlich]

In D38329#883062, @jhenderson wrote:

So, from what I can see, it is no longer a requirement for a dynamic section (or indeed any "SectionWithStrTab" instance) to refer to a section of type SHT_STRTAB? Is this desirable, because it sounds a bit odd to me? I could support a check in SectionWithStrtab::initialize that checks the actual type of the string table. Otherwise, this change looks reasonable to me.

I think a check is a good idea. In fact I thought I added that check; not sure why it didn't make it to review.

[jakehehrlich]

1. Added comment explaining why classof works the way it does.
2. Added check for SHT_STRTAB

I'm starting to notice a slightly non-trivial match up between classof and makeSection. Is there a way we could make this precise? Ideally I'd like to somehow contain the means of construction for a type within the class itself. I'd really like the following properties to be enforced by the library

1. You can't construct a StringTableSection that dosn't pass StringTableSection::classof
2. The checks for SHT_* and SHF_ALLOC shouldn't be duplicated in construction and classof. When one changes the other should change as well.

[jhenderson]

LGTM with typo fixes.

In D38329#887626, @jakehehrlich wrote:

I'm starting to notice a slightly non-trivial match up between classof and makeSection. Is there a way we could make this precise? Ideally I'd like to somehow contain the means of construction for a type within the class itself. I'd really like the following properties to be enforced by the library

1. You can't construct a StringTableSection that dosn't pass StringTableSection::classof
2. The checks for SHT_* and SHF_ALLOC shouldn't be duplicated in construction and classof. When one changes the other should change as well.

I agree with this, although I think it's a separate change. I need to think a bit more about what a good solution might be.

tools/llvm-objcopy/Object.h

119

addrsses -> addresses

124

the agrees -> then agrees

[jakehehrlich]

Fixed typos

tools/llvm-objcopy/Object.h

 private:
   llvm::ArrayRef<uint8_t> Contents;
 
 public:
   Section(llvm::ArrayRef<uint8_t> Data) : Contents(Data) {}
   void writeSection(llvm::FileOutputBuffer &Out) const override;
 };
 
-// This is just a wraper around a StringTableBuilder that implements SectionBase
+// There are two types of string tables that can exist, dynamic and not dynamic.
+// In the dynamic case the string table is allocated. Changing a dynamic string
+// table would mean altering virtual addresses and thus the memory image. So
+// dynamic string tables should not have an interface to modify them or
+// reconstruct them. This type lets us reconstruct a string table. To avoid
+// this class being used for dynamic string tables (which has happened) the
+// classof method checks that the particular instance is not allocated. This
+// then agrees with the makeSection method used to construct most sections.
 class StringTableSection : public SectionBase {
 private:
   llvm::StringTableBuilder StrTabBuilder;
 
 public:
   StringTableSection() : StrTabBuilder(llvm::StringTableBuilder::ELF) {
     Type = llvm::ELF::SHT_STRTAB;
   }
 
   void addString(llvm::StringRef Name);
   uint32_t findIndex(llvm::StringRef Name) const;
   void finalize() override;
   void writeSection(llvm::FileOutputBuffer &Out) const override;
   static bool classof(const SectionBase *S) {
+    if (S->Flags & llvm::ELF::SHF_ALLOC)
+      return false;
     return S->Type == llvm::ELF::SHT_STRTAB;
   }
 };
 
 // Symbols have a st_shndx field that normally stores an index but occasionally
 // stores a different special value. This enum keeps track of what the st_shndx
 // field means. Most of the values are just copies of the special SHN_* values.
 // SYMBOL_SIMPLE_INDEX means that the st_shndx is just an index of a section.
static bool classof(const SectionBase *S) {
     if (S->Flags & llvm::ELF::SHF_ALLOC)
       return false;
     return S->Type == llvm::ELF::SHT_REL || S->Type == llvm::ELF::SHT_RELA;
   }
 };
 
 class SectionWithStrTab : public Section {
 private:
-  StringTableSection *StrTab = nullptr;
+  const SectionBase *StrTab = nullptr;
 
 public:
   SectionWithStrTab(llvm::ArrayRef<uint8_t> Data) : Section(Data) {}
-  void setStrTab(StringTableSection *StringTable) { StrTab = StringTable; }
+  void setStrTab(const SectionBase *StringTable) { StrTab = StringTable; }
   void removeSectionReferences(const SectionBase *Sec) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   static bool classof(const SectionBase *S);
 };
 
 class DynamicSymbolTableSection : public SectionWithStrTab {
 public:

tools/llvm-objcopy/Object.cpp

void SectionWithStrTab::removeSectionReferences(const SectionBase *Sec) {
   }
 }
 
 bool SectionWithStrTab::classof(const SectionBase *S) {
   return isa<DynamicSymbolTableSection>(S) || isa<DynamicSection>(S);
 }
 
 void SectionWithStrTab::initialize(SectionTableRef SecTable) {
-  setStrTab(SecTable.getSectionOfType<StringTableSection>(
-      Link,
-      "Link field value " + Twine(Link) + " in section " + Name + " is invalid",
-      "Link field value " + Twine(Link) + " in section " + Name +
-          " is not a string table"));
+  auto StrTab = SecTable.getSection(Link,
+                                    "Link field value " + Twine(Link) +
+                                        " in section " + Name + " is invalid");
+  if (StrTab->Type != SHT_STRTAB) {
+    error("Link field value " + Twine(Link) + " in section " + Name +
+          " is not a string table");
+  }
+  setStrTab(StrTab);
 }
 
 void SectionWithStrTab::finalize() { this->Link = StrTab->Index; }
 
 // Returns true IFF a section is wholly inside the range of a segment
 static bool sectionWithinSegment(const SectionBase &Section,
                                  const Segment &Segment) {
   // If a section is empty it should be treated like it has a size of 1. This is
for (auto &Section : Sections) {
     if (auto RelSec = dyn_cast<RelocationSection<ELFT>>(Section.get())) {
       auto Shdr = unwrapOrError(ElfFile.sections()).begin() + RelSec->Index;
       if (RelSec->Type == SHT_REL)
         initRelocations(RelSec, SymbolTable, unwrapOrError(ElfFile.rels(Shdr)));
       else
         initRelocations(RelSec, SymbolTable,
                         unwrapOrError(ElfFile.relas(Shdr)));
     }
-
-    if (auto Sec = dyn_cast<SectionWithStrTab>(Section.get())) {
-      Sec->setStrTab(SecTable.getSectionOfType<StringTableSection>(
-          Sec->Link,
-          "Link field value " + Twine(Sec->Link) + " in section " + Sec->Name +
-              " is invalid",
-          "Link field value " + Twine(Sec->Link) + " in section " + Sec->Name +
-              " is not a string table"));
-    }
   }
 
   return SecTable;
 }
 
 template <class ELFT> Object<ELFT>::Object(const ELFObjectFile<ELFT> &Obj) {
   const auto &ElfFile = *Obj.getELFFile();
   const auto &Ehdr = *ElfFile.getHeader();