[llvm-objcopy] Fix crash when removing symbol table at same time as adding a symbol

[jubnzv]

Summary

This patch resolves crash that occurs when user wanted to remove all
symbols and add a brand new one using:

llvm-objcopy -R .symtab --add-symbol foo=1234 in.o out.o

Before these changes the symbol table internally being null when adding
new symbols. For now we will regenerate symtab in this case.

This fixes: https://bugs.llvm.org/show_bug.cgi?id=43930

[jhenderson]

I don't see a benefit in having two different places where the symbol table can be created. I think you probably just want to completely remove the logic from ELFBuilder for creating a symbol table. With a bit of care, you then should be able to create the symbol table when you need to rather than early, and at the same time remove the whole EnsureSymtab logic from the ELFBuilder/ELFReader stack. You'll need to see where things reference the symbol table though to make sure there's no risk of links not being set up properly.

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

111

You shouldn't need --allow-broken-links.

%t has no symbols in it. I think this test case would be better off using an input file that does contain symbols. I'd recommend splitting this case off into a new file and adding your own YAML with one or more symbols in, showing that the old symbols have been deleted and that new ones have been added. You'll want to show that both the new symbols and the new symtab section header are correct too.

113

Nit: The norm is to not put comment markers on blank line.

[jubnzv]

Thanks for the comment @jhenderson. I updated the test.

I think you probably just want to completely remove the logic from ELFBuilder for creating a symbol table. With a bit of care, you then should be able to create the symbol table when you need to rather than early, and at the same time remove the whole EnsureSymtab logic from the ELFBuilder/ELFReader stack.

It is possible to move logic for creating symbol table in a free function or make method in the Object class that performs initialization on demand.
But the problem is that ELFBuilder also sets reference symbols for SH_LINK and SH_INFO fields of relocation sections when creating a new Object. To perform this, it needs an initialized symbol table. All cases that requires symtab are better seen at D69093 diff: https://reviews.llvm.org/differential/changeset/?ref=1658725.
So I will also need to separate set relocations logic and apply it only when it needed, after creating an Object. I think it makes code more hard to read.
Or are simpler solutions possible?

[jhenderson]

In D82935#2125472, @jubnzv wrote:

Thanks for the comment @jhenderson. I updated the test.

I think you probably just want to completely remove the logic from ELFBuilder for creating a symbol table. With a bit of care, you then should be able to create the symbol table when you need to rather than early, and at the same time remove the whole EnsureSymtab logic from the ELFBuilder/ELFReader stack.

It is possible to move logic for creating symbol table in a free function or make method in the Object class that performs initialization on demand.
But the problem is that ELFBuilder also sets reference symbols for SH_LINK and SH_INFO fields of relocation sections when creating a new Object. To perform this, it needs an initialized symbol table. All cases that requires symtab are better seen at D69093 diff: https://reviews.llvm.org/differential/changeset/?ref=1658725.
So I will also need to separate set relocations logic and apply it only when it needed, after creating an Object. I think it makes code more hard to read.
Or are simpler solutions possible?

Actually, does the createSymbolTable function even need to be a member of ELFBuilder? My main objection is the use of the builder a long time after initialization of the Object (along with the complexity of calling it due to the template), but I don't think you actually need it.

[jubnzv]

Actually, does the createSymbolTable function even need to be a member of ELFBuilder? My main objection is the use of the builder a long time after initialization of the Object (along with the complexity of calling it due to the template), but I don't think you actually need it.

Yes, I agree, this template call looked clumsily. I wasn't sure which class in the hierarchy should be responsible for symtab creation.
I think we can define a method in the Object itself.

[jhenderson]

Nearly there, just a few small changes to make now, I think.

llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test

1–2 ↗

