segfault following assert fail at patchelf.cc:556

[jimav]

patchelf segfauilts after printing an assertion error message, when applied a particular file, which I will attach. It seems to occur when --no-default-lib is used with both -set-interpreter and --set-rpath arguments of a certain minimum length.

Steps to reproduce

Here is a cut-down test which reproduces on my system (if it doesn't repro for you, please try making the /aaaa... and /bbb... much longer). /tmp/testprog is the file I will attach:

$ /tmp/patchelf/src/patchelf --no-default-lib --set-interpreter /aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa --set-rpath /bbbbbbbbbbbbb /tmp/testprog
patchelf: patchelf.cc:556: void ElfFile<Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Versym, Elf_Verdef, Elf_Verdaux, Elf_Verneed, Elf_Vernaux, Elf_Rel, Elf_Rela, ElfClass>::shiftFile(unsigned int, size_t, size_t) [with Elf_Ehdr = Elf64_Ehdr; Elf_Phdr = Elf64_Phdr; Elf_Shdr = Elf64_Shdr; Elf_Addr = long unsigned int; Elf_Off = long unsigned int; Elf_Dyn = Elf64_Dyn; Elf_Sym = Elf64_Sym; Elf_Versym = short unsigned int; Elf_Verdef = Elf64_Verdef; Elf_Verdaux = Elf64_Verdaux; Elf_Verneed = Elf64_Verneed; Elf_Vernaux = Elf64_Vernaux; Elf_Rel = Elf64_Rel; Elf_Rela = Elf64_Rela; unsigned int ElfClass = 64; size_t = long unsigned int]: Assertion `splitIndex != -1' failed.
Aborted (core dumped)

Here is a gdb backtrace:

__pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
warning: 44	./nptl/pthread_kill.c: No such file or directory
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff784526e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff78288ff in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff782881b in __assert_fail_base (fmt=0x7ffff79d01e8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x55555558a144 "splitIndex != -1", file=file@entry=0x555555589b99 "patchelf.cc", line=line@entry=556, 
    function=function@entry=0x555555587e60 "void ElfFile<Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Versym, Elf_Verdef, Elf_Verdaux, Elf_Verneed, Elf_Vernaux, Elf_Rel, Elf_Rela, ElfClass>::shiftFile(unsigned int, siz"...) at ./assert/assert.c:94
#6  0x00007ffff783b507 in __assert_fail (assertion=0x55555558a144 "splitIndex != -1", file=0x555555589b99 "patchelf.cc", line=556, 
    function=0x555555587e60 "void ElfFile<Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Versym, Elf_Verdef, Elf_Verdaux, Elf_Verneed, Elf_Vernaux, Elf_Rel, Elf_Rela, ElfClass>::shiftFile(unsigned int, siz"...) at ./assert/assert.c:103
#7  0x000055555556d0ec in ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, unsigned short, Elf64_Verdef, Elf64_Verdaux, Elf64_Verneed, Elf64_Vernaux, Elf64_Rel, Elf64_Rela, 64u>::shiftFile (this=this@entry=0x7fffffffd320, extraPages=extraPages@entry=2, 
    startOffset=startOffset@entry=6672, extraBytes=extraBytes@entry=568) at patchelf.cc:556
#8  0x0000555555582675 in ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, unsigned short, Elf64_Verdef, Elf64_Verdaux, Elf64_Verneed, Elf64_Vernaux, Elf64_Rel, Elf64_Rela, 64u>::rewriteSectionsExecutable (this=this@entry=0x7fffffffd320) at patchelf.cc:1078
#9  0x00005555555828f9 in ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, unsigned short, Elf64_Verdef, Elf64_Verdaux, Elf64_Verneed, Elf64_Vernaux, Elf64_Rel, Elf64_Rela, 64u>::rewriteSections (this=this@entry=0x7fffffffd320, force=force@entry=false)
    at patchelf.cc:1198
#10 0x0000555555584f82 in ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, unsigned short, Elf64_Verdef, Elf64_Verdaux, Elf64_Verneed, Elf64_Vernaux, Elf64_Rel, Elf64_Rela, 64u>::noDefaultLib (this=this@entry=0x7fffffffd320) at patchelf.cc:2022
#11 0x000055555555c804 in patchElf2<ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, unsigned short, Elf64_Verdef, Elf64_Verdaux, Elf64_Verneed, Elf64_Vernaux, Elf64_Rel, Elf64_Rela, 64> > (fileName="/tmp/testprog", 
    fileContents=std::shared_ptr<std::vector<unsigned char, std::allocator<unsigned char> >> (use count 3, weak count 0) = {...}, elfFile=...)
    at patchelf.cc:2493
#12 patchElf () at patchelf.cc:2522
#13 mainWrapped (argc=<optimized out>, argv=<optimized out>) at patchelf.cc:2746
#14 0x000055555555850a in main (argc=<optimized out>, argv=<optimized out>) at patchelf.cc:2759

Build Version

I compiled patchelf from git source current master, 739a486, using gcc 13.3.0 on Ubuntu 24.04.1 LTS.

The compiled patchelf says it is version 0.18.0

The Test File

This is a copy of /usr/bin/x86_64-linux-gnu-cpp-13 i.e. gcc C Preprocessor executable (which I'm trying to patch to work on another server where I don't have root access to properly install anything...)

[Grumble... github would not let me attach the executable directly; so I put it in a tarball]

testprog.tar.gz

[mjklbhvg]

I encountered a similar bug to this, also hitting the assertions in shiftFile. As I understand it, these asserts make sure that the "shifted" pages are covered by exactly one LOAD segment.
What is odd to me is, that in patchelf.cc:566 the size of the extra LOAD segment is calculated using the extraBytes instead of the "real" shift amount. It looks like this creates a gap that is not covered by any load segment, because extraBytes <= shift.
Hitting this gap in a later shiftFile call will cause this crash.

This patch seems to fix this particular issue:

diff --git a/src/patchelf.cc b/src/patchelf.cc
index c8fb56a..9867147 100644
--- a/src/patchelf.cc
+++ b/src/patchelf.cc
@@ -563,7 +563,7 @@ void ElfFile<ElfFileParamNames>::shiftFile(unsigned int extraPages, size_t start
     wri(phdr.p_offset, phdrs.at(splitIndex).p_offset - splitShift - shift);
     wri(phdr.p_paddr, phdrs.at(splitIndex).p_paddr - splitShift - shift);
     wri(phdr.p_vaddr, phdrs.at(splitIndex).p_vaddr - splitShift - shift);
-    wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + extraBytes));
+    wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + shift));
     wri(phdr.p_flags, PF_R | PF_W);
     wri(phdr.p_align, getPageSize());
 }
@@ -1083,22 +1083,6 @@ void ElfFile<ElfFileParamNames>::rewriteSectionsExecutable()

     Elf_Off curOff = sizeof(Elf_Ehdr) + phdrs.size() * sizeof(Elf_Phdr);

-    /* Ensure PHDR is covered by a LOAD segment.
-
-       Because PHDR is supposed to have been covered by such section before, in
-       here we assume that we don't have to create any new section, but rather
-       extend the existing one. */
-    for (auto& phdr : phdrs)
-        if (rdi(phdr.p_type) == PT_LOAD &&
-            rdi(phdr.p_offset) <= curOff &&
-            rdi(phdr.p_offset) + rdi(phdr.p_filesz) > curOff &&
-            rdi(phdr.p_filesz) < neededSpace)
-        {
-            wri(phdr.p_filesz, neededSpace);
-            wri(phdr.p_memsz, neededSpace);
-            break;
-        }
-
     /* Clear out the free space. */
     debug("clearing first %d bytes\n", startOffset - curOff);
     memset(fileContents->data() + curOff, 0, startOffset - curOff);

I do not understand patchelf well enough to judge if this is a reasonable fix though.
This also removes the only usage of the extraBytes argument in shiftFile.
I removed the "Ensure PHDR is covered by LOAD" code because it created overlapping LOAD segments
with the change in shiftFile.

To at least somewhat test this patch I used the following "fuzzing" script using the test binary from this
issue (sorry for using fish):

#!/usr/bin/fish

if not test -f testprog
    wget https://github.com/user-attachments/files/18447000/testprog.tar.gz
    tar -xvf testprog.tar.gz
end
cp testprog /tmp/testprog_backup
while true
    cp /tmp/testprog_backup /tmp/testprog
    set cmdline "./src/patchelf "
    for i in $(seq 10)
        set -a cmdline (random choice --set-interpreter --set-soname --add-rpath --add-needed)
        set -a cmdline (string repeat (random 1 10000) A)
    end

    set -a cmdline /tmp/testprog
    if not eval $cmdline
        echo $cmdline > crash.sh
        echo FOUND CRASH `sh crash.sh`
        break
    else
        echo OK
    end
end