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

[Jake Ehrlich via llvm-commits]

Author: jakehehrlich
Date: Tue Oct 10 14:28:22 2017
New Revision: 315372

URL: http://llvm.org/viewvc/llvm-project?rev=315372&view=rev
Log:
[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

Modified:
    llvm/trunk/tools/llvm-objcopy/Object.cpp
    llvm/trunk/tools/llvm-objcopy/Object.h

Modified: llvm/trunk/tools/llvm-objcopy/Object.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/Object.cpp?rev=315372&r1=315371&r2=315372&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/Object.cpp (original)
+++ llvm/trunk/tools/llvm-objcopy/Object.cpp Tue Oct 10 14:28:22 2017
@@ -289,11 +289,14 @@ bool SectionWithStrTab::classof(const Se
 }
 
 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>::readSectio
         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(F
 template <class ELFT>
 void Object<ELFT>::writeSectionData(FileOutputBuffer &Out) const {
   for (auto &Section : Sections)
-    Section->writeSection(Out);
+      Section->writeSection(Out);
 }
 
 template <class ELFT>

Modified: llvm/trunk/tools/llvm-objcopy/Object.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/Object.h?rev=315372&r1=315371&r2=315372&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/Object.h (original)
+++ llvm/trunk/tools/llvm-objcopy/Object.h Tue Oct 10 14:28:22 2017
@@ -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;