[llvm-objcopy] Remove references for empty section groups

[pranavk]

Otherwise, llvm-objcopy fails with use-after-free when built under sanitizers. Simple repro: run the test ELF/remove-section-in-group.test under asan. This is due to symbol table references to empty section groups that must be removed.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: Pranav Kant (pranavk)

Changes

Otherwise, llvm-objcopy fails with use-after-free when built under sanitizers. Simple repro can be running the test ELF/remove-section-in-group.test under asan. This is due to symbol table references to empty section groups.

---

Full diff: https://github.com/llvm/llvm-project/pull/98106.diff

2 Files Affected:

• (modified) llvm/lib/ObjCopy/ELF/ELFObject.cpp (+5-8)
• (modified) llvm/lib/ObjCopy/ELF/ELFObject.h (+6-1)

diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.cpp b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
index f9c5d2579be69..d222aa4f2cf3d 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
@@ -2203,6 +2203,11 @@ Error Object::removeSections(
           if (auto ToRelSec = RelSec->getSection())
             return !ToRemove(*ToRelSec);
         }
+        // Remove empty group sections.
+        if (Sec->Type == ELF::SHT_GROUP) {
+          auto GroupSec = cast<GroupSection>(Sec.get());
+          return !llvm::all_of(GroupSec->members(), ToRemove);
+        }
         return true;
       });
   if (SymbolTable != nullptr && ToRemove(*SymbolTable))
@@ -2242,14 +2247,6 @@ Error Object::removeSections(
   // Now get rid of them altogether.
   Sections.erase(Iter, std::end(Sections));
 
-  // Finally erase empty SHT_GROUP sections.
-  llvm::erase_if(Sections, [](const SecPtr &Sec) {
-    if (auto GroupSec = dyn_cast<GroupSection>(Sec.get()))
-      return GroupSec->getMembersCount() == 0;
-
-    return false;
-  });
-
   return Error::success();
 }
 
diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.h b/llvm/lib/ObjCopy/ELF/ELFObject.h
index 2a9f337c3f323..fd119cabcc92b 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.h
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.h
@@ -941,6 +941,9 @@ class GroupSection : public SectionBase {
   SmallVector<SectionBase *, 3> GroupMembers;
 
 public:
+  template <class T>
+  using ConstRange = iterator_range<pointee_iterator<
+      typename llvm::SmallVector<T *, 3>::const_iterator>>;
   // TODO: Contents is present in several classes of the hierarchy.
   // This needs to be refactored to avoid duplication.
   ArrayRef<uint8_t> Contents;
@@ -964,7 +967,9 @@ class GroupSection : public SectionBase {
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
   void onRemove() override;
 
-  size_t getMembersCount() const { return GroupMembers.size(); }
+  ConstRange<SectionBase> members() const {
+    return make_pointee_range(GroupMembers);
+  }
 
   static bool classof(const SectionBase *S) {
     return S->OriginalType == ELF::SHT_GROUP;

[pranavk]

Earlier fix was in #97141

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[hokein]

cc @chestnykh

[MaskRay]

Thanks! I think the approach is fine, but @jh7370 and I might take more time to think about it.
I am reverting the original patch instead.

[chestnykh]

Sorry for this, i haven't tested my changes with asan

[jh7370]

I think this looks good to me. It's the "proper" way of marking something for removal.

[MaskRay]

Since #97141 has been reverted, this now needs a rebase and restoring the original test file:)

[MaskRay]

Sorry for this, i haven't tested my changes with asan

No worries. This case was difficult to analyze and we don't have asan premerge bots (the existing bots are underwhelmed even without asan).

You can test asan with -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_USE_SANITIZER=Address. Since it is slow, perhaps test it with targeted testing check-llvm-tools-llvm-objcopy.

[pranavk]

Rebased

[pranavk]

Agree with Fangrui. Pretty hard to figure out with naked eye. We have internal tests that run everything with Asan enabled. So that's how I figured this out :)

Once you have sanitizer output, it's easy to figure out what went wrong.