Hi Kane,
[Sending an HTML only patch is a bit difficult to apply, please try
sending in plain text next time.]
On Fri, 2026-07-03 at 17:20 +0800, 王强 wrote:
> read_number_entries () reads the number of entries of an archive symbol
> index. On the mmap path, taken for images created with elf_memory ()
> (and for mmap-backed files), it copied sizeof (union u) == 8 bytes
> instead of the width-appropriate 4 bytes for a 32-bit ("symdef") index.
Well spotted.
> For a minimal in-memory archive whose index member holds only the 4-byte
> count field, this read 4 bytes past the caller's buffer. Use w, matching
> the pread path and the 64-bit case, so both paths read exactly the bytes
> they need.
Yes, that seems to correct fix.
> * libelf/elf_getarsym.c (read_number_entries): Use w, not sizeof u,
> for the mmap memcpy.
> * tests/ar-getarsym-mem.c: New file.
> * tests/Makefile.am (check_PROGRAMS): Add ar-getarsym-mem.
> (TESTS): Likewise.
> (ar_getarsym_mem_LDADD): New variable.
> * tests/.gitignore: Add ar-getarsym-mem.
Thanks for adding a testcase.
> Signed-off-by: Kane Wang <[email protected]>
> ---
> libelf/elf_getarsym.c | 2 +-
> tests/.gitignore | 1 +
> tests/Makefile.am | 5 +-
> tests/ar-getarsym-mem.c | 143 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 148 insertions(+), 3 deletions(-)
> create mode 100644 tests/ar-getarsym-mem.c
>
> diff --git a/libelf/elf_getarsym.c b/libelf/elf_getarsym.c
> index 281f0c1c..88d184f9 100644
> --- a/libelf/elf_getarsym.c
> +++ b/libelf/elf_getarsym.c
> @@ -55,7 +55,7 @@ read_number_entries (uint64_t *nump, Elf *elf, size_t
> *offp, bool index64_p)
> if (elf->map_address != NULL)
> /* Use memcpy instead of pointer dereference so as not to assume the
> field is naturally aligned within the file. */
> - memcpy (&u, elf->map_address + *offp, sizeof u);
> + memcpy (&u, elf->map_address + *offp, w);
> else if ((size_t) pread_retry (elf->fildes, &u, w, *offp) != w)
> return -1;
OK. The rest of the function also uses w (not sizeof u) for size/offset
calculations.
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 4681024f..72b02a5f 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -12,6 +12,7 @@
> /arls
> /arsymtest
> /ar-extract-ar
> +/ar-getarsym-mem
> /asm-tst1
> /asm-tst2
> /asm-tst3
Ack. New testcase.
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 30a7b6ca..51fea2f6 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -68,7 +68,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx
> scnnames sectiondump \
> cu-dwp-section-info declfiles test-manyfuncs \
> eu_search_cfi eu_search_macros \
> eu_search_lines eu_search_die \
> - ar-extract-ar elf-from-memory \
> + ar-extract-ar ar-getarsym-mem elf-from-memory \
> $(asm_TESTS)
>
> asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
> @@ -239,7 +239,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh
> newfile test-nlist \
> run-test-manyfuncs.sh \
> run-eu-search-cfi.sh run-eu-search-macros.sh \
> run-eu-search-lines.sh run-eu-search-die.sh \
> - elf-from-memory
> + elf-from-memory ar-getarsym-mem
>
> if !BIARCH
> export ELFUTILS_DISABLE_BIARCH = 1
> @@ -782,6 +782,7 @@ libeu = ../lib/libeu.a
> arextract_LDADD = $(libelf)
> arsymtest_LDADD = $(libelf)
> ar_extract_ar_LDADD = $(libelf)
> +ar_getarsym_mem_LDADD = $(libelf)
> newfile_LDADD = $(libelf)
> saridx_LDADD = $(libeu) $(libelf)
> scnnames_LDADD = $(libelf)
OK. new testcase correctly added.
> diff --git a/tests/ar-getarsym-mem.c b/tests/ar-getarsym-mem.c
> new file mode 100644
> index 00000000..7b4892e1
> --- /dev/null
> +++ b/tests/ar-getarsym-mem.c
> @@ -0,0 +1,143 @@
> +/* Test elf_getarsym on a truncated in-memory 32-bit archive index.
> + Copyright (C) 2026 KylinSoft Co., Ltd.
> + 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/>. */
> +
> +/* elf_getarsym reads the number of entries of an archive symbol index
> + through read_number_entries (). On the mmap path, taken for images
> + created with elf_memory (), it used to copy sizeof (union { uint64_t;
> + uint32_t; }) == 8 bytes instead of the width-appropriate 4 bytes for
> + a 32-bit ("symdef") index. For a minimal archive whose index member
> + holds only the 4-byte count field, that copied 4 bytes past the end
> + of the caller's buffer.
> +
> + This reproduces by building such a minimal 32-bit archive in an
> + elf_memory () image placed at the very end of a page, with an
> + inaccessible guard page immediately following it. An unfixed build
> + reads into the guard page and crashes; a fixed build reads exactly 4
> + bytes and returns the (zero-entry) index normally. */
Thanks. Nice description.
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#include <ar.h>
> +#include <errno.h>
> +#include <libelf.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +/* SARMAG + sizeof (struct ar_hdr) + a 4-byte 32-bit index count. */
> +#define ARSZ (8 + 60 + 4)
> +
> +/* Build a minimal 32-bit ("symdef") archive whose index member consists
> + of only the count field, encoding zero entries. */
> +static void
> +build_archive (unsigned char *p)
> +{
> + memset (p, 0, ARSZ);
> + memcpy (p, "!<arch>\n", 8);
> +
> + struct ar_hdr *hdr = (struct ar_hdr *) (p + 8);
> + memset (hdr, ' ', sizeof *hdr);
> + memcpy (hdr->ar_name, "/ ", 16);
> + memcpy (hdr->ar_date, "0 ", 12);
> + memcpy (hdr->ar_uid, "0 ", 6);
> + memcpy (hdr->ar_gid, "0 ", 6);
> + memcpy (hdr->ar_mode, "100644 ", 8);
> + memcpy (hdr->ar_size, "4 ", 10);
> + memcpy (hdr->ar_fmag, "`\n", 2);
> +
> + /* 4-byte count, stored big-endian as on disk; zero entries. */
> + p[68] = 0;
> + p[69] = 0;
> + p[70] = 0;
> + p[71] = 0;
> +}
Nice minimal (empty) ar.
> +int
> +main (void)
> +{
> + int result = 1;
> +
> + long pgsz = sysconf (_SC_PAGESIZE);
> + if (pgsz <= 0 || (size_t) pgsz < ARSZ)
> + {
> + printf ("SKIP: unusable page size (%ld)\n", pgsz);
> + return 77;
> + }
> +
> + /* Map two pages and make the second one inaccessible. The archive is
> + placed at the end of the first page so that an 8-byte read of the
> + 4-byte count field at offset 68 crosses into the guard page. */
> + unsigned char *map = mmap (NULL, (size_t) pgsz * 2,
> + PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> + if (map == MAP_FAILED)
> + {
> + printf ("SKIP: mmap failed: %m\n");
> + return 77;
> + }
> + if (mprotect (map + pgsz, (size_t) pgsz, PROT_NONE) != 0)
> + {
> + printf ("SKIP: mprotect failed: %m\n");
> + munmap (map, (size_t) pgsz * 2);
> + return 77;
> + }
> +
> + unsigned char *buf = map + pgsz - ARSZ;
> + build_archive (buf);
Nice trick to get the ar exactly at the edge of a page.
> + if (elf_version (EV_CURRENT) == EV_NONE)
> + {
> + printf ("FAIL: elf_version: %s\n", elf_errmsg (-1));
> + goto out;
> + }
> +
> + Elf *elf = elf_memory ((char *) buf, ARSZ);
> + if (elf == NULL)
> + {
> + printf ("FAIL: elf_memory: %s\n", elf_errmsg (-1));
> + goto out;
> + }
> +
> + size_t narsym = (size_t) -1;
> + Elf_Arsym *arsym = elf_getarsym (elf, &narsym);
> + /* A zero-entry index still gets the terminating sentinel entry, so
> + the table is non-NULL and the reported count is one. */
> + if (arsym == NULL || narsym != 1
> + || arsym[0].as_name != NULL || arsym[0].as_off != 0
> + || arsym[0].as_hash != ~0UL)
> + {
> + printf ("FAIL: unexpected arsym (narsym=%zu, arsym=%p)\n",
> + narsym, (void *) arsym);
> + result = 1;
> + }
Good check. This indeed fails without your fix. valgrind also flags it.
And it also shows another bug when building with --enable-sanitize-
undefined:
elfutils/libelf/elf_getarsym.c:264:15: runtime error: variable length
array bound evaluates to non-positive value 0
Which is:
/* Now we can build the data structure. */
Elf_Arsym *arsym = elf->state.ar.ar_sym;
uint64_t (*u64)[n] = file_data;
uint32_t (*u32)[n] = file_data;
for (size_t cnt = 0; cnt < n; ++cnt)
That [n] should be [n+1] because even if we won't enter that loop we
are lying to the compiler about the size of the array (there is indeed
space for n+1 entries, which includes the sentinel). We do add that
sentinel at the end to arsym after the loop.
I'll add:
diff --git a/libelf/elf_getarsym.c b/libelf/elf_getarsym.c
index 343c00ef352d..346596d79506 100644
--- a/libelf/elf_getarsym.c
+++ b/libelf/elf_getarsym.c
@@ -261,8 +261,8 @@ elf_getarsym (Elf *elf, size_t *ptr)
/* Now we can build the data structure. */
Elf_Arsym *arsym = elf->state.ar.ar_sym;
- uint64_t (*u64)[n] = file_data;
- uint32_t (*u32)[n] = file_data;
+ uint64_t (*u64)[n+1] = file_data;
+ uint32_t (*u32)[n+1] = file_data;
for (size_t cnt = 0; cnt < n; ++cnt)
{
arsym[cnt].as_name = str_data;
https://sourceware.org/cgit/elfutils/patch/?id=60cb1486c6fb
> + else
> + {
> + printf ("PASS: truncated 32-bit index read within bounds\n");
> + result = 0;
> + }
> +
> + elf_end (elf);
> +
> +out:
> + munmap (map, (size_t) pgsz * 2);
> + return result;
> +}
> --
> 2.43.0
>
Looks good. Pushed as
https://sourceware.org/cgit/elfutils/patch/?id=8b1921607772
Thanks,
Mark
