From: Jake Ehrlich <jakehehrlich@google.com>
Date: Tue, 10 Oct 2017 21:28:22 +0000 (+0000)
Subject: [llvm-objcopy] Fix latent bug that allowed some Sections to be improperly cast to... 
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f2c4679069ec6de63a71bcedb163b48134d009c8;p=llvm

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

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.

Differential Revision: https://reviews.llvm.org/D38329

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@315372 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/tools/llvm-objcopy/Object.cpp b/tools/llvm-objcopy/Object.cpp
index bb79e4269d4..2bd3aa16f0b 100644
--- a/tools/llvm-objcopy/Object.cpp
+++ b/tools/llvm-objcopy/Object.cpp
@@ -289,11 +289,14 @@ bool SectionWithStrTab::classof(const SectionBase *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; }
@@ -535,15 +538,6 @@ SectionTableRef Object<ELFT>::readSectionHeaders(const ELFFile<ELFT> &ElfFile) {
         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;
@@ -621,7 +615,7 @@ void Object<ELFT>::writeSectionHeaders(FileOutputBuffer &Out) const {
 template <class ELFT>
 void Object<ELFT>::writeSectionData(FileOutputBuffer &Out) const {
   for (auto &Section : Sections)
-    Section->writeSection(Out);
+      Section->writeSection(Out);
 }
 
 template <class ELFT>
diff --git a/tools/llvm-objcopy/Object.h b/tools/llvm-objcopy/Object.h
index 3def0930a94..391465db65e 100644
--- a/tools/llvm-objcopy/Object.h
+++ b/tools/llvm-objcopy/Object.h
@@ -114,7 +114,14 @@ public:
   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;
@@ -129,6 +136,8 @@ public:
   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;
   }
 };
@@ -259,11 +268,11 @@ public:
 
 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;