Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Finally to help track section rewrites #363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce Finally to help track section rewrites #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 88 additions & 32 deletions src/patchelf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,20 @@

#include <algorithm>
#include <limits>
#include <map>
#include <memory>
#include <set>
#include <sstream>
#include <stdexcept>
#include <string>
#include <unordered_map>
#include <vector>
#include <optional>

#include <cassert>
#include <cerrno>
#include <cstdarg>
#include <cstdio>
#include <cstdlib>
#include <cstring>

#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#include "elf.h"
#include "patchelf.h"

#ifndef PACKAGE_STRING
Expand Down Expand Up @@ -1082,6 +1073,16 @@ std::string ElfFile<ElfFileParamNames>::getInterpreter()
template<ElfFileParams>
void ElfFile<ElfFileParamNames>::modifySoname(sonameMode op, const std::string & newSoname)
{

bool soNameModified = false;

Finally finally([&]() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually prefer the old code in cases where we don't need to handle early returns
to avoid hidden control flow. This one is such a case.

if (soNameModified) {
this->rewriteSections();
changed = true;
}
});

if (rdi(hdr()->e_type) != ET_DYN) {
debug("this is not a dynamic library\n");
return;
Expand Down Expand Up @@ -1149,17 +1150,24 @@ void ElfFile<ElfFileParamNames>::modifySoname(sonameMode op, const std::string &
setSubstr(newDynamic, 0, std::string((char *)&newDyn, sizeof(Elf_Dyn)));
}

changed = true;
this->rewriteSections();
soNameModified = true;
}

template<ElfFileParams>
void ElfFile<ElfFileParamNames>::setInterpreter(const std::string & newInterpreter)
{
bool interpreterChanged = false;

Finally finally([&]() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one as well.

if (interpreterChanged) {
this->rewriteSections();
changed = true;
}
});

std::string & section = replaceSection(".interp", newInterpreter.size() + 1);
setSubstr(section, 0, newInterpreter + '\0');
changed = true;
this->rewriteSections();
interpreterChanged = true;
}


Expand Down Expand Up @@ -1222,27 +1230,42 @@ std::string ElfFile<ElfFileParamNames>::shrinkRPath(char* rpath, std::vector<std

template<ElfFileParams>
void ElfFile<ElfFileParamNames>::removeRPath(Elf_Shdr & shdrDynamic) {
bool rpathChanged = false;
Finally finally([&]() {
if (rpathChanged) {
this->rewriteSections();
changed = true;
}
});

auto dyn = (Elf_Dyn *)(fileContents->data() + rdi(shdrDynamic.sh_offset));
Elf_Dyn * last = dyn;
for ( ; rdi(dyn->d_tag) != DT_NULL; dyn++) {
if (rdi(dyn->d_tag) == DT_RPATH) {
debug("removing DT_RPATH entry\n");
changed = true;
rpathChanged = true;
} else if (rdi(dyn->d_tag) == DT_RUNPATH) {
debug("removing DT_RUNPATH entry\n");
changed = true;
rpathChanged = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems fine.

} else {
*last++ = *dyn;
}
}
memset(last, 0, sizeof(Elf_Dyn) * (dyn - last));
this->rewriteSections();
}

template<ElfFileParams>
void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
const std::vector<std::string> & allowedRpathPrefixes, std::string newRPath)
{
bool rpathChanged = false;
Finally finally([&]() {
if (rpathChanged) {
this->rewriteSections();
changed = true;
}
});

auto shdrDynamic = findSectionHeader(".dynamic");

if (rdi(shdrDynamic.sh_type) == SHT_NOBITS) {
Expand Down Expand Up @@ -1321,18 +1344,18 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
wri(dynRPath->d_tag, DT_RUNPATH);
dynRunPath = dynRPath;
dynRPath = nullptr;
changed = true;
rpathChanged = true;
} else if (forceRPath && dynRunPath) { /* convert DT_RUNPATH to DT_RPATH */
wri(dynRunPath->d_tag, DT_RPATH);
dynRPath = dynRunPath;
dynRunPath = nullptr;
changed = true;
rpathChanged = true;
}

if (rpath && rpath == newRPath) {
return;
}
changed = true;
rpathChanged = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems fine.


/* Zero out the previous rpath to prevent retained dependencies in
Nix. */
Expand Down Expand Up @@ -1383,7 +1406,6 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
newDyn.d_un.d_val = shdrDynStr.sh_size;
setSubstr(newDynamic, 0, std::string((char *) &newDyn, sizeof(Elf_Dyn)));
}
this->rewriteSections();
}


