[llvm-objcopy] - Check dynamic relocation sections for broken references.

[grimar]

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=41371.

Currently, it is possible to break the sh_link field of the dynamic relocation section
by removing the section it refers to. The patch fixes an issue and adds 2 test cases.

One of them shows that it does not seem possible to break the sh_info field.
That is explained in the comments. I added an assert to verify this.

[grimar]

test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shlink-reference.test

24 ↗

(On Diff #195563)

This TODO can be removed if we land D60820.

[jakehehrlich]

So overall this looks good and literally yesterday I'd have just given a +2 but James noticed an edge case we should think about. If there are ever reasons to have a dynamic relocation without a symbol table (I don't know of any) we should hold off and account for the edge case. O think that concern only applies to relocatable files however.

[grimar]

In D60825#1470322, @jakehehrlich wrote:

So overall this looks good and literally yesterday I'd have just given a +2 but James noticed an edge case we should think about. If there are ever reasons to have a dynamic relocation without a symbol table (I don't know of any) we should hold off and account for the edge case. O think that concern only applies to relocatable files however.

In theory, I think R_X86_64_RELATIVE relocations should not need a symbol table since they just adjust a place by program base.
This will be easy to support after landing the D60324 which introduce the --allow-broken-links option.
We can just wait for it and I'll update this patch then or I could do that in a follow-up.

[jhenderson]

FYI, I've just landed D60324, and approved D60820. This looks good, except for the edge case mentioned by @jakehehrlich. The code should also implement --allow-broken-links now that that has landed.

test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shinfo-reference.test

11 ↗

