This is an archive of the discontinued LLVM Phabricator instance.

IFSStub destructor should be virtual
ClosedPublic

Authored by AMP999 on Jul 8 2023, 2:45 PM.

Details

Summary

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.

Diff Detail

Event Timeline

AMP999 created this revision.Jul 8 2023, 2:45 PM
AMP999 created this object with edit policy "Administrators".
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2023, 2:45 PM
AMP999 requested review of this revision.Jul 8 2023, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2023, 2:45 PM
AMP999 changed the edit policy from "Administrators" to "All Users".Jul 10 2023, 11:09 AM
haowei accepted this revision.Jul 10 2023, 11:30 AM

Thanks for working on this. What you describe is correctly and the patch looks good to me.

This revision is now accepted and ready to land.Jul 10 2023, 11:30 AM

You're Welcome Haowei!
Since I don't have permission to commit I'll send you my name and Email
Name: Amirreza Ashouri
Email: ar.ashouri999@gmail.com
Thank You!

This revision was automatically updated to reflect the committed changes.

You're Welcome Haowei!
Since I don't have permission to commit I'll send you my name and Email
Name: Amirreza Ashouri
Email: ar.ashouri999@gmail.com
Thank You!

My bad, I typed the email address wrong. And it is probably a bad idea to rewrite commit history at this point.

Mistakes happen!
Although it was important for me, but don't worry, no problem!
Thank you for committing the patch on behalf of me!