Hi Mark, On Tue, Oct 27, 2026 at 2:44 PM Mark Wielaard <[email protected]> wrote: > > When the section headers aren't at the end of the ELF file elf_update > might overwrite the section headers while creating fill between the > sections. For ELF_C_RDWR_MMAP this mostly worked correctly, but might > get the fill wrong if there was no extra space between the end of the > section and the start of the section headers. For ELF_C_RDWR the code > didn't take the shdrs into account at all and would always overwrite > them. > > Fix fill_mmap to not fill from the section start, but from the > last_position if that comes after the section headers table. Introduce > a fill_file function to use if the ELF file isn't mmapped. > > We already had an test program elfrdwrnop that wrote out an existing > ELF file as is. It did however not really write anything since it > didn't alter anything. The test now explicitly marks some (existing) > data as ELF_F_DIRTY to force elf_update to actually (re)write the > data. It now also does that with ELF_C_RDWR and then again with > ELF_C_RDWR_MMAP. This made tests/run-reverse-sections.sh file without > the fixes. > > Also add an explicit testfile that has the sections in the middle of > some sections. And add an explicit run-rdwrnop.sh testcase for this > file. > > * libelf/elf32_updatefile.c (fill_mmap): Fix fill_start after > shdr_end. > (fill_file): New function. > (updatefile): Calculate shdr_endoff. Call fill_file instead of > fill for data and after section. > * tests/elfrdwrnop.c (test): New function split off from... > (main): here. Call test twice. > * tests/run-rdwrnop.sh: New test runner. > * tests/testfile-elfsection-gap.so.bz2: New test file. > * tests/Makefile.am (TESTS): Add run-rdwrnop.sh. > (EXTRA_DIST): Add run-rdwrnop.sh and > testfile-elfsection-gap.so.bz2. > > https://sourceware.org/bugzilla/show_bug.cgi?id=34468 > > Signed-off-by: Mark Wielaard <[email protected]>
LGTM. Aaron > --- > libelf/elf32_updatefile.c | 47 ++++++++++++++++++++----- > tests/Makefile.am | 2 ++ > tests/elfrdwrnop.c | 27 +++++++++++++-- > tests/run-rdwrnop.sh | 49 +++++++++++++++++++++++++++ > tests/testfile-elfsection-gap.so.bz2 | Bin 0 -> 646 bytes > 5 files changed, 115 insertions(+), 10 deletions(-) > create mode 100755 tests/run-rdwrnop.sh > create mode 100755 tests/testfile-elfsection-gap.so.bz2 > > diff --git a/libelf/elf32_updatefile.c b/libelf/elf32_updatefile.c > index 46afa1f4e275..a4cda63630e4 100644 > --- a/libelf/elf32_updatefile.c > +++ b/libelf/elf32_updatefile.c > @@ -1,5 +1,6 @@ > /* Write changed data structures. > Copyright (C) 2000-2010, 2014, 2015, 2016, 2018 Red Hat, Inc. > + Copyright (C) 2026 Mark J. Wielaard <[email protected]> > This file is part of elfutils. > Written by Ulrich Drepper <[email protected]>, 2000. > > @@ -114,7 +115,7 @@ fill_mmap (size_t offset, char *last_position, char > *scn_start, > if (last_position + written != scn_start + offset > && shdr_end < scn_start + offset) > { > - char *fill_start = MAX (shdr_end, scn_start); > + char *fill_start = MAX (shdr_end, last_position + written); > memset (fill_start, __libelf_fill_byte, > scn_start + offset - fill_start); > } > @@ -527,6 +528,34 @@ fill (int fd, int64_t pos, size_t len, char *fillbuf, > size_t *filledp) > } > > > +static inline int > +fill_file (int fd, size_t offset, int64_t last_offset, > + int64_t scn_off, int64_t shdrs_start, int64_t shdrs_end, > + char *fillbuf, size_t *filledp) > +{ > + size_t written = 0; > + > + if (last_offset < shdrs_start) > + { > + written = MIN (scn_off + offset - last_offset, > + shdrs_start - last_offset); > + if (fill (fd, last_offset, written, fillbuf, filledp) != 0) > + return -1; > + } > + > + if (last_offset + written != scn_off + offset > + && shdrs_end < scn_off + (int64_t) offset) > + { > + size_t fill_off = MAX (shdrs_end, last_offset + written); > + if (fill (fd, fill_off, scn_off + offset - fill_off, > + fillbuf, filledp) != 0) > + return -1; > + } > + > + return 0; > +} > + > + > int > internal_function > __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum) > @@ -662,6 +691,8 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, > size_t shnum) > return 1; > > int64_t shdr_offset = elf->start_offset + ehdr->e_shoff; > + int64_t shdr_endoff = shdr_offset + shnum * ehdr->e_shentsize; > + > #undef shdr_fctp > #define shdr_fctp __elf_xfctstom[ELFW(ELFCLASS, LIBELFBITS) - 1][ELF_T_SHDR] > > @@ -722,10 +753,10 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int > change_bo, size_t shnum) > || ((scn->flags | dl->flags | elf->flags) > & ELF_F_DIRTY) != 0)) > { > - if (unlikely (fill (elf->fildes, last_offset, > - (scn_start + dl->data.d.d_off) > - - last_offset, fillbuf, > - &filled) != 0)) > + if (unlikely (fill_file (elf->fildes, dl->data.d.d_off, > + last_offset, scn_start, > + shdr_offset, shdr_endoff, > + fillbuf, &filled) != 0)) > { > fail_free: > free (shdr_data_mem); > @@ -797,9 +828,9 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, > size_t shnum) > header) changed we might have to fill the gap. */ > if (scn_start > last_offset && previous_scn_changed) > { > - if (unlikely (fill (elf->fildes, last_offset, > - scn_start - last_offset, fillbuf, > - &filled) != 0)) > + if (unlikely (fill_file (elf->fildes, 0, last_offset, > + scn_start, shdr_offset, > shdr_endoff, > + fillbuf, &filled) != 0)) > goto fail_free; > } > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 261649fd0d09..638f13ae0278 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -225,6 +225,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh > newfile test-nlist \ > run-reloc-bpf.sh \ > run-next-cfi.sh run-next-cfi-self.sh \ > run-reverse-sections.sh run-reverse-sections-self.sh \ > + run-rdwrnop.sh \ > run-copyadd-sections.sh \ > run-copymany-be32.sh run-copymany-le32.sh \ > run-copymany-be64.sh run-copymany-le64.sh \ > @@ -621,6 +622,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \ > testfile-riscv64.bz2 testfile-riscv64-s.bz2 \ > testfile-riscv64-core.bz2 \ > run-reverse-sections.sh run-reverse-sections-self.sh \ > + run-rdwrnop.sh testfile-elfsection-gap.so.bz2 \ > run-copyadd-sections.sh test-copymany-subr.sh \ > run-copymany-be32.sh run-copymany-le32.sh \ > run-copymany-be64.sh run-copymany-le64.sh \ > diff --git a/tests/elfrdwrnop.c b/tests/elfrdwrnop.c > index 997150b958cd..415f4a49719c 100644 > --- a/tests/elfrdwrnop.c > +++ b/tests/elfrdwrnop.c > @@ -34,6 +34,7 @@ > #include ELFUTILS_HEADER(elf) > #include <gelf.h> > > +static int test (const char *name, int mode); > > int > main (int argc, const char *argv[]) > @@ -48,8 +49,15 @@ main (int argc, const char *argv[]) > elf_version (EV_CURRENT); > > const char *name = argv[1]; > - printf ("elfrdwrdnop %s\n", name); > + printf ("elfrdwrdnop %s (RDWR)\n", name); > + test (name, ELF_C_RDWR); > + printf ("elfrdwrdnop %s (RDWR_MMAP)\n", name); > + test (name, ELF_C_RDWR_MMAP); > +} > > +static int > +test (const char *name, int elf_mode) > +{ > int fd = open (name, O_RDWR); > if (fd < 0) > { > @@ -58,7 +66,7 @@ main (int argc, const char *argv[]) > exit (1); > } > > - Elf *elf = elf_begin (fd, ELF_C_RDWR, NULL); > + Elf *elf = elf_begin (fd, elf_mode, NULL); > if (elf == NULL) > { > fprintf (stderr, "Couldn't open ELF file '%s': %s\n", > @@ -78,6 +86,21 @@ main (int argc, const char *argv[]) > if (phnum > 0) > elf_flagelf (elf, ELF_C_SET, ELF_F_LAYOUT); > > + /* Go through all sections and mark some data dirty, so it > + has to be rewritten. */ > + Elf_Scn *scn = NULL; > + int c = 0; > + while ((scn = elf_nextscn (elf, scn)) != NULL) > + { > + c++; > + if (c % 2 == 0) > + { > + Elf_Data *data = elf_getdata (scn, NULL); > + if (data != NULL) > + elf_flagdata (data, ELF_C_SET, ELF_F_DIRTY); > + } > + } > + > if (elf_update (elf, ELF_C_WRITE) < 0) > { > printf ("failure in elf_update: %s\n", elf_errmsg (-1)); > diff --git a/tests/run-rdwrnop.sh b/tests/run-rdwrnop.sh > new file mode 100755 > index 000000000000..ea3df02340a1 > --- /dev/null > +++ b/tests/run-rdwrnop.sh > @@ -0,0 +1,49 @@ > +#! /bin/sh > +# Copyright (C) 2026 Mark J. Wielaard <[email protected]> > +# This file is part of elfutils. > +# > +# This file is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# elfutils is distributed in the hope that it will be useful, but > +# WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +. $srcdir/test-subr.sh > + > +# Create an ELF file that has the section headers in the middle of > +# the file with some sections before the section table and some after. > +# It has phdrs, so when (re)writing it we want to use ELF_F_LAYOUT. > +# > +# echo "" | gcc -shared -nostdlib -s -Wl,-soname,libshort.so \ > +# -o testfile-elfsection-gap.so -xc - > +# patchelf --debug --set-soname large-testfile-soname.so.9 \ > +# testfile-elfsection-gap.so > +# patching ELF file 'testfile-elfsection-gap.so' > +# new SONAME is 'large-testfile-soname.so.9' > +# SONAME is too long, resizing... > +# replacing section '.dynstr' with size 40 > +# this is a dynamic library > +# last page is 0x3000 > +# first page is 0x0 > +# needed space is 720 > +# allocating new PT_LOAD segment > +# rewriting sht from offset 0x2098 to offset 0x3000 (size 640) > +# rewriting section '.note.gnu.build-id' from offset 0x200 (size 36) to > offset 0x3280 (size 36) > +# rewriting section '.dynstr' from offset 0x1038 (size 13) to offset 0x32a8 > (size 40) > +# rewriting symbol table section 2 > +# writing testfile-elfsection-gap.so > + > +testfiles testfile-elfsection-gap.so > +testrun ${abs_builddir}/elfrdwrnop testfile-elfsection-gap.so > +testrun ${abs_top_builddir}/src/readelf -S testfile-elfsection-gap.so \ > + | grep -q "<corrupt>" && exit 1 > +echo $? > + > +exit 0 > diff --git a/tests/testfile-elfsection-gap.so.bz2 > b/tests/testfile-elfsection-gap.so.bz2 > new file mode 100755 > index > 0000000000000000000000000000000000000000..6d1eb117e40b9f96fb90573b4724754dbf17c328 > GIT binary patch > literal 646 > zcmV;10(t#HT4*^jL0KkKS@2l13;+W-fB*mg)z!rB@o`_pJ`um?-eAB0K!E^2AV5eE > zKtcim0~x>qJ(n4$0MP;(O-G~}WB@V%0Lah)06joxdY+*4H3Xp$2ux~vp|sS%1Y!+6 > zMgR!F0007D2^5MN8a+cz0iXZ?8UO$WKmY?k0VtYO@*~vH&@?mv01W`p0jH=M0Ad3m > z(Fa7iX_BBUDoq&-a{Uz?fWo{UJu)D&JrymR0hRZcQ%6Mbx_Mm{^~>Z+VR1vscNBtA > z^o0n?2Kg;Cxn@)CJgbOnM70U4#6>&BW(O4tgcYH)zv()NfcRNQ8Uv<qk_m)ZWPx0- > z5d?^ND8_gIG@B8S!dUi;SVcUnArUl!LX_K441}_DkrB3Hj1ZE?Xj!aD6WB~)BP5pq > zNd@CpS8xy;{d}}~C{Uv)DrU}!1#`<{NVh4JE8IhFlOr1%CNPC749*aYf%=aT1Z70B > zpa9Yg*y51A^@T)oqO&3oMHG6Up^m2;kVLLh3hao3pWx!z^{{NH;Yh)h2Bp0S<{nj& > zk*r06RdFiOP~A32VSrLRyvthFA&Dbqq)=$zLsm&J2@N7MT)*j+2$fldTW3%OP?Unh > z3_7&aP**JXb_%GwP!MI%Ni5W<v@ylE4W-FcBMyTFBSW3mWQ%E|b)cUK1`sZyX(fvc > zoIFcNqFAqsUNIW9{v*LVgdyUn>WhILs|m2QeJDpjLc)>sAU^d?2_b}TbKQqeP$qCO > z1>sF{VDZj<8bPh0J`3fl1#~!9s(Gdu_>`I)fRw^-%_tdEsHm{fEOykhn!*xh0pSnJ > gU#ky_a?545m@Sw9haAw;6^Hn{k}1N3hl0hJVEhXVt^fc4 > > literal 0 > HcmV?d00001 > > -- > 2.56.0 >
