[PATCH] D154776: IFSStub destructor should be virtual

[Amirreza Ashouri via Phabricator via llvm-commits]

AMP999 created this revision.
AMP999 added a reviewer: haowei.
AMP999 created this object with edit policy "Administrators".
AMP999 added a project: libc++.
Herald added a project: All.
AMP999 requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

The problem arises when a pointer of the polymorphic base class IFSStub, pointing to its child class's object, gets deleted according to this example:

  IFSStub *p = new IFSStubTriple;
  delete p; //Undefined Behavior, fails to destroy the most derived object

This problem in the source code is disguised in `llvm/lib/InterfaceStub/IFSStub.cpp` when `std::unique_ptr<IFSStubTriple>` implicitly converted to `std::unique_ptr<IFSStub>` as the return type of member function `ifs::readIFSFromBuffer` which happens at:

  llvm/lib/InterfaceStub/IFSHandler.cpp:204:10:
        return std::move(Stub);
               ^~~~~~~~~~~~~~~

when destructor of `unique_ptr<IFSStubTriple>` is called everything is okay, since it correctly calls the destructor of `IFSStubTriple` but after the implicit conversion destructor of `unique_ptr<IFSStub>` incorrectly calls only the destructor of the base class `IFSStub` which leads to an Undefined Behavior. this call happens at this part of the code:

  llvm/unittests/InterfaceStub/ELFYAMLTest.cpp:46
       Expected<std::unique_ptr<IFSStub>> StubOrErr = readIFSFromBuffer(Data); //Implicit conversion happens here
       [...]
       std::unique_ptr<IFSStub> Stub = std::move(StubOrErr.get());
       [...]
     } // destroy base class here, UB

With this explanation in mind the simplest fix is offered through making the base class's destructor virtual.
This way `std::unique_ptr<IFSStub>` includes a virtual pointer that points to `IFSStubTriple`'s destructor at runtime and everything will work just fine.

I have to mention that this bug  was discovered automatically by changing unique_ptr so that it rejects polymorphic types with non-virtual destructors. This idea was originally due to Lénárd Szolnoki, implemented by Arthur O'Dwyer in my libc++ fork, but we don't propose to merge that unique_ptr patch into the main line; we're just fixing this real bug that it found.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154776

Files:
  llvm/include/llvm/InterfaceStub/IFSStub.h


Index: llvm/include/llvm/InterfaceStub/IFSStub.h
===================================================================
--- llvm/include/llvm/InterfaceStub/IFSStub.h
+++ llvm/include/llvm/InterfaceStub/IFSStub.h
@@ -97,6 +97,7 @@
   IFSStub() = default;
   IFSStub(const IFSStub &Stub);
   IFSStub(IFSStub &&Stub);
+  virtual ~IFSStub() = default;
 };
 
 // Create a alias class for IFSStub.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D154776.538374.patch
Type: text/x-patch
Size: 389 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230708/b0add339/attachment.bin>