Skip to content

Commit

Permalink
Fix memory leaks in BinaryEdit::openResolvedLibraryName (#879)
Browse files Browse the repository at this point in the history 
* Improve memory handling and simplify compatibility checks
* Use range-for loops
* Use emplacement for retMap
* Clean up string handling
* Simplify return paths
    This also improves the error messages.
* Fix memory leaks in static executable case
* Cleanup whitespace
* Omit BinaryEdits for incompatible archives of static binaries
  • Loading branch information
@hainest
hainest committed Oct 6, 2020
1 parent 6008fbf commit f0b23c2
Showing 1 changed file with 92 additions and 97 deletions.
189 changes: 92 additions & 97 deletions dyninstAPI/src/unix.C
View file
Original file line number Diff line number Diff line change
Expand Up @@ -553,110 +553,105 @@ bool PCProcess::startDebugger() {

#include "dyninstAPI/src/binaryEdit.h"
#include "symtabAPI/h/Archive.h"
#include <memory>

using namespace Dyninst::SymtabAPI;


mapped_object *BinaryEdit::openResolvedLibraryName(std::string filename,
std::map<std::string, BinaryEdit*> &retMap) {
std::vector<std::string> paths;
std::vector<std::string>::iterator pathIter;
// First, find the specified library file
bool resolved = getResolvedLibraryPath(filename, paths);

// Second, create a set of BinaryEdits for the found library
if ( resolved ) {
startup_printf("[%s:%u] - Opening dependent file %s\n",
FILE__, __LINE__, filename.c_str());

Symtab *origSymtab = getMappedObject()->parse_img()->getObject();
assert(mgr());
// Dynamic case
if ( !origSymtab->isStaticBinary() ) {
for(pathIter = paths.begin(); pathIter != paths.end(); ++pathIter) {
BinaryEdit *temp = BinaryEdit::openFile(*pathIter, mgr(), patcher());

if (temp && temp->getAddressWidth() == getAddressWidth()) {
retMap.insert(std::make_pair(*pathIter, temp));
return temp->getMappedObject();
}
delete temp;
}
} else {
// Static executable case

/*
* Alright, this is a kludge, but even though the Archive is opened
* twice (once here and once by the image class later on), it is
* only parsed once because the Archive class keeps track of all
* open Archives.
*
* This is partly due to the fact that Archives are collections of
* Symtab objects and their is one Symtab for each BinaryEdit. In
* some sense, an Archive is a collection of BinaryEdits.
*/
for(pathIter = paths.begin(); pathIter != paths.end(); ++pathIter) {
Archive *library;
Symtab *singleObject;
if (Archive::openArchive(library, *pathIter)) {
std::vector<Symtab *> members;
if (library->getAllMembers(members)) {
std::vector <Symtab *>::iterator member_it;
for (member_it = members.begin(); member_it != members.end();
++member_it)
{
BinaryEdit *temp = BinaryEdit::openFile(*pathIter,
mgr(), patcher(), (*member_it)->memberName());

if (temp && temp->getAddressWidth() == getAddressWidth()) {
std::string mapName = *pathIter + string(":") +
(*member_it)->memberName();
retMap.insert(std::make_pair(mapName, temp));
}else{
if(temp) delete temp;
retMap.clear();
break;
}
}

if (retMap.size() > 0) {
origSymtab->addLinkingResource(library);
// So we tried loading "libc.a", and got back a swarm of individual members.
// Lovely.
// Just return the first thing...
return retMap.begin()->second->getMappedObject();
}
//if( library ) delete library;
}
} else if (Symtab::openFile(singleObject, *pathIter)) {
BinaryEdit *temp = BinaryEdit::openFile(*pathIter, mgr(), patcher());


if (temp && temp->getAddressWidth() == getAddressWidth()) {
if( singleObject->getObjectType() == obj_SharedLib ||
singleObject->getObjectType() == obj_Executable )
{
startup_printf("%s[%d]: cannot load dynamic object(%s) when rewriting a static binary\n",
FILE__, __LINE__, pathIter->c_str());
std::string msg = std::string("Cannot load a dynamic object when rewriting a static binary");
showErrorCallback(71, msg.c_str());

delete singleObject;
}else{
retMap.insert(std::make_pair(*pathIter, temp));
return temp->getMappedObject();
}
}
if(temp) delete temp;
}
}
std::map<std::string, BinaryEdit *> &retMap) {
std::vector<std::string> paths;
if (!getResolvedLibraryPath(filename, paths)) {
startup_printf("[%s:%u] - Unable to resolve library path for '%s'\n", FILE__, __LINE__,
filename.c_str());
return nullptr;
}

// A little helper to fix some clunky checks
auto is_compatible = [this](std::string const &path, std::string const &member = {}) {
auto temp = std::unique_ptr<BinaryEdit>{BinaryEdit::openFile(path, mgr(), patcher(), member)};
if (temp && temp->getAddressWidth() == getAddressWidth()) {
return temp;
}
temp.reset(nullptr);
return temp;
};

assert(mgr());

// Dynamic case
Symtab *origSymtab = getMappedObject()->parse_img()->getObject();
if (!origSymtab->isStaticBinary()) {
for (auto const &path : paths) {
if (auto temp = is_compatible(path)) {
auto ret = retMap.emplace(path, temp.release());
return (*ret.first).second->getMappedObject();
}
}
startup_printf("[%s:%u] - Unable to find compatible BinaryEdit for '%s'\n", FILE__, __LINE__,
filename.c_str());
return nullptr;
}

// Static executable case

/*
* Alright, this is a kludge, but even though the Archive is opened
* twice (once here and once by the image class later on), it is
* only parsed once because the Archive class keeps track of all
* open Archives.
*
* This is partly due to the fact that Archives are collections of
* Symtab objects and their is one Symtab for each BinaryEdit. In
* some sense, an Archive is a collection of BinaryEdits.
*/
for (auto const &path : paths) {
Archive *library{nullptr};
if (Archive::openArchive(library, path)) {
std::unique_ptr<Archive> lib{library};
std::vector<Symtab *> members;
if (lib->getAllMembers(members)) {
for (auto *member : members) {
if (auto temp = is_compatible(path, member->memberName())) {
std::string mapName = path + ":" + member->memberName();
retMap.emplace(std::move(mapName), temp.release());
} else {
retMap.clear();
break;
}
}

if (retMap.size() > 0) {
origSymtab->addLinkingResource(lib.release());
// So we tried loading "libc.a", and got back a swarm of individual members.
// Just return the first thing...
return retMap.begin()->second->getMappedObject();
}
}
startup_printf(
"[%s:%u] - Failed to find archive members in '%s' for static executable '%s'\n", FILE__,
__LINE__, path.c_str(), filename.c_str());
} else {
Symtab *singleObject{nullptr};
if (Symtab::openFile(singleObject, path)) {
std::unique_ptr<Symtab> obj{singleObject};
if (auto temp = is_compatible(path)) {
if (obj->getObjectType() == obj_SharedLib || obj->getObjectType() == obj_Executable) {
startup_printf(
"%s[%d]: cannot load dynamic object(%s) when rewriting a static binary\n", FILE__,
__LINE__, path.c_str());
std::string msg{"Cannot load a dynamic object when rewriting a static binary"};
showErrorCallback(71, std::move(msg));
} else {
auto ret = retMap.emplace(path, temp.release());
return (*ret.first).second->getMappedObject();
}
}
}
}
}

startup_printf("[%s:%u] - Creation error opening %s\n",
FILE__, __LINE__, filename.c_str());
// If the only thing we could find was a dynamic lib for a static executable, we can reach here; caller should handle this.
return NULL;
startup_printf("[%s:%u] - Creation error opening %s\n", FILE__, __LINE__, filename.c_str());
return nullptr;
}

#endif
Expand Down

0 comments on commit f0b23c2

Please sign in to comment.