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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich updated this revision to Diff 117592.
jakehehrlich added a comment.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D38329

Files:
  tools/llvm-objcopy/Object.cpp
  tools/llvm-objcopy/Object.h


Index: tools/llvm-objcopy/Object.h
===================================================================
--- tools/llvm-objcopy/Object.h
+++ tools/llvm-objcopy/Object.h
@@ -114,7 +114,14 @@
   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 addrsses 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
+// the agrees with the makeSection method used to construct most sections.
 class StringTableSection : public SectionBase {
 private:
   llvm::StringTableBuilder StrTabBuilder;
@@ -129,6 +136,8 @@
   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 @@
 
 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;
Index: tools/llvm-objcopy/Object.cpp
===================================================================
--- tools/llvm-objcopy/Object.cpp
+++ tools/llvm-objcopy/Object.cpp
@@ -289,11 +289,14 @@
 }
 
 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 @@
         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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38329.117592.patch
Type: text/x-patch
Size: 3420 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171003/a3cd5af7/attachment.bin>