Expand All @@ -1392,6 +1414,14 @@ void ElfFile<ElfFileParamNames>::removeNeeded(const std::set<std::string> & libs
{
if (libs.empty()) return;

bool neededChanged = false;
Finally finally([&]() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems fine, but there should be no rewriteSections at the end.

if (neededChanged) {
this->rewriteSections();
changed = true;
}
});

auto shdrDynamic = findSectionHeader(".dynamic");
auto shdrDynStr = findSectionHeader(".dynstr");
char * strTab = (char *) fileContents->data() + rdi(shdrDynStr.sh_offset);
Expand All @@ -1403,7 +1433,7 @@ void ElfFile<ElfFileParamNames>::removeNeeded(const std::set<std::string> & libs
char * name = strTab + rdi(dyn->d_un.d_val);
if (libs.count(name)) {
debug("removing DT_NEEDED entry '%s'\n", name);
changed = true;
neededChanged = true;
} else {
debug("keeping DT_NEEDED entry '%s'\n", name);
*last++ = *dyn;
Expand All @@ -1422,6 +1452,14 @@ void ElfFile<ElfFileParamNames>::replaceNeeded(const std::map<std::string, std::
{
if (libs.empty()) return;

bool neededChanged = false;
Finally finally([&]() {
if (neededChanged) {
this->rewriteSections();
changed = true;
}
});

auto shdrDynamic = findSectionHeader(".dynamic");
auto shdrDynStr = findSectionHeader(".dynstr");
char * strTab = (char *) fileContents->data() + rdi(shdrDynStr.sh_offset);
Expand Down Expand Up @@ -1464,7 +1502,7 @@ void ElfFile<ElfFileParamNames>::replaceNeeded(const std::map<std::string, std::

dynStrAddedBytes += replacement.size() + 1;

changed = true;
neededChanged = true;
} else {
debug("keeping DT_NEEDED entry '%s'\n", name);
}
Expand Down Expand Up @@ -1529,7 +1567,7 @@ void ElfFile<ElfFileParamNames>::replaceNeeded(const std::map<std::string, std::
verStrAddedBytes += replacement.size() + 1;
}

changed = true;
neededChanged = true;
} else {
debug("keeping .gnu.version_r entry '%s'\n", file);
}
Expand All @@ -1538,15 +1576,21 @@ void ElfFile<ElfFileParamNames>::replaceNeeded(const std::map<std::string, std::
--verNeedNum;
}
}

this->rewriteSections();
}

template<ElfFileParams>
void ElfFile<ElfFileParamNames>::addNeeded(const std::set<std::string> & libs)
{
if (libs.empty()) return;

bool neededChanged = false;
Finally finally([&]() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be dropped here.

if (neededChanged) {
this->rewriteSections();
changed = true;
}
});

auto shdrDynamic = findSectionHeader(".dynamic");
auto shdrDynStr = findSectionHeader(".dynstr");

Expand Down Expand Up @@ -1589,9 +1633,7 @@ void ElfFile<ElfFileParamNames>::addNeeded(const std::set<std::string> & libs)
i++;
}

changed = true;

this->rewriteSections();
neededChanged = true;
}

template<ElfFileParams>
Expand All @@ -1615,6 +1657,14 @@ void ElfFile<ElfFileParamNames>::printNeededLibs() // const
template<ElfFileParams>
void ElfFile<ElfFileParamNames>::noDefaultLib()
{
bool defaultLibChanged = false;
Finally finally([&]() {
if (defaultLibChanged) {
this->rewriteSections();
changed = true;
}
});

auto shdrDynamic = findSectionHeader(".dynamic");

auto dyn = (Elf_Dyn *)(fileContents->data() + rdi(shdrDynamic.sh_offset));
Expand Down Expand Up @@ -1648,15 +1698,22 @@ void ElfFile<ElfFileParamNames>::noDefaultLib()
setSubstr(newDynamic, 0, std::string((char *) &newDyn, sizeof(Elf_Dyn)));
}

this->rewriteSections();
changed = true;
defaultLibChanged = true;
}

template<ElfFileParams>
void ElfFile<ElfFileParamNames>::clearSymbolVersions(const std::set<std::string> & syms)
{
if (syms.empty()) return;

bool symbolVersionsChanged = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be dropped here.

Finally finally([&]() {
if (symbolVersionsChanged) {
this->rewriteSections();
changed = true;
}
});

auto shdrDynStr = findSectionHeader(".dynstr");
auto shdrDynsym = findSectionHeader(".dynsym");
auto shdrVersym = findSectionHeader(".gnu.version");
Expand All @@ -1677,8 +1734,7 @@ void ElfFile<ElfFileParamNames>::clearSymbolVersions(const std::set<std::string>
wri(versyms[i], 1);
}
}
changed = true;
this->rewriteSections();
symbolVersionsChanged = true;
}

static bool printInterpreter = false;
Expand Down
24 changes: 24 additions & 0 deletions src/patchelf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
#pragma once

#include <string>
#include <functional>
#include <vector>
#include <optional>
#include <map>
#include <set>
#include <memory>
Comment on lines +3 to +9
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to make this header file more "correct".


#include "elf.h"

using FileContents = std::shared_ptr<std::vector<unsigned char>>;

#define ElfFileParams class Elf_Ehdr, class Elf_Phdr, class Elf_Shdr, class Elf_Addr, class Elf_Off, class Elf_Dyn, class Elf_Sym, class Elf_Verneed, class Elf_Versym
Expand Down Expand Up @@ -157,3 +169,15 @@ class ElfFile
return (const Elf_Ehdr *)fileContents->data();
}
};

/* A trivial class to run a function at the end of a scope. */
class Finally
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/NixOS/nix/blob/3228a38e1c09289b8051b42fc0da1ab0695ca9d7/src/libutil/finally.hh#L6

{
private:
std::function<void()> fun;

public:
Finally(std::function<void()> fun) : fun(fun) { }
~Finally() { fun(); }
};