From ce5907fc40d5e355872a4288fde3de7586e6e766 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:44 +0100 Subject: Add required includes in header file Enable to parse the header file on its own, e.g. for language servers (clangd). --- src/patchelf.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/patchelf.h b/src/patchelf.h index c3096ff..1308815 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -1,3 +1,12 @@ +#include +#include +#include +#include +#include +#include + +#include "elf.h" + using FileContents = std::shared_ptr>; #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 -- cgit v1.2.1 From fbb12a72ad1a98c079597b820364196fcfc512af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:52 +0100 Subject: Add misc functions annotations Make the behavior of functions more explicit. --- src/patchelf.cc | 8 ++++---- src/patchelf.h | 32 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index e91e2ab..1a67158 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -104,7 +104,7 @@ static std::string downcase(std::string s) why... */ template template -I ElfFile::rdi(I i) const +constexpr I ElfFile::rdi(I i) const noexcept { I r = 0; if (littleEndian) { @@ -131,7 +131,7 @@ static void debug(const char * format, ...) } -void fmt2(std::ostringstream & out) +static void fmt2([[maybe_unused]] std::ostringstream & out) { } @@ -309,7 +309,7 @@ ElfFile::ElfFile(FileContents fContents) template -unsigned int ElfFile::getPageSize() const +unsigned int ElfFile::getPageSize() const noexcept { if (forcedPageSize > 0) return forcedPageSize; @@ -555,7 +555,7 @@ std::optional> ElfFile::tryF template -unsigned int ElfFile::getSectionIndex(const SectionName & sectionName) +unsigned int ElfFile::getSectionIndex(const SectionName & sectionName) const { for (unsigned int i = 1; i < rdi(hdr()->e_shnum); ++i) if (getSectionName(shdrs.at(i)) == sectionName) return i; diff --git a/src/patchelf.h b/src/patchelf.h index 1308815..4857ab6 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -39,14 +39,14 @@ private: /* Align on 4 or 8 bytes boundaries on 32- or 64-bit platforms respectively. */ - size_t sectionAlignment = sizeof(Elf_Off); + static constexpr size_t sectionAlignment = sizeof(Elf_Off); std::vector sectionsByOldIndex; public: explicit ElfFile(FileContents fileContents); - bool isChanged() + [[nodiscard]] bool isChanged() const noexcept { return changed; } @@ -55,8 +55,8 @@ private: struct CompPhdr { - ElfFile * elfFile; - bool operator ()(const Elf_Phdr & x, const Elf_Phdr & y) + const ElfFile * elfFile; + bool operator ()(const Elf_Phdr & x, const Elf_Phdr & y) const noexcept { // A PHDR comes before everything else. if (elfFile->rdi(y.p_type) == PT_PHDR) return false; @@ -73,8 +73,8 @@ private: struct CompShdr { - ElfFile * elfFile; - bool operator ()(const Elf_Shdr & x, const Elf_Shdr & y) + const ElfFile * elfFile; + bool operator ()(const Elf_Shdr & x, const Elf_Shdr & y) const noexcept { return elfFile->rdi(x.sh_offset) < elfFile->rdi(y.sh_offset); } @@ -82,24 +82,24 @@ private: friend struct CompShdr; - unsigned int getPageSize() const; + [[nodiscard]] unsigned int getPageSize() const noexcept; void sortShdrs(); void shiftFile(unsigned int extraPages, size_t sizeOffset, size_t extraBytes); - std::string getSectionName(const Elf_Shdr & shdr) const; + [[nodiscard]] std::string getSectionName(const Elf_Shdr & shdr) const; Elf_Shdr & findSectionHeader(const SectionName & sectionName); - std::optional> tryFindSectionHeader(const SectionName & sectionName); + [[nodiscard]] std::optional> tryFindSectionHeader(const SectionName & sectionName); - unsigned int getSectionIndex(const SectionName & sectionName); + [[nodiscard]] unsigned int getSectionIndex(const SectionName & sectionName) const; std::string & replaceSection(const SectionName & sectionName, unsigned int size); - bool haveReplacedSection(const SectionName & sectionName) const; + [[nodiscard]] bool haveReplacedSection(const SectionName & sectionName) const; void writeReplacedSections(Elf_Off & curOff, Elf_Addr startAddr, Elf_Off startOffset); @@ -116,7 +116,7 @@ public: void rewriteSections(bool force = false); - std::string getInterpreter(); + [[nodiscard]] std::string getInterpreter(); typedef enum { printOsAbi, replaceOsAbi } osAbiMode; @@ -158,21 +158,21 @@ private: specified by the ELF header) to this platform's integer representation. */ template - I rdi(I i) const; + constexpr I rdi(I i) const noexcept; /* Convert back to the ELF representation. */ template - I wri(I & t, unsigned long long i) const + constexpr I wri(I & t, unsigned long long i) const { t = rdi((I) i); return i; } - Elf_Ehdr *hdr() { + [[nodiscard]] Elf_Ehdr *hdr() noexcept { return (Elf_Ehdr *)fileContents->data(); } - const Elf_Ehdr *hdr() const { + [[nodiscard]] const Elf_Ehdr *hdr() const noexcept { return (const Elf_Ehdr *)fileContents->data(); } }; -- cgit v1.2.1 From 09673eb553b3dc7c67627b51c34a8b9cc2f8d0f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:54 +0100 Subject: Drop unnecessary friend declarations --- src/patchelf.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/patchelf.h b/src/patchelf.h index 4857ab6..79e7a17 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -67,8 +67,6 @@ private: } }; - friend struct CompPhdr; - void sortPhdrs(); struct CompShdr @@ -80,8 +78,6 @@ private: } }; - friend struct CompShdr; - [[nodiscard]] unsigned int getPageSize() const noexcept; void sortShdrs(); -- cgit v1.2.1 From 404761cf4df321580b40baacd50cd3d3e5cb643f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:55 +0100 Subject: Use C++ casts instead of raw C ones in hdr() --- src/patchelf.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/patchelf.h b/src/patchelf.h index 79e7a17..7c1165b 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -165,10 +165,10 @@ private: } [[nodiscard]] Elf_Ehdr *hdr() noexcept { - return (Elf_Ehdr *)fileContents->data(); + return reinterpret_cast(fileContents->data()); } [[nodiscard]] const Elf_Ehdr *hdr() const noexcept { - return (const Elf_Ehdr *)fileContents->data(); + return reinterpret_cast(fileContents->data()); } }; -- cgit v1.2.1 From a3934549633550ea625604c03e25724664048f40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:56 +0100 Subject: Avoid unnecessary copies in splitColonDelimitedString() Avoid creating a stringstream by using find(). --- src/patchelf.cc | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 1a67158..946e81c 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -69,14 +70,18 @@ static int forcedPageSize = -1; #define EM_LOONGARCH 258 #endif - -static std::vector splitColonDelimitedString(const char * s) +[[nodiscard]] static std::vector splitColonDelimitedString(std::string_view s) { - std::string item; std::vector parts; - std::stringstream ss(s); - while (std::getline(ss, item, ':')) - parts.push_back(item); + + size_t pos; + while ((pos = s.find(':')) != std::string_view::npos) { + parts.emplace_back(s.substr(0, pos)); + s = s.substr(pos + 1); + } + + if (!s.empty()) + parts.emplace_back(s); return parts; } -- cgit v1.2.1 From c00676a28af82f4c73c19916b803d7058f02d104 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:57 +0100 Subject: Close file before potentially throwing --- src/patchelf.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 946e81c..8240361 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -196,11 +196,11 @@ static FileContents readFile(const std::string & fileName, while ((portion = read(fd, contents->data() + bytesRead, size - bytesRead)) > 0) bytesRead += portion; + close(fd); + if (bytesRead != size) throw SysError(fmt("reading '", fileName, "'")); - close(fd); - return contents; } -- cgit v1.2.1 From 98d1813f2516aa4c771dd8824e7cada98393049f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:58 +0100 Subject: Drop unnecessary casts in getElfType() Also declare the function static and warn if return value not used. --- src/patchelf.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 8240361..4c94175 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -212,10 +212,10 @@ struct ElfType }; -ElfType getElfType(const FileContents & fileContents) +[[nodiscard]] static ElfType getElfType(const FileContents & fileContents) { /* Check the ELF header for basic validity. */ - if (fileContents->size() < static_cast(sizeof(Elf32_Ehdr))) + if (fileContents->size() < sizeof(Elf32_Ehdr)) error("missing ELF header"); auto contents = fileContents->data(); -- cgit v1.2.1 From e8832294372f0e7c69948d701f3613183a4f78a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:40:59 +0100 Subject: Avoid potential overflows in checkPointer() Prevent overflows in the addtion of q and size, and avoid truncations in callers by using size_t as type for size. --- src/patchelf.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 4c94175..6ea238e 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -236,10 +236,9 @@ struct ElfType } -static void checkPointer(const FileContents & contents, void * p, unsigned int size) +static void checkPointer(const FileContents & contents, const void * p, size_t size) { - auto q = static_cast(p); - if (!(q >= contents->data() && q + size <= contents->data() + contents->size())) + if (p < contents->data() || size > contents->size() || p > contents->data() + contents->size() - size) error("data region extends past file end"); } -- cgit v1.2.1 From b2897ab819812060c43ebc9fafa1483b1843536b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:01 +0100 Subject: Use C++11 [[noreturn]] --- src/patchelf.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 6ea238e..0b7c249 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -167,7 +167,7 @@ struct SysError : std::runtime_error { } }; -__attribute__((noreturn)) static void error(const std::string & msg) +[[noreturn]] static void error(const std::string & msg) { if (errno) throw SysError(msg); -- cgit v1.2.1 From 1c2d1fffaffb1cd97b49d48c503d8dde03f56696 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:02 +0100 Subject: Drop superfluous semicolons --- src/patchelf.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 0b7c249..c33379f 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -1472,7 +1472,7 @@ void ElfFile::modifyRPath(RPathOp op, case rpPrint: { printf("%s\n", rpath ? rpath : ""); return; - }; + } case rpRemove: { if (!rpath) { debug("no RPATH to delete\n"); @@ -1485,7 +1485,7 @@ void ElfFile::modifyRPath(RPathOp op, if (!rpath) { debug("no RPATH to shrink\n"); return; - ;} + } newRPath = shrinkRPath(rpath, neededLibs, allowedRpathPrefixes); break; } -- cgit v1.2.1 From 889350de509de7d5d6724b66c18ef1e09c92cbb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:03 +0100 Subject: Declare file local functions static --- src/patchelf.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index c33379f..80c2d93 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -2077,7 +2077,7 @@ static void patchElf() } } -std::string resolveArgument(const char *arg) { +[[nodiscard]] static std::string resolveArgument(const char *arg) { if (strlen(arg) > 0 && arg[0] == '@') { FileContents cnts = readFile(arg + 1); return std::string((char *)cnts->data(), cnts->size()); @@ -2087,7 +2087,7 @@ std::string resolveArgument(const char *arg) { } -void showHelp(const std::string & progName) +static void showHelp(const std::string & progName) { fprintf(stderr, "syntax: %s\n\ [--set-interpreter FILENAME]\n\ @@ -2122,7 +2122,7 @@ void showHelp(const std::string & progName) } -int mainWrapped(int argc, char * * argv) +static int mainWrapped(int argc, char * * argv) { if (argc <= 1) { showHelp(argv[0]); -- cgit v1.2.1 From b5b59ca4cbdc7bc9ba30c46fb6307edfdfe90c94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:04 +0100 Subject: Avoid dropping const qualifier --- src/Makefile.am | 2 +- src/patchelf.cc | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index fa0a9cc..a85204e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,4 +1,4 @@ -AM_CXXFLAGS = -Wall -std=c++17 -D_FILE_OFFSET_BITS=64 +AM_CXXFLAGS = -Wall -Wcast-qual -std=c++17 -D_FILE_OFFSET_BITS=64 if WITH_ASAN AM_CXXFLAGS += -fsanitize=address -fsanitize-address-use-after-scope diff --git a/src/patchelf.cc b/src/patchelf.cc index 80c2d93..08f0882 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -621,7 +621,7 @@ void ElfFile::writeReplacedSections(Elf_Off & curOff, debug("rewriting section '%s' from offset 0x%x (size %d) to offset 0x%x (size %d)\n", sectionName.c_str(), rdi(shdr.sh_offset), rdi(shdr.sh_size), curOff, i->second.size()); - memcpy(fileContents->data() + curOff, (unsigned char *) i->second.c_str(), + memcpy(fileContents->data() + curOff, i->second.c_str(), i->second.size()); /* Update the section header for this section. */ @@ -1316,7 +1316,7 @@ void ElfFile::modifySoname(sonameMode op, const std::string & std::string & newDynamic = replaceSection(".dynamic", rdi(shdrDynamic.sh_size) + sizeof(Elf_Dyn)); unsigned int idx = 0; - for (; rdi(((Elf_Dyn *) newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++); + for (; rdi(reinterpret_cast(newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++); debug("DT_NULL index is %d\n", idx); /* Shift all entries down by one. */ @@ -1551,7 +1551,7 @@ void ElfFile::modifyRPath(RPathOp op, rdi(shdrDynamic.sh_size) + sizeof(Elf_Dyn)); unsigned int idx = 0; - for ( ; rdi(((Elf_Dyn *) newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; + for ( ; rdi(reinterpret_cast(newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; debug("DT_NULL index is %d\n", idx); /* Shift all entries down by one. */ @@ -1753,7 +1753,7 @@ void ElfFile::addNeeded(const std::set & libs) rdi(shdrDynamic.sh_size) + sizeof(Elf_Dyn) * libs.size()); unsigned int idx = 0; - for ( ; rdi(((Elf_Dyn *) newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; + for ( ; rdi(reinterpret_cast(newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; debug("DT_NULL index is %d\n", idx); /* Shift all entries down by the number of new entries. */ @@ -1815,7 +1815,7 @@ void ElfFile::noDefaultLib() rdi(shdrDynamic.sh_size) + sizeof(Elf_Dyn)); unsigned int idx = 0; - for ( ; rdi(((Elf_Dyn *) newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; + for ( ; rdi(reinterpret_cast(newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; debug("DT_NULL index is %d\n", idx); /* Shift all entries down by one. */ @@ -1848,7 +1848,7 @@ void ElfFile::addDebugTag() rdi(shdrDynamic.sh_size) + sizeof(Elf_Dyn)); unsigned int idx = 0; - for ( ; rdi(((Elf_Dyn *) newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; + for ( ; rdi(reinterpret_cast(newDynamic.c_str())[idx].d_tag) != DT_NULL; idx++) ; debug("DT_NULL index is %d\n", idx); /* Shift all entries down by one. */ -- cgit v1.2.1 From 959cd168db3860af6d63892d5cf6836d55bf2ced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:05 +0100 Subject: Enable Wextra Enable common additional compiler warnings. --- src/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index a85204e..93e92d8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,4 +1,4 @@ -AM_CXXFLAGS = -Wall -Wcast-qual -std=c++17 -D_FILE_OFFSET_BITS=64 +AM_CXXFLAGS = -Wall -Wextra -Wcast-qual -std=c++17 -D_FILE_OFFSET_BITS=64 if WITH_ASAN AM_CXXFLAGS += -fsanitize=address -fsanitize-address-use-after-scope -- cgit v1.2.1 From 28e95b30f3e7f1920a51a43ae275ec6f3508b495 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:06 +0100 Subject: Avoid implicit value truncations in wri() Abort on truncation of values being written to the ELF data, to prevent silent behavior mismatch. --- src/patchelf.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/patchelf.h b/src/patchelf.h index 7c1165b..101b1c9 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -157,11 +158,14 @@ private: constexpr I rdi(I i) const noexcept; /* Convert back to the ELF representation. */ - template - constexpr I wri(I & t, unsigned long long i) const + template + constexpr inline I wri(I & t, U i) const { - t = rdi((I) i); - return i; + I val = static_cast(i); + if (static_cast(val) != i) + throw std::runtime_error { "value truncation" }; + t = rdi(val); + return val; } [[nodiscard]] Elf_Ehdr *hdr() noexcept { -- cgit v1.2.1 From 00d1e82f2b1e415ec66d9aacb7e760d3ff617f02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:07 +0100 Subject: Avoid implicit conversion Use auto to avoid implicit type conversion, hiding possible value truncation. --- src/patchelf.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 08f0882..c10e369 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -290,11 +290,11 @@ ElfFile::ElfFile(FileContents fContents) /* Get the section header string table section (".shstrtab"). Its index in the section header table is given by e_shstrndx field of the ELF header. */ - unsigned int shstrtabIndex = rdi(hdr()->e_shstrndx); + auto shstrtabIndex = rdi(hdr()->e_shstrndx); if (shstrtabIndex >= shdrs.size()) error("string table index out of bounds"); - unsigned int shstrtabSize = rdi(shdrs[shstrtabIndex].sh_size); + auto shstrtabSize = rdi(shdrs[shstrtabIndex].sh_size); char * shstrtab = (char * ) fileContents->data() + rdi(shdrs[shstrtabIndex].sh_offset); checkPointer(fileContents, shstrtab, shstrtabSize); -- cgit v1.2.1 From 81c64ddc99ad448408689bea0de2f1d22dd10762 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:08 +0100 Subject: Declare more read-only functions const --- src/patchelf.cc | 16 ++++++++-------- src/patchelf.h | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index c10e369..7bfabb4 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -535,7 +535,7 @@ std::string ElfFile::getSectionName(const Elf_Shdr & shdr) co template -Elf_Shdr & ElfFile::findSectionHeader(const SectionName & sectionName) +const Elf_Shdr & ElfFile::findSectionHeader(const SectionName & sectionName) const { auto shdr = tryFindSectionHeader(sectionName); if (!shdr) { @@ -549,7 +549,7 @@ Elf_Shdr & ElfFile::findSectionHeader(const SectionName & sec template -std::optional> ElfFile::tryFindSectionHeader(const SectionName & sectionName) +std::optional> ElfFile::tryFindSectionHeader(const SectionName & sectionName) const { auto i = getSectionIndex(sectionName); if (i) @@ -602,7 +602,7 @@ void ElfFile::writeReplacedSections(Elf_Off & curOff, clobbering previously written new section contents. */ for (auto & i : replacedSections) { const std::string & sectionName = i.first; - Elf_Shdr & shdr = findSectionHeader(sectionName); + const Elf_Shdr & shdr = findSectionHeader(sectionName); if (rdi(shdr.sh_type) != SHT_NOBITS) memset(fileContents->data() + rdi(shdr.sh_offset), 'X', rdi(shdr.sh_size)); } @@ -1190,10 +1190,10 @@ static void setSubstr(std::string & s, unsigned int pos, const std::string & t) template -std::string ElfFile::getInterpreter() +std::string ElfFile::getInterpreter() const { auto shdr = findSectionHeader(".interp"); - return std::string((char *) fileContents->data() + rdi(shdr.sh_offset), rdi(shdr.sh_size) - 1); + return std::string((const char *) fileContents->data() + rdi(shdr.sh_offset), rdi(shdr.sh_size) - 1); } template @@ -1776,13 +1776,13 @@ void ElfFile::addNeeded(const std::set & libs) } template -void ElfFile::printNeededLibs() // const +void ElfFile::printNeededLibs() const { const auto shdrDynamic = findSectionHeader(".dynamic"); const auto shdrDynStr = findSectionHeader(".dynstr"); - const char *strTab = (char *)fileContents->data() + rdi(shdrDynStr.sh_offset); + const char *strTab = (const char *)fileContents->data() + rdi(shdrDynStr.sh_offset); - const Elf_Dyn *dyn = (Elf_Dyn *) (fileContents->data() + rdi(shdrDynamic.sh_offset)); + const Elf_Dyn *dyn = (const Elf_Dyn *) (fileContents->data() + rdi(shdrDynamic.sh_offset)); for (; rdi(dyn->d_tag) != DT_NULL; dyn++) { if (rdi(dyn->d_tag) == DT_NEEDED) { diff --git a/src/patchelf.h b/src/patchelf.h index 101b1c9..677462d 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -87,9 +87,9 @@ private: [[nodiscard]] std::string getSectionName(const Elf_Shdr & shdr) const; - Elf_Shdr & findSectionHeader(const SectionName & sectionName); + const Elf_Shdr & findSectionHeader(const SectionName & sectionName) const; - [[nodiscard]] std::optional> tryFindSectionHeader(const SectionName & sectionName); + [[nodiscard]] std::optional> tryFindSectionHeader(const SectionName & sectionName) const; [[nodiscard]] unsigned int getSectionIndex(const SectionName & sectionName) const; @@ -113,7 +113,7 @@ public: void rewriteSections(bool force = false); - [[nodiscard]] std::string getInterpreter(); + [[nodiscard]] std::string getInterpreter() const; typedef enum { printOsAbi, replaceOsAbi } osAbiMode; @@ -137,7 +137,7 @@ public: void replaceNeeded(const std::map & libs); - void printNeededLibs() /* should be const */; + void printNeededLibs() const; void noDefaultLib(); -- cgit v1.2.1 From ff7a5beb000a06842fc046c81388e0e103255f3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 21 Feb 2023 19:41:11 +0100 Subject: Avoid memory corruption on invalid ELF input Reject ELF data that would lead to invalid memory access or integer overflows. --- src/patchelf.cc | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 7bfabb4..126cada 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -243,6 +243,25 @@ static void checkPointer(const FileContents & contents, const void * p, size_t s } +static void checkOffset(const FileContents & contents, size_t offset, size_t size) +{ + size_t end; + + if (offset > contents->size() + || size > contents->size() + || __builtin_add_overflow(offset, size, &end) + || end > contents->size()) + error("data offset extends past file end"); +} + + +static std::string extractString(const FileContents & contents, size_t offset, size_t size) +{ + checkOffset(contents, offset, size); + return { reinterpret_cast(contents->data()) + offset, size }; +} + + template ElfFile::ElfFile(FileContents fContents) : fileContents(fContents) @@ -259,14 +278,34 @@ ElfFile::ElfFile(FileContents fContents) if (rdi(hdr()->e_type) != ET_EXEC && rdi(hdr()->e_type) != ET_DYN) error("wrong ELF type"); - if (rdi(hdr()->e_phoff) + rdi(hdr()->e_phnum) * rdi(hdr()->e_phentsize) > fileContents->size()) - error("program header table out of bounds"); + { + auto ph_offset = rdi(hdr()->e_phoff); + auto ph_num = rdi(hdr()->e_phnum); + auto ph_entsize = rdi(hdr()->e_phentsize); + size_t ph_size, ph_end; + + if (__builtin_mul_overflow(ph_num, ph_entsize, &ph_size) + || __builtin_add_overflow(ph_offset, ph_size, &ph_end) + || ph_end > fileContents->size()) { + error("program header table out of bounds"); + } + } if (rdi(hdr()->e_shnum) == 0) error("no section headers. The input file is probably a statically linked, self-decompressing binary"); - if (rdi(hdr()->e_shoff) + rdi(hdr()->e_shnum) * rdi(hdr()->e_shentsize) > fileContents->size()) - error("section header table out of bounds"); + { + auto sh_offset = rdi(hdr()->e_shoff); + auto sh_num = rdi(hdr()->e_shnum); + auto sh_entsize = rdi(hdr()->e_shentsize); + size_t sh_size, sh_end; + + if (__builtin_mul_overflow(sh_num, sh_entsize, &sh_size) + || __builtin_add_overflow(sh_offset, sh_size, &sh_end) + || sh_end > fileContents->size()) { + error("section header table out of bounds"); + } + } if (rdi(hdr()->e_phentsize) != sizeof(Elf_Phdr)) error("program headers have wrong size"); @@ -295,7 +334,10 @@ ElfFile::ElfFile(FileContents fContents) error("string table index out of bounds"); auto shstrtabSize = rdi(shdrs[shstrtabIndex].sh_size); - char * shstrtab = (char * ) fileContents->data() + rdi(shdrs[shstrtabIndex].sh_offset); + size_t shstrtabptr; + if (__builtin_add_overflow(reinterpret_cast(fileContents->data()), rdi(shdrs[shstrtabIndex].sh_offset), &shstrtabptr)) + error("string table overflow"); + const char *shstrtab = reinterpret_cast(shstrtabptr); checkPointer(fileContents, shstrtab, shstrtabSize); if (shstrtabSize == 0) @@ -583,7 +625,7 @@ std::string & ElfFile::replaceSection(const SectionName & sec s = std::string(i->second); } else { auto shdr = findSectionHeader(sectionName); - s = std::string((char *) fileContents->data() + rdi(shdr.sh_offset), rdi(shdr.sh_size)); + s = extractString(fileContents, rdi(shdr.sh_offset), rdi(shdr.sh_size)); } s.resize(size); @@ -1193,7 +1235,10 @@ template std::string ElfFile::getInterpreter() const { auto shdr = findSectionHeader(".interp"); - return std::string((const char *) fileContents->data() + rdi(shdr.sh_offset), rdi(shdr.sh_size) - 1); + auto size = rdi(shdr.sh_size); + if (size > 0) + size--; + return extractString(fileContents, rdi(shdr.sh_offset), size); } template -- cgit v1.2.1