Hi Mark,
On Tue, Aug 18, 2026 at 8:52 AM Mark Wielaard <[email protected]> wrote:
>
> elf_newscn would just add a new section without making sure all
> existing section data was read in. This would confuse other library
> code that tried to make sure all section data was read. The number of
> sections would no longer match the number of sections on disk and the
> internal load_shdr_wrlock would try to read too many section data from
> disk (and fail because the data wasn't there). In particular this
> would confuse elf_update which assumed that all section data is there.
>
> * libelf/elf_newscn.c (elf_newscn): Explicitly get the data
> for section one (if it exists) to load all existing section
> data into memory.
> * tests/addnewscn.c: New test.
> * tests/run-addnewscn.sh: New test runner.
> * Makefile.am (check_PROGRAMS): Add addnewscn.
> (TESTS): Add run-addnewscn.sh.
> (EXTRA_DIST): Likewise.
> (addnewscn_LDADD): New variable.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=34412
>
> Signed-off-by: Mark Wielaard <[email protected]>
> ---
> libelf/elf_newscn.c | 23 ++++++
> tests/Makefile.am | 4 ++
> tests/addnewscn.c | 159 +++++++++++++++++++++++++++++++++++++++++
> tests/run-addnewscn.sh | 28 ++++++++
> 4 files changed, 214 insertions(+)
> create mode 100644 tests/addnewscn.c
> create mode 100755 tests/run-addnewscn.sh
>
> diff --git a/libelf/elf_newscn.c b/libelf/elf_newscn.c
> index f7b20ab7cb2d..d7bc9054e34f 100644
> --- a/libelf/elf_newscn.c
> +++ b/libelf/elf_newscn.c
> @@ -1,5 +1,6 @@
> /* Append new section.
> Copyright (C) 1998,1999,2000,2001,2002,2005,2009,2014,2015 Red Hat, Inc.
> + Copyright (C) 2026 Mark J. Wielaard <[email protected]>
> This file is part of elfutils.
> Written by Ulrich Drepper <[email protected]>, 1998.
>
> @@ -60,6 +61,28 @@ elf_newscn (Elf *elf)
>
> rwlock_wrlock (elf->lock);
>
> + /* Make sure any existing sections have been read in. */
> + struct Elf_Scn *scn1 = NULL;
> + Elf_ScnList *list = (elf->class == ELFCLASS32
> + ? &elf->state.elf32.scns
> + : &elf->state.elf64.scns);
> +
> + /* Find the first section. */
> + if (list->cnt > 1)
> + scn1 = &list->data[1];
> + else if (list->next != NULL)
> + scn1 = &list->next->data[0];
If there's just one section then preload doesnt occur and the segfault
can still happen. AFAICT it isn't possible to create a 1 section elf
file with libelf, gcc or llvm but handling this case could protect
against some malformed inputs as well as trivial (yet spec-conforming)
binaries with only an SHT_NULL section. Otherwise this patch LGTM.
Aaron
> +
> + /* Load the section headers if necessary. Getting the header for
> + one section loads the headers for all sections. */
> + if (scn1 != NULL)
> + {
> + if (elf->class == ELFCLASS32 && scn1->shdr.e32 == NULL)
> + (void) __elf32_getshdr_wrlock (scn1);
> + else if (elf->class == ELFCLASS64 && scn1->shdr.e64 == NULL)
> + (void) __elf64_getshdr_wrlock (scn1);
> + }
> +
> again:
> if (elf->state.elf.scns_last->cnt < elf->state.elf.scns_last->max)
> {
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 137e96163928..ac21c59aab8e 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -60,6 +60,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx
> scnnames sectiondump \
> fillfile dwarf_default_lower_bound \
> dwarf_language_lower_bound dwarf-die-addr-die \
> dwarf-type \
> + addnewscn \
> get-units-invalid get-units-split attr-integrate-skel \
> all-dwarf-ranges unit-info next_cfi \
> elfcopy addsections xlate_notes elfrdwrnop \
> @@ -218,6 +219,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh
> newfile test-nlist \
> dwarf_language_lower_bound \
> run-dwarf-die-addr-die.sh \
> run-dwarf-type.sh \
> + run-addnewscn.sh \
> run-get-units-invalid.sh run-get-units-split.sh \
> run-attr-integrate-skel.sh \
> run-all-dwarf-ranges.sh run-unit-info.sh \
> @@ -603,6 +605,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
> testfile-m68k-core.bz2 testfile-m68k.bz2 testfile-m68k-s.bz2 \
> run-dwarf-die-addr-die.sh \
> run-dwarf-type.sh \
> + run-addnewscn.sh \
> run-get-units-invalid.sh run-get-units-split.sh \
> testfile-hello4.dwo.bz2 testfile-hello5.dwo.bz2 \
> testfile-splitdwarf-4.bz2 testfile-splitdwarf-5.bz2 \
> @@ -808,6 +811,7 @@ endif
> libebl = ../libebl/libebl.a ../backends/libebl_backends.a ../libcpu/libcpu.a
> libeu = ../lib/libeu.a
>
> +addnewscn_LDADD = $(libelf)
> arextract_LDADD = $(libelf)
> arsymtest_LDADD = $(libelf)
> ar_extract_ar_LDADD = $(libelf)
> diff --git a/tests/addnewscn.c b/tests/addnewscn.c
> new file mode 100644
> index 000000000000..a3ba063b4e81
> --- /dev/null
> +++ b/tests/addnewscn.c
> @@ -0,0 +1,159 @@
> +/* 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/>. */
> +
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <libelf.h>
> +#include <gelf.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +
> +int
> +main (int argc, char **argv)
> +{
> + if (argc < 2)
> + {
> + fprintf (stderr, "Need at least one argument\n");
> + exit (1);
> + }
> +
> + if (elf_version (EV_CURRENT) == EV_NONE)
> + {
> + fprintf (stderr, "elf_version %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + const char *file = argv[1];
> + int fd = open (file, O_RDWR);
> + if (fd < 0)
> + {
> + fprintf (stderr, "Couldn't open file '%s': %s\n",
> + file, strerror (errno));
> + exit (1);
> + }
> +
> + Elf *elf = elf_begin (fd, ELF_C_RDWR, NULL);
> + if (elf == NULL)
> + {
> + fprintf (stderr, "Couldn't open ELF file '%s': %s\n",
> + file, elf_errmsg (-1));
> + exit (1);
> + }
> +
> + Elf_Scn *section = elf_newscn (elf);
> + if (section == NULL)
> + {
> + printf ("cannot create new section: %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + Elf_Data *data = elf_newdata (section);
> + data->d_buf = "frob";
> + data->d_size = strlen ("frob") + 1;
> +
> + GElf_Shdr shdr;
> + if (gelf_getshdr (section, &shdr) == NULL)
> + {
> + printf ("cannot get shdr of new section: %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + shdr.sh_name = 0;
> + shdr.sh_type = SHT_PROGBITS;
> + shdr.sh_flags = SHF_STRINGS;
> + shdr.sh_addr = 0;
> + shdr.sh_offset = 0;
> + shdr.sh_size = 0;
> + shdr.sh_link = 0;
> + shdr.sh_info = 0;
> + shdr.sh_addralign = 4;
> + shdr.sh_entsize = 1;
> +
> + if (gelf_update_shdr (section, &shdr) == 0)
> + {
> + printf ("cannot update section header: %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + if (elf_update (elf, ELF_C_WRITE) < 0)
> + {
> + printf ("cannot update elf: %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + elf_end (elf);
> + close (fd);
> +
> + /* Reopen. Recheck. */
> + fd = open (file, O_RDONLY);
> + if (fd < 0)
> + {
> + fprintf (stderr, "Couldn't reopen file '%s': %s\n",
> + file, strerror (errno));
> + exit (1);
> + }
> +
> + elf = elf_begin (fd, ELF_C_RDWR, NULL);
> + if (elf == NULL)
> + {
> + fprintf (stderr, "Couldn't reopen ELF file '%s': %s\n",
> + file, elf_errmsg (-1));
> + exit (1);
> + }
> +
> + size_t shnum;
> + if (elf_getshdrnum (elf, &shnum) < 0)
> + {
> + printf ("couldn't get shnum: %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + section = elf_getscn (elf, shnum -1 );
> + if (section == NULL)
> + {
> + printf ("cannot get last section: %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + if (gelf_getshdr (section, &shdr) == NULL)
> + {
> + printf ("cannot get shdr of last section: %s\n", elf_errmsg (-1));
> + exit (1);
> + }
> +
> + /* Must be the same as set. */
> + assert (shdr.sh_name == 0);
> + assert (shdr.sh_type == SHT_PROGBITS);
> + assert (shdr.sh_flags == SHF_STRINGS);
> + assert (shdr.sh_addr == 0);
> + assert (shdr.sh_link == 0);
> + assert (shdr.sh_info == 0);
> + assert (shdr.sh_addralign == 4);
> + assert (shdr.sh_entsize == 1);
> +
> + elf_end (elf);
> + close (fd);
> +
> + return 0;
> +}
> diff --git a/tests/run-addnewscn.sh b/tests/run-addnewscn.sh
> new file mode 100755
> index 000000000000..d9ff98ac2a72
> --- /dev/null
> +++ b/tests/run-addnewscn.sh
> @@ -0,0 +1,28 @@
> +#!/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
> +
> +# Four small 32/64, big/little endian ELF files to add an section to
> +testfiles testfile_gnu_props.32be.o testfile_gnu_props.32le.o
> +testfiles testfile_gnu_props.64be.o testfile_gnu_props.64le.o
> +
> +testrun ${abs_builddir}/addnewscn testfile_gnu_props.32le.o
> +testrun ${abs_builddir}/addnewscn testfile_gnu_props.32be.o
> +testrun ${abs_builddir}/addnewscn testfile_gnu_props.64le.o
> +testrun ${abs_builddir}/addnewscn testfile_gnu_props.64be.o
> +
> --
> 2.55.0
>