(On Diff #275294)

Perhaps this comment could be simplified slightly:

"Show that llvm-objcopy can replace the symbol table with a new one."

4 ↗

(On Diff #275294)

In general, I find tests easier to follow when related RUN lines are kept together. Thus I'd go:

# RUN: yaml2obj ...
# RUN: llvm-objcopy ...
# RUN: llvm-readelf ... | FileCheck %s

# CHECK: ...

<YAML>

36 ↗

(On Diff #275294)

I don't think you need this llvm-readelf line, or the corresponding checks, since it is just verifying yaml2obj does the right thing, but we can assume it does, since there is testing for that in the yaml2obj testing.

49 ↗

(On Diff #275294)

I assume you are using it to show the sh_link value of the new symbol table is correct? If so, you a) can add -S to the llvm-readelf call above and b) just use the same check-prefix for everything (CHECK is fine, since you will only have the one FileCheck command).

57 ↗

(On Diff #275294)

Aside from the fact that the checks and commands are not in a sensible order, you don't need two separate commands, as I note in my previous comment.

llvm/tools/llvm-objcopy/ELF/Object.cpp

1909

I think you want something, probably an assert, that prevents this function being called if there already is a symbol table in the Object.

1911

Whilst you're moving this code, please change this to not use auto. I was a bit lax earlier in my reviewing of this code when it was all first written, and I find auto in this context makes it harder to figure out exactly what sort of section class Sec actually is.

llvm/tools/llvm-objcopy/ELF/Object.h

1043

I'd call the new function addSymbolTable (or possibly addNewSymbolTable and put it next to the addSection function in the header and code as conceptually, the two are related.

[jubnzv]

Thanks. Fixed.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test

44 ↗

(On Diff #275333)

Since this is a new section, I'd check the values are all correct, rather than skipping them. Don't forget the alignment column!

4 ↗

(On Diff #275294)

You haven't moved the YAML to the end, as I asked...

llvm/tools/llvm-objcopy/ELF/Object.cpp

1909

Add a && "some explanatory message to this assert.

[jubnzv]

llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test

44 ↗

(On Diff #275333)

I agree. I think it will be more readable if I add values for the other columns as well.
The other tests for llvm-objcopy are written in the same way.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test

44 ↗

(On Diff #275333)

It's not just about readability, it's about correctness. You could have added a new section called .symtab, with invalid properties easily (e.g. incorrect type, or a non-zero info field, for example). Without the explicit value checks, this would fail.

One exception is that you don't need to check the Offset field. The reason for that is because if we changed something internal in llvm-objcopy that changed the position the symbol table is put in the file, it would cause this test to break, but the Offset isn't actually something you control by simply adding the section, plus the ability to dump the symbol table validates that as a side-effect.

[jhenderson]

LGTM, with one suggestion.

If you haven't already, you should be able to request commit access, as outlined in the docs.

llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test

6 ↗

(On Diff #275367)

Add --match-full-lines to the FileCheck call. This will prevent there being spurious data at the end of the line. For example, if a section's alignment were 16, a check of 1 would still pass, because FileCheck only does partial matches by default.

[jubnzv]

llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test

44 ↗

(On Diff #275333)

Understood, thanks for the explanation.
Interestingly, that the offset value generated by objcopy --add-section from GNU binutils and llvm-objcopy differs due to different internal implementation.

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

FileHeader:
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 DynamicSymbols:
   - Name:    dummy
     Binding: STB_GLOBAL
 Symbols: []
 ...
+
+## Show that llvm-objcopy can handle a case when we wanted to remove all symbols
+## and add a brand new one.
+# RUN: llvm-objcopy --allow-broken-links -R .symtab --add-symbol foo=1234 %t %t2
+# RUN: llvm-readelf --sections %t2 | FileCheck %s --check-prefix=SECTIONS
+#
+# SECTIONS: There are 4 section headers
+# SECTIONS:      [Nr] Name      Type   Address Off    Size   ES     Flg Lk Inf
+# SECTIONS-NEXT: [ 0] NULL
+# SECTIONS-NEXT: [ 1] .strtab
+# SECTIONS-NEXT: [ 2] .shstrtab
+# SECTIONS-NEXT: [ 3] .symtab   {{.+}} {{.+}} {{.+}} {{.+}} {{.+}}      1  {{.+}}

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

for (const auto &Flag : Config.AddSection) {
     if (SecName.startswith(".note") && SecName != ".note.GNU-stack")
       NewSection.Type = SHT_NOTE;
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
                                         Config.GnuDebugLinkCRC32);
 
+  // If the symbol table was previously removed, we need to create a new one
+  // before adding new symbols.
+  if (!Obj.SymbolTable && !Config.ELF->SymbolsToAdd.empty()) {
+    switch (OutputElfType) {
+    case ELFT_ELF32LE:
+      ELFBuilder<ELF32LE>::createSymbolTable(&Obj);
+      break;
+    case ELFT_ELF64LE:
+      ELFBuilder<ELF64LE>::createSymbolTable(&Obj);
+      break;
+    case ELFT_ELF32BE:
+      ELFBuilder<ELF32BE>::createSymbolTable(&Obj);
+      break;
+    case ELFT_ELF64BE:
+      ELFBuilder<ELF64BE>::createSymbolTable(&Obj);
+      break;
+    }
+  }
+
   for (const NewSymbolInfo &SI : Config.ELF->SymbolsToAdd) {
     SectionBase *Sec = Obj.findSection(SI.SectionName);
     uint64_t Value = Sec ? Sec->Addr + SI.Value : SI.Value;
     Obj.SymbolTable->addSymbol(
         SI.SymbolName, SI.Bind, SI.Type, Sec, Value, SI.Visibility,
         Sec ? (uint16_t)SYMBOL_SIMPLE_INDEX : (uint16_t)SHN_ABS, 0);
   }
 

llvm/tools/llvm-objcopy/ELF/Object.h

 
 public:
   ELFBuilder(const ELFObjectFile<ELFT> &ElfObj, Object &Obj,
              Optional<StringRef> ExtractPartition)
       : ElfFile(*ElfObj.getELFFile()), Obj(Obj),
         ExtractPartition(ExtractPartition) {}
 
   void build(bool EnsureSymtab);
+  static void createSymbolTable(Object *Obj);
 };
 
 class BinaryReader : public Reader {
   MemoryBuffer *MemBuf;
   uint8_t NewSymbolVisibility;
 
 public:
   BinaryReader(MemoryBuffer *MB, const uint8_t NewSymbolVisibility)
public:
   uint32_t Flags;
 
   bool HadShdrs = true;
   bool MustBeRelocatable = false;
   StringTableSection *SectionNames = nullptr;
   SymbolTableSection *SymbolTable = nullptr;
   SectionIndexSection *SectionIndexTable = nullptr;
 
   void sortSections();
   SectionTableRef sections() { return SectionTableRef(Sections); }
   ConstRange<SectionBase> sections() const {
     return make_pointee_range(Sections);
   }
   iterator_range<
       filter_iterator<pointee_iterator<std::vector<SecPtr>::const_iterator>,
                       decltype(&sectionIsAlloc)>>
   allocSections() const {

llvm/tools/llvm-objcopy/ELF/Object.cpp

if (Sym.st_shndx == SHN_XINDEX) {
                             Twine(Sym.st_shndx));
     }
 
     SymTab->addSymbol(Name, Sym.getBinding(), Sym.getType(), DefSection,
                       Sym.getValue(), Sym.st_other, Sym.st_shndx, Sym.st_size);
   }
 }
 
+template <class ELFT> void ELFBuilder<ELFT>::createSymbolTable(Object *Obj) {
+  // Reuse an existing SHT_STRTAB section if it exists.
+  StringTableSection *StrTab = nullptr;
+  for (auto &Sec : Obj->sections()) {
+    if (Sec.Type == ELF::SHT_STRTAB && !(Sec.Flags & SHF_ALLOC)) {
+      StrTab = static_cast<StringTableSection *>(&Sec);
+
+      // Prefer a string table that is not the section header string table, if
+      // such a table exists.
+      if (Obj->SectionNames != &Sec)
+        break;
+    }
+  }
+  if (!StrTab)
+    StrTab = &Obj->addSection<StringTableSection>();
+
+  SymbolTableSection &SymTab = Obj->addSection<SymbolTableSection>();
+  SymTab.Name = ".symtab";
+  SymTab.Link = StrTab->Index;
+  SymTab.initialize(Obj->sections());
+  SymTab.addSymbol("", 0, 0, nullptr, 0, 0, 0, 0);
+
+  Obj->SymbolTable = &SymTab;
+}
+
 template <class ELFT>
 static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, false> &Rel) {}
 
 template <class ELFT>
 static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, true> &Rela) {
   ToSet = Rela.r_addend;
 }
 
template <class ELFT> void ELFBuilder<ELFT>::readSections(bool EnsureSymtab) {
 
   // Now that all of the sections have been added we can fill out some extra
   // details about symbol tables. We need the symbol table filled out before
   // any relocations.
   if (Obj.SymbolTable) {
     Obj.SymbolTable->initialize(Obj.sections());
     initSymbolTable(Obj.SymbolTable);
   } else if (EnsureSymtab) {
-    // Reuse an existing SHT_STRTAB section if it exists.
-    StringTableSection *StrTab = nullptr;
-    for (auto &Sec : Obj.sections()) {
-      if (Sec.Type == ELF::SHT_STRTAB && !(Sec.Flags & SHF_ALLOC)) {
-        StrTab = static_cast<StringTableSection *>(&Sec);
-
-        // Prefer a string table that is not the section header string table, if
-        // such a table exists.
-        if (Obj.SectionNames != &Sec)
-          break;
-      }
-    }
-    if (!StrTab)
-      StrTab = &Obj.addSection<StringTableSection>();
-
-    SymbolTableSection &SymTab = Obj.addSection<SymbolTableSection>();
-    SymTab.Name = ".symtab";
-    SymTab.Link = StrTab->Index;
-    SymTab.initialize(Obj.sections());
-    SymTab.addSymbol("", 0, 0, nullptr, 0, 0, 0, 0);
-    Obj.SymbolTable = &SymTab;
+    createSymbolTable(&Obj);
   }
 
   // Now that all sections and symbols have been added we can add
   // relocations that reference symbols and set the link and info fields for
   // relocation sections.
   for (auto &Sec : Obj.sections()) {
     if (&Sec == Obj.SymbolTable)
       continue;
Error Object::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   if (SymbolTable)
     for (const SecPtr &Sec : Sections)
       if (Error E = Sec->removeSymbols(ToRemove))
         return E;
   return Error::success();
 }
 
 void Object::sortSections() {
   // Use stable_sort to maintain the original ordering as closely as possible.
   llvm::stable_sort(Sections, [](const SecPtr &A, const SecPtr &B) {
     // Put SHT_GROUP sections first, since group section headers must come
     // before the sections they contain. This also matches what GNU objcopy
     // does.
     if (A->Type != B->Type &&
         (A->Type == ELF::SHT_GROUP || B->Type == ELF::SHT_GROUP))
       return A->Type == ELF::SHT_GROUP;
     // For all other sections, sort by offset order.
     return A->OriginalOffset < B->OriginalOffset;
   });