From 00121bb932ddbf026297f357c2d3cdf1414f628a Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Tue, 29 Apr 2014 21:52:46 +0000 Subject: [PATCH] PR19553: Memory leak in RuntimeDyldELF::createObjectImageFromFile This starts in MCJIT::getSymbolAddress where the unique_ptr is release()d and (after a cast) passed to a single caller, MCJIT::addObjectFile. addObjectFile calls RuntimeDyld::loadObject. RuntimeDld::loadObject calls RuntimeDyldELF::createObjectFromFile And the pointer is never owned at this point. I say this point, because the alternative codepath, RuntimeDyldMachO::createObjectFile certainly does take ownership, so this seemed like a good hint that this was a/the right place to take ownership. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@207580 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../llvm/ExecutionEngine/ExecutionEngine.h | 2 +- include/llvm/ExecutionEngine/RuntimeDyld.h | 2 +- lib/ExecutionEngine/MCJIT/MCJIT.cpp | 10 +-- lib/ExecutionEngine/MCJIT/MCJIT.h | 2 +- .../RuntimeDyld/ObjectImageCommon.h | 21 ++--- .../RuntimeDyld/RuntimeDyld.cpp | 10 ++- .../RuntimeDyld/RuntimeDyldELF.cpp | 80 +++++++++++-------- .../RuntimeDyld/RuntimeDyldELF.h | 2 +- .../RuntimeDyld/RuntimeDyldMachO.h | 4 +- tools/lli/lli.cpp | 2 +- 10 files changed, 78 insertions(+), 57 deletions(-) diff --git a/include/llvm/ExecutionEngine/ExecutionEngine.h b/include/llvm/ExecutionEngine/ExecutionEngine.h index 70440d725d1..07a04154cd2 100644 --- a/include/llvm/ExecutionEngine/ExecutionEngine.h +++ b/include/llvm/ExecutionEngine/ExecutionEngine.h @@ -222,7 +222,7 @@ public: /// needed by another object. /// /// MCJIT will take ownership of the ObjectFile. - virtual void addObjectFile(object::ObjectFile *O) { + virtual void addObjectFile(std::unique_ptr O) { llvm_unreachable( "ExecutionEngine subclass doesn't implement addObjectFile."); } diff --git a/include/llvm/ExecutionEngine/RuntimeDyld.h b/include/llvm/ExecutionEngine/RuntimeDyld.h index 8d7b81bb6e2..30c0d49ade0 100644 --- a/include/llvm/ExecutionEngine/RuntimeDyld.h +++ b/include/llvm/ExecutionEngine/RuntimeDyld.h @@ -55,7 +55,7 @@ public: /// Ownership of the input object is transferred to the ObjectImage /// instance returned from this function if successful. In the case of load /// failure, the input object will be deleted. - ObjectImage *loadObject(object::ObjectFile *InputObject); + ObjectImage *loadObject(std::unique_ptr InputObject); /// Get the address of our local copy of the symbol. This may or may not /// be the address used for relocation (clients can copy the data around diff --git a/lib/ExecutionEngine/MCJIT/MCJIT.cpp b/lib/ExecutionEngine/MCJIT/MCJIT.cpp index 6b9dff238ad..42cb4ea6d84 100644 --- a/lib/ExecutionEngine/MCJIT/MCJIT.cpp +++ b/lib/ExecutionEngine/MCJIT/MCJIT.cpp @@ -113,8 +113,8 @@ bool MCJIT::removeModule(Module *M) { -void MCJIT::addObjectFile(object::ObjectFile *Obj) { - ObjectImage *LoadedObject = Dyld.loadObject(Obj); +void MCJIT::addObjectFile(std::unique_ptr Obj) { + ObjectImage *LoadedObject = Dyld.loadObject(std::move(Obj)); if (!LoadedObject || Dyld.hasError()) report_fatal_error(Dyld.getErrorString()); @@ -308,10 +308,10 @@ uint64_t MCJIT::getSymbolAddress(const std::string &Name, std::unique_ptr ChildBin; // FIXME: Support nested archives? if (!ChildIt->getAsBinary(ChildBin) && ChildBin->isObject()) { - object::ObjectFile *OF = reinterpret_cast( - ChildBin.release()); + std::unique_ptr OF( + static_cast(ChildBin.release())); // This causes the object file to be loaded. - addObjectFile(OF); + addObjectFile(std::move(OF)); // The address should be here now. Addr = getExistingSymbolAddress(Name); if (Addr) diff --git a/lib/ExecutionEngine/MCJIT/MCJIT.h b/lib/ExecutionEngine/MCJIT/MCJIT.h index da1e975d4cd..100e9a23fcd 100644 --- a/lib/ExecutionEngine/MCJIT/MCJIT.h +++ b/lib/ExecutionEngine/MCJIT/MCJIT.h @@ -239,7 +239,7 @@ public: /// @name ExecutionEngine interface implementation /// @{ void addModule(Module *M) override; - void addObjectFile(object::ObjectFile *O) override; + void addObjectFile(std::unique_ptr O) override; void addArchive(object::Archive *O) override; bool removeModule(Module *M) override; diff --git a/lib/ExecutionEngine/RuntimeDyld/ObjectImageCommon.h b/lib/ExecutionEngine/RuntimeDyld/ObjectImageCommon.h index f5a4ea93285..4917b93a96e 100644 --- a/lib/ExecutionEngine/RuntimeDyld/ObjectImageCommon.h +++ b/lib/ExecutionEngine/RuntimeDyld/ObjectImageCommon.h @@ -18,6 +18,8 @@ #include "llvm/ExecutionEngine/ObjectImage.h" #include "llvm/Object/ObjectFile.h" +#include + namespace llvm { namespace object { @@ -30,13 +32,13 @@ class ObjectImageCommon : public ObjectImage { void anchor() override; protected: - object::ObjectFile *ObjFile; + std::unique_ptr ObjFile; // This form of the constructor allows subclasses to use // format-specific subclasses of ObjectFile directly - ObjectImageCommon(ObjectBuffer *Input, object::ObjectFile *Obj) + ObjectImageCommon(ObjectBuffer *Input, std::unique_ptr Obj) : ObjectImage(Input), // saves Input as Buffer and takes ownership - ObjFile(Obj) + ObjFile(std::move(Obj)) { } @@ -44,12 +46,13 @@ public: ObjectImageCommon(ObjectBuffer* Input) : ObjectImage(Input) // saves Input as Buffer and takes ownership { - ObjFile = - object::ObjectFile::createObjectFile(Buffer->getMemBuffer()).get(); + // FIXME: error checking? createObjectFile returns an ErrorOr + // and should probably be checked for failure. + ObjFile.reset(object::ObjectFile::createObjectFile(Buffer->getMemBuffer()).get()); } - ObjectImageCommon(object::ObjectFile* Input) - : ObjectImage(nullptr), ObjFile(Input) {} - virtual ~ObjectImageCommon() { delete ObjFile; } + ObjectImageCommon(std::unique_ptr Input) + : ObjectImage(nullptr), ObjFile(std::move(Input)) {} + virtual ~ObjectImageCommon() { } object::symbol_iterator begin_symbols() const override { return ObjFile->symbol_begin(); } @@ -66,7 +69,7 @@ public: StringRef getData() const override { return ObjFile->getData(); } - object::ObjectFile* getObjectFile() const override { return ObjFile; } + object::ObjectFile* getObjectFile() const override { return ObjFile.get(); } // Subclasses can override these methods to update the image with loaded // addresses for sections and common symbols diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp index cc2f29d9df1..12b3e8d75d5 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp @@ -698,21 +698,23 @@ createRuntimeDyldMachO(RTDyldMemoryManager *MM, bool ProcessAllSections) { return Dyld; } -ObjectImage *RuntimeDyld::loadObject(ObjectFile *InputObject) { +ObjectImage *RuntimeDyld::loadObject(std::unique_ptr InputObject) { std::unique_ptr InputImage; + ObjectFile &Obj = *InputObject; + if (InputObject->isELF()) { - InputImage.reset(RuntimeDyldELF::createObjectImageFromFile(InputObject)); + InputImage.reset(RuntimeDyldELF::createObjectImageFromFile(std::move(InputObject))); if (!Dyld) Dyld = createRuntimeDyldELF(MM, ProcessAllSections).release(); } else if (InputObject->isMachO()) { - InputImage.reset(RuntimeDyldMachO::createObjectImageFromFile(InputObject)); + InputImage.reset(RuntimeDyldMachO::createObjectImageFromFile(std::move(InputObject))); if (!Dyld) Dyld = createRuntimeDyldMachO(MM, ProcessAllSections).release(); } else report_fatal_error("Incompatible object format!"); - if (!Dyld->isCompatibleFile(InputObject)) + if (!Dyld->isCompatibleFile(&Obj)) report_fatal_error("Incompatible object format!"); Dyld->loadObject(InputImage.get()); diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp index abe8b6c388c..41ae7010485 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp @@ -51,7 +51,12 @@ template class DyldELFObject : public ELFObjectFile { typedef typename ELFDataTypeTypedefHelper::value_type addr_type; + std::unique_ptr UnderlyingFile; + public: + DyldELFObject(std::unique_ptr UnderlyingFile, + MemoryBuffer *Wrapper, error_code &ec); + DyldELFObject(MemoryBuffer *Wrapper, error_code &ec); void updateSectionAddress(const SectionRef &Sec, uint64_t Addr); @@ -68,13 +73,11 @@ public: }; template class ELFObjectImage : public ObjectImageCommon { -protected: - DyldELFObject *DyldObj; bool Registered; public: - ELFObjectImage(ObjectBuffer *Input, DyldELFObject *Obj) - : ObjectImageCommon(Input, Obj), DyldObj(Obj), Registered(false) {} + ELFObjectImage(ObjectBuffer *Input, std::unique_ptr> Obj) + : ObjectImageCommon(Input, std::move(Obj)), Registered(false) {} virtual ~ELFObjectImage() { if (Registered) @@ -84,11 +87,13 @@ public: // Subclasses can override these methods to update the image with loaded // addresses for sections and common symbols void updateSectionAddress(const SectionRef &Sec, uint64_t Addr) override { - DyldObj->updateSectionAddress(Sec, Addr); + static_cast*>(getObjectFile()) + ->updateSectionAddress(Sec, Addr); } void updateSymbolAddress(const SymbolRef &Sym, uint64_t Addr) override { - DyldObj->updateSymbolAddress(Sym, Addr); + static_cast*>(getObjectFile()) + ->updateSymbolAddress(Sym, Addr); } void registerWithDebugger() override { @@ -109,6 +114,14 @@ DyldELFObject::DyldELFObject(MemoryBuffer *Wrapper, error_code &ec) this->isDyldELFObject = true; } +template +DyldELFObject::DyldELFObject(std::unique_ptr UnderlyingFile, + MemoryBuffer *Wrapper, error_code &ec) + : ELFObjectFile(Wrapper, ec), + UnderlyingFile(std::move(UnderlyingFile)) { + this->isDyldELFObject = true; +} + template void DyldELFObject::updateSectionAddress(const SectionRef &Sec, uint64_t Addr) { @@ -165,7 +178,7 @@ void RuntimeDyldELF::deregisterEHFrames() { } ObjectImage * -RuntimeDyldELF::createObjectImageFromFile(object::ObjectFile *ObjFile) { +RuntimeDyldELF::createObjectImageFromFile(std::unique_ptr ObjFile) { if (!ObjFile) return nullptr; @@ -174,21 +187,23 @@ RuntimeDyldELF::createObjectImageFromFile(object::ObjectFile *ObjFile) { MemoryBuffer::getMemBuffer(ObjFile->getData(), "", false); if (ObjFile->getBytesInAddress() == 4 && ObjFile->isLittleEndian()) { - DyldELFObject> *Obj = - new DyldELFObject>(Buffer, ec); - return new ELFObjectImage>(nullptr, Obj); + auto Obj = make_unique>>(std::move(ObjFile), Buffer, ec); + return new ELFObjectImage>( + nullptr, std::move(Obj)); } else if (ObjFile->getBytesInAddress() == 4 && !ObjFile->isLittleEndian()) { - DyldELFObject> *Obj = - new DyldELFObject>(Buffer, ec); - return new ELFObjectImage>(nullptr, Obj); + auto Obj = make_unique>>( + std::move(ObjFile), Buffer, ec); + return new ELFObjectImage>(nullptr, std::move(Obj)); } else if (ObjFile->getBytesInAddress() == 8 && !ObjFile->isLittleEndian()) { - DyldELFObject> *Obj = - new DyldELFObject>(Buffer, ec); - return new ELFObjectImage>(nullptr, Obj); + auto Obj = make_unique>>( + std::move(ObjFile), Buffer, ec); + return new ELFObjectImage>(nullptr, + std::move(Obj)); } else if (ObjFile->getBytesInAddress() == 8 && ObjFile->isLittleEndian()) { - DyldELFObject> *Obj = - new DyldELFObject>(Buffer, ec); - return new ELFObjectImage>(nullptr, Obj); + auto Obj = make_unique>>( + std::move(ObjFile), Buffer, ec); + return new ELFObjectImage>( + nullptr, std::move(Obj)); } else llvm_unreachable("Unexpected ELF format"); } @@ -202,28 +217,29 @@ ObjectImage *RuntimeDyldELF::createObjectImage(ObjectBuffer *Buffer) { error_code ec; if (Ident.first == ELF::ELFCLASS32 && Ident.second == ELF::ELFDATA2LSB) { - DyldELFObject> *Obj = - new DyldELFObject>( - Buffer->getMemBuffer(), ec); - return new ELFObjectImage>(Buffer, Obj); + auto Obj = make_unique>>( + Buffer->getMemBuffer(), ec); + return new ELFObjectImage>( + Buffer, std::move(Obj)); } else if (Ident.first == ELF::ELFCLASS32 && Ident.second == ELF::ELFDATA2MSB) { - DyldELFObject> *Obj = - new DyldELFObject>( + auto Obj = + make_unique>>( Buffer->getMemBuffer(), ec); - return new ELFObjectImage>(Buffer, Obj); + return new ELFObjectImage>(Buffer, + std::move(Obj)); } else if (Ident.first == ELF::ELFCLASS64 && Ident.second == ELF::ELFDATA2MSB) { - DyldELFObject> *Obj = - new DyldELFObject>( + auto Obj = + make_unique>>( Buffer->getMemBuffer(), ec); - return new ELFObjectImage>(Buffer, Obj); + return new ELFObjectImage>(Buffer, std::move(Obj)); } else if (Ident.first == ELF::ELFCLASS64 && Ident.second == ELF::ELFDATA2LSB) { - DyldELFObject> *Obj = - new DyldELFObject>( + auto Obj = + make_unique>>( Buffer->getMemBuffer(), ec); - return new ELFObjectImage>(Buffer, Obj); + return new ELFObjectImage>(Buffer, std::move(Obj)); } else llvm_unreachable("Unexpected ELF format"); } diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h index 27db5cdf42c..e08f2ee4390 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h @@ -119,7 +119,7 @@ public: virtual ~RuntimeDyldELF(); static ObjectImage *createObjectImage(ObjectBuffer *InputBuffer); - static ObjectImage *createObjectImageFromFile(object::ObjectFile *Obj); + static ObjectImage *createObjectImageFromFile(std::unique_ptr Obj); }; } // end namespace llvm diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.h b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.h index 10061767534..85d6501a264 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.h +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.h @@ -88,8 +88,8 @@ public: } static ObjectImage * - createObjectImageFromFile(object::ObjectFile *InputObject) { - return new ObjectImageCommon(InputObject); + createObjectImageFromFile(std::unique_ptr InputObject) { + return new ObjectImageCommon(std::move(InputObject)); } }; diff --git a/tools/lli/lli.cpp b/tools/lli/lli.cpp index f1413f302a3..4cde10579c8 100644 --- a/tools/lli/lli.cpp +++ b/tools/lli/lli.cpp @@ -534,7 +534,7 @@ int main(int argc, char **argv, char * const *envp) { Err.print(argv[0], errs()); return 1; } - EE->addObjectFile(Obj.get()); + EE->addObjectFile(std::unique_ptr(Obj.get())); } for (unsigned i = 0, e = ExtraArchives.size(); i != e; ++i) { -- 2.34.1