Hi Aaron,
On Fri, 2023-11-17 at 21:48 -0500, Aaron Merey wrote:
> v1: https://sourceware.org/pipermail/elfutils-devel/2023q4/006644.html
>
> v2 changes:
>
> The size of the uncompressed testcore-noncontig has been reduced from
> 736M to 54K.
Uncompressed it is 580K. Still a bit large, but much more reasonable.
We even have a couple of larger test files in the repo. Thanks.
And in theory it can be replicated.
> dwfl_addrsegment tests have been added to verify correct handling of
> non-contiguous segments.
BTW. Adding extra comments after the --- makes it easier to post a
commit as you will apply it because comments after the --- will be
ignored by git am.
Please restore the original commit message before pushing.
The description of the issue was really good.
> ---
> libdwfl/dwfl_segment_report_module.c | 37 ++++++++++-----
> tests/.gitignore | 1 +
> tests/Makefile.am | 8 ++--
> tests/dwfl-core-noncontig.c | 67 +++++++++++++++++++++++++++
> tests/run-dwfl-core-noncontig.sh | 63 +++++++++++++++++++++++++
> tests/testcore-noncontig.bz2 | Bin 0 -> 54684 bytes
> 6 files changed, 162 insertions(+), 14 deletions(-)
> create mode 100644 tests/dwfl-core-noncontig.c
> create mode 100755 tests/run-dwfl-core-noncontig.sh
> create mode 100644 tests/testcore-noncontig.bz2
>
> diff --git a/libdwfl/dwfl_segment_report_module.c
> b/libdwfl/dwfl_segment_report_module.c
> index 3ef62a7d..09ee37b3 100644
> --- a/libdwfl/dwfl_segment_report_module.c
> +++ b/libdwfl/dwfl_segment_report_module.c
> @@ -737,17 +737,34 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
> char *name,
> && invalid_elf (module->elf, module->disk_file_has_build_id,
> &build_id))
> {
> - elf_end (module->elf);
> - close (module->fd);
> - module->elf = NULL;
> - module->fd = -1;
> + /* If MODULE's build-id doesn't match the disk file's
> + build-id, close ELF only if MODULE and ELF refer to
> + different builds of files with the same name. This
> + prevents premature closure of the correct ELF in cases
> + where segments of a module are non-contiguous in memory. */
> + if (name != NULL && module->name[0] != '\0'
> + && strcmp (basename (module->name), basename (name)) == 0)
> + {
> + elf_end (module->elf);
> + close (module->fd);
> + module->elf = NULL;
> + module->fd = -1;
> + }
> }
> - if (module->elf != NULL)
> + else if (module->elf != NULL)
> {
> - /* Ignore this found module if it would conflict in address
> - space with any already existing module of DWFL. */
> + /* This module has already been reported. */
> skip_this_module = true;
> }
> + else
> + {
> + /* Only report this module if we haven't already done so. */
> + for (Dwfl_Module *mod = dwfl->modulelist; mod != NULL;
> + mod = mod->next)
> + if (mod->low_addr == module_start
> + && mod->high_addr == module_end)
> + skip_this_module = true;
> + }
> }
> if (skip_this_module)
> goto out;
> @@ -781,10 +798,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
> char *name,
> }
> }
>
> - /* Our return value now says to skip the segments contained
> - within the module. */
> - ndx = addr_segndx (dwfl, segment, module_end, true);
> -
> /* Examine its .dynamic section to get more interesting details.
> If it has DT_SONAME, we'll use that as the module name.
> If it has a DT_DEBUG, then it's actually a PIE rather than a DSO.
> @@ -929,6 +942,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
> char *name,
> ndx = -1;
> goto out;
> }
> + else
> + ndx++;
>
> /* We have reported the module. Now let the caller decide whether we
> should read the whole thing in right now. */
Code looks the same.
> diff --git a/tests/.gitignore b/tests/.gitignore
> index b9aa22ba..5bebb2c4 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -49,6 +49,7 @@
> /dwfl-report-elf-align
> /dwfl-report-offline-memory
> /dwfl-report-segment-contiguous
> +/dwfl-core-noncontig
> /dwfllines
> /dwflmodtest
> /dwflsyms
Ack.
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 7fb8efb1..9f8f7698 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -42,7 +42,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx
> scnnames sectiondump \
> dwfl-bug-addr-overflow arls dwfl-bug-fd-leak \
> dwfl-addr-sect dwfl-bug-report early-offscn \
> dwfl-bug-getmodules dwarf-getmacros dwarf-ranges addrcfi \
> - dwarfcfi \
> + dwfl-core-noncontig dwarfcfi \
> test-flag-nobits dwarf-getstring rerequest_tag \
> alldts typeiter typeiter2 low_high_pc \
> test-elf_cntl_gelf_getshdr dwflsyms dwfllines \
> @@ -212,7 +212,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh
> newfile test-nlist \
> $(asm_TESTS) run-disasm-bpf.sh run-low_high_pc-dw-form-indirect.sh \
> run-nvidia-extended-linemap-libdw.sh
> run-nvidia-extended-linemap-readelf.sh \
> run-readelf-dw-form-indirect.sh run-strip-largealign.sh \
> - run-readelf-Dd.sh
> + run-readelf-Dd.sh run-dwfl-core-noncontig.sh
>
> if !BIARCH
> export ELFUTILS_DISABLE_BIARCH = 1
> @@ -632,7 +632,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
> run-nvidia-extended-linemap-libdw.sh
> run-nvidia-extended-linemap-readelf.sh \
> testfile_nvidia_linemap.bz2 \
> testfile-largealign.o.bz2 run-strip-largealign.sh \
> - run-funcretval++11.sh
> + run-funcretval++11.sh \
> + run-dwfl-core-noncontig.sh testcore-noncontig.bz2
>
>
> if USE_VALGRIND
> @@ -738,6 +739,7 @@ dwfl_bug_fd_leak_LDADD = $(libeu) $(libdw) $(libebl)
> $(libelf)
> dwfl_bug_report_LDADD = $(libdw) $(libebl) $(libelf)
> dwfl_bug_getmodules_LDADD = $(libeu) $(libdw) $(libebl) $(libelf)
> dwfl_addr_sect_LDADD = $(libeu) $(libdw) $(libebl) $(libelf) $(argp_LDADD)
> +dwfl_core_noncontig_LDADD = $(libdw) $(libelf)
> dwarf_getmacros_LDADD = $(libdw)
> dwarf_ranges_LDADD = $(libdw)
> dwarf_getstring_LDADD = $(libdw)
OK.
> diff --git a/tests/dwfl-core-noncontig.c b/tests/dwfl-core-noncontig.c
> new file mode 100644
> index 00000000..8db4cd18
> --- /dev/null
> +++ b/tests/dwfl-core-noncontig.c
> @@ -0,0 +1,67 @@
> +/* Test program for dwfl_getmodules bug.
> + Copyright (C) 2008 Red Hat, Inc.
> + 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/>. */
> +
> +#include <config.h>
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <assert.h>
> +#include ELFUTILS_HEADER(dwfl)
> +#include ELFUTILS_HEADER(elf)
> +
> +static const Dwfl_Callbacks cb =
> +{
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> +};
OK. No callbacks, so no lookups, just the reported core file will be
considered.
> +
> +int
> +main (int argc, char **argv)
> +{
> + assert (argc == 2);
> +
> + Dwfl *dwfl = dwfl_begin (&cb);
> +
> + int fd = open (argv[1], O_RDONLY);
> + assert (fd != -1);
> +
> + Elf *elf = elf_begin (fd, ELF_C_READ, NULL);
> + (void) dwfl_core_file_report (dwfl, elf, argv[0]);
> +
> + /* First segment of the non-contiguous module. */
> + int seg = dwfl_addrsegment (dwfl, 0x7f14e458c000, NULL);
> + assert (seg == 32);
> +
> + /* First seg of module within the non-contiguous module's address range.
> */
> + seg = dwfl_addrsegment (dwfl, 0x7f14e4795000, NULL);
> + assert (seg == 33);
> +
> + /* Last segment of the module within the non-contiguous module's
> + address range. */
> + seg = dwfl_addrsegment (dwfl, 0x7f14e47a0000, NULL);
> + assert (seg == 37);
> +
> + /* First segment of non-contiguous module following the address space gap.
> */
> + seg = dwfl_addrsegment (dwfl, 0x7f14e47ad000, NULL);
> + assert (seg == 40);
OK. This does make sense if you have seen the eu-readelf -l testcore-
noncontig output. Maybe that should be there in a comment?
> + dwfl_end (dwfl);
> + elf_end (elf);
> +
> + return 0;
> +}
> diff --git a/tests/run-dwfl-core-noncontig.sh
> b/tests/run-dwfl-core-noncontig.sh
> new file mode 100755
> index 00000000..1245b67f
> --- /dev/null
> +++ b/tests/run-dwfl-core-noncontig.sh
> @@ -0,0 +1,63 @@
> +#! /bin/sh
> +# Copyright (C) 2023 Red Hat, Inc.
> +# 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
> +
> +# Test whether libdwfl can handle corefiles containing non-contiguous
> +# segments where multiple modules are contained within the address
> +# space of some other module.
> +
> +# testcore-noncontig was generated from the following program with
> +# systemd-coredump on RHEL 7.9 Workstation, kernel
> +# 3.10.0-1160.105.1.el7.x86_64. liblgpllibs.so was packaged with
> +# firefox-115.4.0-1.el7_9.x86_64.rpm.
> +
> +# #include <unistd.h>
> +# #include <dlfcn.h>
> +#
> +# int main () {
> +# dlopen ("/usr/lib64/firefox/liblgpllibs.so", RTLD_GLOBAL | RTLD_NOW);
> +# sleep (60);
> +# return 0;
> +# }
> +#
> +# gcc -ldl -o test test.c
> +
> +tempfiles out
> +testfiles testcore-noncontig
> +
> +testrun ${abs_builddir}/dwfl-core-noncontig testcore-noncontig
> +
> +# Remove parts of the output that could change depending on which
> +# libraries are locally installed.
> +testrun ${abs_top_builddir}/src/unstrip -n --core testcore-noncontig \
> + | sed 's/+/ /g' | cut -d " " -f1,3 | sort > out
> +
> +testrun_compare cat out <<\EOF
> +0x400000 3a1748a544b40a38b3be3d2d13ffa34a2a5a71c0@0x400284
> +0x7f14e357e000 edf51350c7f71496149d064aa8b1441f786df88a@0x7f14e357e1d8
> +0x7f14e3794000 7615604eaf4a068dfae5085444d15c0dee93dfbd@0x7f14e37941d8
> +0x7f14e3a96000 09cfb171310110bc7ea9f4476c9fa044d85baff4@0x7f14e3a96210
> +0x7f14e3d9e000 e10cc8f2b932fc3daeda22f8dac5ebb969524e5b@0x7f14e3d9e248
> +0x7f14e3fba000 fc4fa58e47a5acc137eadb7689bce4357c557a96@0x7f14e3fba280
> +0x7f14e4388000 7f2e9cb0769d7e57bd669b485a74b537b63a57c4@0x7f14e43881d8
> +0x7f14e458c000 62c449974331341bb08dcce3859560a22af1e172@0x7f14e458c1d8
> +0x7f14e4795000 175efdcef445455872a86a6fbee7567ca16a513e@0x7f14e4795248
> +0x7ffcfe59f000 80d79b32785868a2dc10047b39a80d1daec8923d@0x7ffcfe59f328
> +EOF
> +
> +exit 0
Nice collection of tests.
> diff --git a/tests/testcore-noncontig.bz2 b/tests/testcore-noncontig.bz2
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..514ad010e00499b9a75a98800aa20ec61596d491
> GIT binary patch
OK.
All looks good. Maybe add the suggested comment about looking at the
program headers to undestand the testscase and please restore the
original commit message before applying.
Thanks,
Mark