(On Diff #195563)

I'm not too bothered, but I find it marginally easier to read the line as a continuation if the '|' is on the second line.

[grimar]

• Addressed comments, rebased, updated the test case.

[grimar]

Ping.

[jakehehrlich]

Hmm...right now we don't parse the dynamic symbol table but parsing it just enough to check if no symbols are needed and then not throwing an error if no symbol table is needed seems like a better option than using that flag. That flag should of course also be respected here however.

[jakehehrlich]

Maybe we should land this and wait for someone to have that issue. Can someone file a bug about that issue now?

[grimar]

In D60825#1479436, @jakehehrlich wrote:

Maybe we should land this and wait for someone to have that issue. Can someone file a bug about that issue now?

Right now we already have a bug saying that "If you strip a section that is referred to by a dynamic relocation section (e.g. the .dynsym),
llvm-objcopy lets it happen assuming that there are no other references".
(https://bugs.llvm.org/show_bug.cgi?id=41371)

As far I understand, after changing the behavior, we might want to post a bug saying something like the following, right?
"Currently when --allow-broken-links is *not* given we always report a error when removing a symbol table used by a dynamic relocation section.
Though it should be possible to do that only in a case when we know that using symbols from dynamic symbol table."

And then we need to land this patch first (as PR41371 already covers the current behavior I think?).

[jhenderson]

LGTM. I agree that inspecting the dynamic relocations to see if there are any references to symbols is a separate issue. Indeed, it technically also applies to regular relocations, not just dynamic ones. There is a potential performance issue here though, which I think might be a problem if we tried to pass the relocations to do this.

test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shlink-reference.test

12 ↗

(On Diff #195718)

Either "with --allow-broken-links" or "with the --allow-broken-links switch"

[grimar]

I submitted a bug discussed here: https://bugs.llvm.org/show_bug.cgi?id=41669

llvm/trunk/test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shinfo-reference.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -R .got.plt %t %t2
+
+## .rela.plt is a dynamic relocation section that has a connection
+## via sh_info field with its target section .got.plt.
+## Here we check that if the target section is removed then dynamic
+## relocation section is also removed and we do not end up with a broken
+## sh_info value, for example.
+
+# RUN: llvm-readelf --sections %t2 \
+# RUN:  | FileCheck %s --implicit-check-not=".got.plt" --implicit-check-not=".rela.plt"
+
+--- !ELF
+FileHeader:
+  Class:          ELFCLASS64
+  Data:           ELFDATA2LSB
+  Type:           ET_DYN
+  Machine:        EM_X86_64
+Sections:
+  - Name:         .rela.plt
+    Type:         SHT_RELA
+    Flags:        [ SHF_ALLOC ]
+    Link:         .dynsym
+    Info:         .got.plt
+  - Name:         .got.plt
+    Type:         SHT_PROGBITS
+    Flags:        [ SHF_WRITE, SHF_ALLOC ]
+DynamicSymbols:
+  - Name:          bar
+    Binding:       STB_GLOBAL

llvm/trunk/test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shlink-reference.test

+# RUN: yaml2obj %s > %t
+
+## Check we cannot remove the .dynsym symbol table because dynamic
+## relocation section .rela.dyn still references it via sh_link field.
+# RUN: not llvm-objcopy -R .dynsym %t %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# ERR: error: Symbol table .dynsym cannot be removed because it is referenced by the relocation section .rela.dyn.
+
+## Check we can remove .dynsym after removing the reference.
+# RUN: llvm-objcopy -R .dynsym -R .rela.dyn %t %t2
+# RUN: llvm-readelf --sections %t2 | FileCheck %s --implicit-check-not=".dynsym"
+
+## Check we zero out sh_link field and allow producing output with the --allow-broken-links switch.
+# RUN: llvm-objcopy -R .dynsym --allow-broken-links %t %t2
+# RUN: llvm-readelf --sections %t2 | FileCheck %s --check-prefix=DROP-LINK
+# DROP-LINK:     [Nr] Name      Type Address          Off    Size   ES   Flg L
+# DROP-LINK:     [ 1] .rela.dyn RELA 0000000000000270 000040 000000 18   A   0
+# DROP-LINK-NOT: .dynsym
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_DYN
+  Machine:         EM_X86_64
+Sections:
+  - Name:          .rela.dyn
+    Type:          SHT_RELA
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x0000000000000270
+    Link:          .dynsym
+    EntSize:       0x0000000000000018
+DynamicSymbols:
+  - Name:          bar
+    Binding:       STB_GLOBAL

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

class DynamicRelocationSection
     : public RelocSectionWithSymtabBase<DynamicSymbolTableSection> {
   MAKE_SEC_WRITER_FRIEND
 
 private:
   ArrayRef<uint8_t> Contents;
 
 public:
   explicit DynamicRelocationSection(ArrayRef<uint8_t> Data) : Contents(Data) {}
-
-  void accept(SectionVisitor &) const override;
-  void accept(MutableSectionVisitor &Visitor) override;
-
-  static bool classof(const SectionBase *S) {
-    if (!(S->Flags & ELF::SHF_ALLOC))
+
+  void accept(SectionVisitor &) const override;
+  void accept(MutableSectionVisitor &Visitor) override;
+  Error removeSectionReferences(
+      bool AllowBrokenLinks,
+      function_ref<bool(const SectionBase *)> ToRemove) override;
+
+  static bool classof(const SectionBase *S) {
+    if (!(S->Flags & ELF::SHF_ALLOC))
       return false;
     return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
   }
 };
 
 class GnuDebugLinkSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 

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

llvm::copy(Sec.Contents,
             Out.getBufferStart() + Sec.Offset);
 }
 
 void DynamicRelocationSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
 void DynamicRelocationSection::accept(MutableSectionVisitor &Visitor) {
-  Visitor.visit(*this);
-}
-
-Error Section::removeSectionReferences(bool AllowBrokenDependency,
-    function_ref<bool(const SectionBase *)> ToRemove) {
-  if (ToRemove(LinkSection)) {
+  Visitor.visit(*this);
+}
+
+Error DynamicRelocationSection::removeSectionReferences(
+    bool AllowBrokenLinks, function_ref<bool(const SectionBase *)> ToRemove) {
+  if (ToRemove(Symbols)) {
+    if (!AllowBrokenLinks)
+      return createStringError(
+          llvm::errc::invalid_argument,
+          "Symbol table %s cannot be removed because it is "
+          "referenced by the relocation section %s.",
+          Symbols->Name.data(), this->Name.data());
+    Symbols = nullptr;
+  }
+
+  // SecToApplyRel contains a section referenced by sh_info field. It keeps
+  // a section to which the relocation section applies. When we remove any
+  // sections we also remove their relocation sections. Since we do that much
+  // earlier, this assert should never be triggered.
+  assert(!SecToApplyRel || !ToRemove(SecToApplyRel));
+
+  return Error::success();
+}
+
+Error Section::removeSectionReferences(bool AllowBrokenDependency,
+    function_ref<bool(const SectionBase *)> ToRemove) {
+  if (ToRemove(LinkSection)) {
     if (!AllowBrokenDependency)
       return createStringError(llvm::errc::invalid_argument,
                                "Section %s cannot be removed because it is "
                                "referenced by the section %s",
                                LinkSection->Name.data(), this->Name.data());
     LinkSection = nullptr;
   }
   return Error::success();