| Message ID | 20190417144233.2470-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] elf: Fix pldd (BZ#18035) | expand |
Ping. On 17/04/2019 11:42, Adhemerval Zanella wrote: > Changes from previous version: > > - Removal of more braces. > - Added a comment where the old code was removed to warn against looking > at l_libname. > - Adjusted test comment. > - Remove file artifact. > > --- > > Since 9182aa67994 (Fix vDSO l_name for GDB's, BZ#387) the initial link_map > for executable itself and loader will have both l_name and l_libname->name > holding the same value due: > > elf/dl-object.c > > 95 new->l_name = *realname ? realname : (char *) newname->name + libname_len - 1; > > Since newname->name points to new->l_libname->name. > > This leads to pldd to an infinite call at: > > elf/pldd-xx.c > > 203 again: > 204 while (1) > 205 { > 206 ssize_t n = pread64 (memfd, tmpbuf.data, tmpbuf.length, name_offset); > > 228 /* Try the l_libname element. */ > 229 struct E(libname_list) ln; > 230 if (pread64 (memfd, &ln, sizeof (ln), m.l_libname) == sizeof (ln)) > 231 { > 232 name_offset = ln.name; > 233 goto again; > 234 } > > Since the value at ln.name (l_libname->name) will be the same as previously > read. The straightforward fix is just avoid the check and read the new list > entry. > > I checked also against binaries issues with old loaders with fix for BZ#387, > and pldd could dump the shared objects. > > Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, and > powerpc64le-linux-gnu. > > [BZ #18035] > * elf/Makefile (tests-container): Add tst-pldd. > * elf/pldd-xx.c: Use _Static_assert in of pldd_assert. > (E(find_maps)): Avoid use alloca, use default read file operations > instead of explicit LFS names, and fix infinite loop. > * elf/pldd.c: Explicit set _FILE_OFFSET_BITS, cleanup headers. > (get_process_info): Use _Static_assert instead of assert, use default > directory operations instead of explicit LFS names, and free some > leadek pointers. > * elf/tst-pldd.c: New file. > * elf/tst-pldd.root/tst-pldd.script: Likewise. > --- > elf/Makefile | 1 + > elf/pldd-xx.c | 114 ++++++++++------------------- > elf/pldd.c | 64 ++++++++-------- > elf/tst-pldd.c | 118 ++++++++++++++++++++++++++++++ > elf/tst-pldd.root/tst-pldd.script | 1 + > 5 files changed, 190 insertions(+), 108 deletions(-) > create mode 100644 elf/tst-pldd.c > create mode 100644 elf/tst-pldd.root/tst-pldd.script > > diff --git a/elf/Makefile b/elf/Makefile > index 310a37cc13..0b4a877880 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -194,6 +194,7 @@ tests-internal += loadtest unload unload2 circleload1 \ > tst-tls3 tst-tls6 tst-tls7 tst-tls8 tst-dlmopen2 \ > tst-ptrguard1 tst-stackguard1 tst-libc_dlvsym \ > tst-create_format1 > +tests-container += tst-pldd > ifeq ($(build-hardcoded-path-in-tests),yes) > tests += tst-dlopen-aout > tst-dlopen-aout-no-pie = yes > diff --git a/elf/pldd-xx.c b/elf/pldd-xx.c > index 547f840ee1..28a8659935 100644 > --- a/elf/pldd-xx.c > +++ b/elf/pldd-xx.c > @@ -23,10 +23,6 @@ > #define EW_(e, w, t) EW__(e, w, _##t) > #define EW__(e, w, t) e##w##t > > -#define pldd_assert(name, exp) \ > - typedef int __assert_##name[((exp) != 0) - 1] > - > - > struct E(link_map) > { > EW(Addr) l_addr; > @@ -39,12 +35,12 @@ struct E(link_map) > EW(Addr) l_libname; > }; > #if CLASS == __ELF_NATIVE_CLASS > -pldd_assert (l_addr, (offsetof (struct link_map, l_addr) > - == offsetof (struct E(link_map), l_addr))); > -pldd_assert (l_name, (offsetof (struct link_map, l_name) > - == offsetof (struct E(link_map), l_name))); > -pldd_assert (l_next, (offsetof (struct link_map, l_next) > - == offsetof (struct E(link_map), l_next))); > +_Static_assert (offsetof (struct link_map, l_addr) > + == offsetof (struct E(link_map), l_addr), "l_addr"); > +_Static_assert (offsetof (struct link_map, l_name) > + == offsetof (struct E(link_map), l_name), "l_name"); > +_Static_assert (offsetof (struct link_map, l_next) > + == offsetof (struct E(link_map), l_next), "l_next"); > #endif > > > @@ -54,10 +50,10 @@ struct E(libname_list) > EW(Addr) next; > }; > #if CLASS == __ELF_NATIVE_CLASS > -pldd_assert (name, (offsetof (struct libname_list, name) > - == offsetof (struct E(libname_list), name))); > -pldd_assert (next, (offsetof (struct libname_list, next) > - == offsetof (struct E(libname_list), next))); > +_Static_assert (offsetof (struct libname_list, name) > + == offsetof (struct E(libname_list), name), "name"); > +_Static_assert (offsetof (struct libname_list, next) > + == offsetof (struct E(libname_list), next), "next"); > #endif > > struct E(r_debug) > @@ -69,16 +65,17 @@ struct E(r_debug) > EW(Addr) r_map; > }; > #if CLASS == __ELF_NATIVE_CLASS > -pldd_assert (r_version, (offsetof (struct r_debug, r_version) > - == offsetof (struct E(r_debug), r_version))); > -pldd_assert (r_map, (offsetof (struct r_debug, r_map) > - == offsetof (struct E(r_debug), r_map))); > +_Static_assert (offsetof (struct r_debug, r_version) > + == offsetof (struct E(r_debug), r_version), "r_version"); > +_Static_assert (offsetof (struct r_debug, r_map) > + == offsetof (struct E(r_debug), r_map), "r_map"); > #endif > > > static int > > -E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > +E(find_maps) (const char *exe, int memfd, pid_t pid, void *auxv, > + size_t auxv_size) > { > EW(Addr) phdr = 0; > unsigned int phnum = 0; > @@ -104,12 +101,9 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > if (phdr == 0 || phnum == 0 || phent == 0) > error (EXIT_FAILURE, 0, gettext ("cannot find program header of process")); > > - EW(Phdr) *p = alloca (phnum * phent); > - if (pread64 (memfd, p, phnum * phent, phdr) != phnum * phent) > - { > - error (0, 0, gettext ("cannot read program header")); > - return EXIT_FAILURE; > - } > + EW(Phdr) *p = xmalloc (phnum * phent); > + if (pread (memfd, p, phnum * phent, phdr) != phnum * phent) > + error (EXIT_FAILURE, 0, gettext ("cannot read program header")); > > /* Determine the load offset. We need this for interpreting the > other program header entries so we do this in a separate loop. > @@ -129,24 +123,18 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > if (p[i].p_type == PT_DYNAMIC) > { > EW(Dyn) *dyn = xmalloc (p[i].p_filesz); > - if (pread64 (memfd, dyn, p[i].p_filesz, offset + p[i].p_vaddr) > + if (pread (memfd, dyn, p[i].p_filesz, offset + p[i].p_vaddr) > != p[i].p_filesz) > - { > - error (0, 0, gettext ("cannot read dynamic section")); > - return EXIT_FAILURE; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read dynamic section")); > > /* Search for the DT_DEBUG entry. */ > for (unsigned int j = 0; j < p[i].p_filesz / sizeof (EW(Dyn)); ++j) > if (dyn[j].d_tag == DT_DEBUG && dyn[j].d_un.d_ptr != 0) > { > struct E(r_debug) r; > - if (pread64 (memfd, &r, sizeof (r), dyn[j].d_un.d_ptr) > + if (pread (memfd, &r, sizeof (r), dyn[j].d_un.d_ptr) > != sizeof (r)) > - { > - error (0, 0, gettext ("cannot read r_debug")); > - return EXIT_FAILURE; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read r_debug")); > > if (r.r_map != 0) > { > @@ -160,13 +148,10 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > } > else if (p[i].p_type == PT_INTERP) > { > - interp = alloca (p[i].p_filesz); > - if (pread64 (memfd, interp, p[i].p_filesz, offset + p[i].p_vaddr) > + interp = xmalloc (p[i].p_filesz); > + if (pread (memfd, interp, p[i].p_filesz, offset + p[i].p_vaddr) > != p[i].p_filesz) > - { > - error (0, 0, gettext ("cannot read program interpreter")); > - return EXIT_FAILURE; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read program interpreter")); > } > > if (list == 0) > @@ -174,14 +159,16 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > if (interp == NULL) > { > // XXX check whether the executable itself is the loader > - return EXIT_FAILURE; > + exit (EXIT_FAILURE); > } > > // XXX perhaps try finding ld.so and _r_debug in it > - > - return EXIT_FAILURE; > + exit (EXIT_FAILURE); > } > > + free (p); > + free (interp); > + > /* Print the PID and program name first. */ > printf ("%lu:\t%s\n", (unsigned long int) pid, exe); > > @@ -192,47 +179,27 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > do > { > struct E(link_map) m; > - if (pread64 (memfd, &m, sizeof (m), list) != sizeof (m)) > - { > - error (0, 0, gettext ("cannot read link map")); > - status = EXIT_FAILURE; > - goto out; > - } > + if (pread (memfd, &m, sizeof (m), list) != sizeof (m)) > + error (EXIT_FAILURE, 0, gettext ("cannot read link map")); > > EW(Addr) name_offset = m.l_name; > - again: > while (1) > { > - ssize_t n = pread64 (memfd, tmpbuf.data, tmpbuf.length, name_offset); > + ssize_t n = pread (memfd, tmpbuf.data, tmpbuf.length, name_offset); > if (n == -1) > - { > - error (0, 0, gettext ("cannot read object name")); > - status = EXIT_FAILURE; > - goto out; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read object name")); > > if (memchr (tmpbuf.data, '\0', n) != NULL) > break; > > if (!scratch_buffer_grow (&tmpbuf)) > - { > - error (0, 0, gettext ("cannot allocate buffer for object name")); > - status = EXIT_FAILURE; > - goto out; > - } > + error (EXIT_FAILURE, 0, > + gettext ("cannot allocate buffer for object name")); > } > > - if (((char *)tmpbuf.data)[0] == '\0' && name_offset == m.l_name > - && m.l_libname != 0) > - { > - /* Try the l_libname element. */ > - struct E(libname_list) ln; > - if (pread64 (memfd, &ln, sizeof (ln), m.l_libname) == sizeof (ln)) > - { > - name_offset = ln.name; > - goto again; > - } > - } > + /* The m.l_name and m.l_libname.name for loader linkmap points to same > + values (since BZ#387 fix). Trying to use l_libname name as the > + shared object name might lead to an infinite loop (BZ#18035). */ > > /* Skip over the executable. */ > if (((char *)tmpbuf.data)[0] != '\0') > @@ -242,7 +209,6 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > } > while (list != 0); > > - out: > scratch_buffer_free (&tmpbuf); > return status; > } > diff --git a/elf/pldd.c b/elf/pldd.c > index f3fac4e487..69629bd5d2 100644 > --- a/elf/pldd.c > +++ b/elf/pldd.c > @@ -17,23 +17,17 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#include <alloca.h> > +#define _FILE_OFFSET_BITS 64 > + > #include <argp.h> > -#include <assert.h> > #include <dirent.h> > -#include <elf.h> > -#include <errno.h> > #include <error.h> > #include <fcntl.h> > #include <libintl.h> > -#include <link.h> > -#include <stddef.h> > #include <stdio.h> > #include <stdlib.h> > -#include <string.h> > #include <unistd.h> > #include <sys/ptrace.h> > -#include <sys/stat.h> > #include <sys/wait.h> > #include <scratch_buffer.h> > > @@ -76,14 +70,9 @@ static struct argp argp = > options, parse_opt, args_doc, doc, NULL, more_help, NULL > }; > > -// File descriptor of /proc/*/mem file. > -static int memfd; > - > -/* Name of the executable */ > -static char *exe; > > /* Local functions. */ > -static int get_process_info (int dfd, long int pid); > +static int get_process_info (const char *exe, int dfd, long int pid); > static void wait_for_ptrace_stop (long int pid); > > > @@ -102,8 +91,10 @@ main (int argc, char *argv[]) > return 1; > } > > - assert (sizeof (pid_t) == sizeof (int) > - || sizeof (pid_t) == sizeof (long int)); > + _Static_assert (sizeof (pid_t) == sizeof (int) > + || sizeof (pid_t) == sizeof (long int), > + "sizeof (pid_t) != sizeof (int) or sizeof (long int)"); > + > char *endp; > errno = 0; > long int pid = strtol (argv[remaining], &endp, 10); > @@ -119,25 +110,24 @@ main (int argc, char *argv[]) > if (dfd == -1) > error (EXIT_FAILURE, errno, gettext ("cannot open %s"), buf); > > - struct scratch_buffer exebuf; > - scratch_buffer_init (&exebuf); > + /* Name of the executable */ > + struct scratch_buffer exe; > + scratch_buffer_init (&exe); > ssize_t nexe; > while ((nexe = readlinkat (dfd, "exe", > - exebuf.data, exebuf.length)) == exebuf.length) > + exe.data, exe.length)) == exe.length) > { > - if (!scratch_buffer_grow (&exebuf)) > + if (!scratch_buffer_grow (&exe)) > { > nexe = -1; > break; > } > } > if (nexe == -1) > - exe = (char *) "<program name undetermined>"; > + /* Default stack allocation is at least 1024. */ > + snprintf (exe.data, exe.length, "<program name undetermined>"); > else > - { > - exe = exebuf.data; > - exe[nexe] = '\0'; > - } > + ((char*)exe.data)[nexe] = '\0'; > > /* Stop all threads since otherwise the list of loaded modules might > change while we are reading it. */ > @@ -155,8 +145,8 @@ main (int argc, char *argv[]) > error (EXIT_FAILURE, errno, gettext ("cannot prepare reading %s/task"), > buf); > > - struct dirent64 *d; > - while ((d = readdir64 (dir)) != NULL) > + struct dirent *d; > + while ((d = readdir (dir)) != NULL) > { > if (! isdigit (d->d_name[0])) > continue; > @@ -182,7 +172,7 @@ main (int argc, char *argv[]) > > wait_for_ptrace_stop (tid); > > - struct thread_list *newp = alloca (sizeof (*newp)); > + struct thread_list *newp = xmalloc (sizeof (*newp)); > newp->tid = tid; > newp->next = thread_list; > thread_list = newp; > @@ -190,17 +180,22 @@ main (int argc, char *argv[]) > > closedir (dir); > > - int status = get_process_info (dfd, pid); > + if (thread_list == NULL) > + error (EXIT_FAILURE, 0, gettext ("no valid %s/task entries"), buf); > + > + int status = get_process_info (exe.data, dfd, pid); > > - assert (thread_list != NULL); > do > { > ptrace (PTRACE_DETACH, thread_list->tid, NULL, NULL); > + struct thread_list *prev = thread_list; > thread_list = thread_list->next; > + free (prev); > } > while (thread_list != NULL); > > close (dfd); > + scratch_buffer_free (&exe); > > return status; > } > @@ -281,9 +276,10 @@ warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n\ > > > static int > -get_process_info (int dfd, long int pid) > +get_process_info (const char *exe, int dfd, long int pid) > { > - memfd = openat (dfd, "mem", O_RDONLY); > + /* File descriptor of /proc/<pid>/mem file. */ > + int memfd = openat (dfd, "mem", O_RDONLY); > if (memfd == -1) > goto no_info; > > @@ -333,9 +329,9 @@ get_process_info (int dfd, long int pid) > > int retval; > if (e_ident[EI_CLASS] == ELFCLASS32) > - retval = find_maps32 (pid, auxv, auxv_size); > + retval = find_maps32 (exe, memfd, pid, auxv, auxv_size); > else > - retval = find_maps64 (pid, auxv, auxv_size); > + retval = find_maps64 (exe, memfd, pid, auxv, auxv_size); > > free (auxv); > close (memfd); > diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c > new file mode 100644 > index 0000000000..ed19cedd05 > --- /dev/null > +++ b/elf/tst-pldd.c > @@ -0,0 +1,118 @@ > +/* Basic tests for pldd program. > + Copyright (C) 2019 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library 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 > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <stdio.h> > +#include <string.h> > +#include <unistd.h> > +#include <stdint.h> > +#include <libgen.h> > +#include <stdbool.h> > + > +#include <array_length.h> > +#include <gnu/lib-names.h> > + > +#include <support/subprocess.h> > +#include <support/capture_subprocess.h> > +#include <support/check.h> > + > +static void > +target_process (void *arg) > +{ > + pause (); > +} > + > +/* The test runs in a container because pldd does not support tracing > + a binary started by the loader iself (as with testrun.sh). */ > + > +static int > +do_test (void) > +{ > + /* Create a copy of current test to check with pldd. */ > + struct support_subprocess target = support_subprocess (target_process, NULL); > + > + /* Run 'pldd' on test subprocess. */ > + struct support_capture_subprocess pldd; > + { > + /* Three digits per byte plus null terminator. */ > + char pid[3 * sizeof (uint32_t) + 1]; > + snprintf (pid, array_length (pid), "%d", target.pid); > + > + const char prog[] = "/usr/bin/pldd"; > + > + pldd = support_capture_subprogram (prog, > + (char *const []) { (char *) prog, pid, NULL }); > + > + support_capture_subprocess_check (&pldd, "pldd", 0, sc_allow_stdout); > + } > + > + /* Check 'pldd' output. The test is expected to be linked against only > + loader and libc. */ > + { > + pid_t pid; > + char buffer[512]; > +#define STRINPUT(size) "%" # size "s" > + > + FILE *out = fmemopen (pldd.out.buffer, pldd.out.length, "r"); > + TEST_VERIFY (out != NULL); > + > + /* First line is in the form of <pid>: <full path of executable> */ > + TEST_COMPARE (fscanf (out, "%u: " STRINPUT (512), &pid, buffer), 2); > + > + TEST_COMPARE (pid, target.pid); > + TEST_COMPARE (strcmp (basename (buffer), "tst-pldd"), 0); > + > + /* It expects only one loader and libc loaded by the program. */ > + bool interpreter_found = false, libc_found = false; > + while (fgets (buffer, array_length (buffer), out) != NULL) > + { > + /* Ignore vDSO. */ > + if (buffer[0] != '/') > + continue; > + > + /* Remove newline so baseline (buffer) can compare against the > + LD_SO and LIBC_SO macros unmodified. */ > + if (buffer[strlen(buffer)-1] == '\n') > + buffer[strlen(buffer)-1] = '\0'; > + > + if (strcmp (basename (buffer), LD_SO) == 0) > + { > + TEST_COMPARE (interpreter_found, false); > + interpreter_found = true; > + continue; > + } > + > + if (strcmp (basename (buffer), LIBC_SO) == 0) > + { > + TEST_COMPARE (libc_found, false); > + libc_found = true; > + continue; > + } > + } > + TEST_COMPARE (interpreter_found, true); > + TEST_COMPARE (libc_found, true); > + > + fclose (out); > + } > + > + support_capture_subprocess_free (&pldd); > + support_process_terminate (&target); > + > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/elf/tst-pldd.root/tst-pldd.script b/elf/tst-pldd.root/tst-pldd.script > new file mode 100644 > index 0000000000..5a197b2ed0 > --- /dev/null > +++ b/elf/tst-pldd.root/tst-pldd.script > @@ -0,0 +1 @@ > +cp $B/elf/pldd $I/bin/pldd >
On 4/17/19 10:42 AM, Adhemerval Zanella wrote: > Changes from previous version: > > - Removal of more braces. > - Added a comment where the old code was removed to warn against looking > at l_libname. > - Adjusted test comment. > - Remove file artifact. > If you remove elf/tst-pldd.root/tst-pldd.script, because it's not needed because pldd is always installed as part of 'make install' in the testroot.root, the OK for commit. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- ^^^ Should be mostly scissors and dashes for git am to exclude this part in a commit review. > > Since 9182aa67994 (Fix vDSO l_name for GDB's, BZ#387) the initial link_map > for executable itself and loader will have both l_name and l_libname->name > holding the same value due: > > elf/dl-object.c > > 95 new->l_name = *realname ? realname : (char *) newname->name + libname_len - 1; > > Since newname->name points to new->l_libname->name. > > This leads to pldd to an infinite call at: > > elf/pldd-xx.c > > 203 again: > 204 while (1) > 205 { > 206 ssize_t n = pread64 (memfd, tmpbuf.data, tmpbuf.length, name_offset); > > 228 /* Try the l_libname element. */ > 229 struct E(libname_list) ln; > 230 if (pread64 (memfd, &ln, sizeof (ln), m.l_libname) == sizeof (ln)) > 231 { > 232 name_offset = ln.name; > 233 goto again; > 234 } > > Since the value at ln.name (l_libname->name) will be the same as previously > read. The straightforward fix is just avoid the check and read the new list > entry. > > I checked also against binaries issues with old loaders with fix for BZ#387, > and pldd could dump the shared objects. > > Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, and > powerpc64le-linux-gnu. > > [BZ #18035] > * elf/Makefile (tests-container): Add tst-pldd. > * elf/pldd-xx.c: Use _Static_assert in of pldd_assert. > (E(find_maps)): Avoid use alloca, use default read file operations > instead of explicit LFS names, and fix infinite loop. > * elf/pldd.c: Explicit set _FILE_OFFSET_BITS, cleanup headers. > (get_process_info): Use _Static_assert instead of assert, use default > directory operations instead of explicit LFS names, and free some > leadek pointers. > * elf/tst-pldd.c: New file. > * elf/tst-pldd.root/tst-pldd.script: Likewise. OK. > --- > elf/Makefile | 1 + > elf/pldd-xx.c | 114 ++++++++++------------------- > elf/pldd.c | 64 ++++++++-------- > elf/tst-pldd.c | 118 ++++++++++++++++++++++++++++++ > elf/tst-pldd.root/tst-pldd.script | 1 + > 5 files changed, 190 insertions(+), 108 deletions(-) > create mode 100644 elf/tst-pldd.c > create mode 100644 elf/tst-pldd.root/tst-pldd.script > > diff --git a/elf/Makefile b/elf/Makefile > index 310a37cc13..0b4a877880 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -194,6 +194,7 @@ tests-internal += loadtest unload unload2 circleload1 \ > tst-tls3 tst-tls6 tst-tls7 tst-tls8 tst-dlmopen2 \ > tst-ptrguard1 tst-stackguard1 tst-libc_dlvsym \ > tst-create_format1 > +tests-container += tst-pldd OK. > ifeq ($(build-hardcoded-path-in-tests),yes) > tests += tst-dlopen-aout > tst-dlopen-aout-no-pie = yes > diff --git a/elf/pldd-xx.c b/elf/pldd-xx.c > index 547f840ee1..28a8659935 100644 > --- a/elf/pldd-xx.c > +++ b/elf/pldd-xx.c > @@ -23,10 +23,6 @@ > #define EW_(e, w, t) EW__(e, w, _##t) > #define EW__(e, w, t) e##w##t > > -#define pldd_assert(name, exp) \ > - typedef int __assert_##name[((exp) != 0) - 1] > - > - OK. > struct E(link_map) > { > EW(Addr) l_addr; > @@ -39,12 +35,12 @@ struct E(link_map) > EW(Addr) l_libname; > }; > #if CLASS == __ELF_NATIVE_CLASS > -pldd_assert (l_addr, (offsetof (struct link_map, l_addr) > - == offsetof (struct E(link_map), l_addr))); > -pldd_assert (l_name, (offsetof (struct link_map, l_name) > - == offsetof (struct E(link_map), l_name))); > -pldd_assert (l_next, (offsetof (struct link_map, l_next) > - == offsetof (struct E(link_map), l_next))); > +_Static_assert (offsetof (struct link_map, l_addr) > + == offsetof (struct E(link_map), l_addr), "l_addr"); > +_Static_assert (offsetof (struct link_map, l_name) > + == offsetof (struct E(link_map), l_name), "l_name"); > +_Static_assert (offsetof (struct link_map, l_next) > + == offsetof (struct E(link_map), l_next), "l_next"); OK. > #endif > > > @@ -54,10 +50,10 @@ struct E(libname_list) > EW(Addr) next; > }; > #if CLASS == __ELF_NATIVE_CLASS > -pldd_assert (name, (offsetof (struct libname_list, name) > - == offsetof (struct E(libname_list), name))); > -pldd_assert (next, (offsetof (struct libname_list, next) > - == offsetof (struct E(libname_list), next))); > +_Static_assert (offsetof (struct libname_list, name) > + == offsetof (struct E(libname_list), name), "name"); > +_Static_assert (offsetof (struct libname_list, next) > + == offsetof (struct E(libname_list), next), "next"); OK. > #endif > > struct E(r_debug) > @@ -69,16 +65,17 @@ struct E(r_debug) > EW(Addr) r_map; > }; > #if CLASS == __ELF_NATIVE_CLASS > -pldd_assert (r_version, (offsetof (struct r_debug, r_version) > - == offsetof (struct E(r_debug), r_version))); > -pldd_assert (r_map, (offsetof (struct r_debug, r_map) > - == offsetof (struct E(r_debug), r_map))); > +_Static_assert (offsetof (struct r_debug, r_version) > + == offsetof (struct E(r_debug), r_version), "r_version"); > +_Static_assert (offsetof (struct r_debug, r_map) > + == offsetof (struct E(r_debug), r_map), "r_map"); OK. > #endif > > > static int > > -E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > +E(find_maps) (const char *exe, int memfd, pid_t pid, void *auxv, > + size_t auxv_size) OK. > { > EW(Addr) phdr = 0; > unsigned int phnum = 0; > @@ -104,12 +101,9 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > if (phdr == 0 || phnum == 0 || phent == 0) > error (EXIT_FAILURE, 0, gettext ("cannot find program header of process")); > > - EW(Phdr) *p = alloca (phnum * phent); > - if (pread64 (memfd, p, phnum * phent, phdr) != phnum * phent) > - { > - error (0, 0, gettext ("cannot read program header")); > - return EXIT_FAILURE; > - } > + EW(Phdr) *p = xmalloc (phnum * phent); > + if (pread (memfd, p, phnum * phent, phdr) != phnum * phent) > + error (EXIT_FAILURE, 0, gettext ("cannot read program header")); OK. > > /* Determine the load offset. We need this for interpreting the > other program header entries so we do this in a separate loop. > @@ -129,24 +123,18 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > if (p[i].p_type == PT_DYNAMIC) > { > EW(Dyn) *dyn = xmalloc (p[i].p_filesz); > - if (pread64 (memfd, dyn, p[i].p_filesz, offset + p[i].p_vaddr) > + if (pread (memfd, dyn, p[i].p_filesz, offset + p[i].p_vaddr) OK. > != p[i].p_filesz) > - { > - error (0, 0, gettext ("cannot read dynamic section")); > - return EXIT_FAILURE; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read dynamic section")); OK. > > /* Search for the DT_DEBUG entry. */ > for (unsigned int j = 0; j < p[i].p_filesz / sizeof (EW(Dyn)); ++j) > if (dyn[j].d_tag == DT_DEBUG && dyn[j].d_un.d_ptr != 0) > { > struct E(r_debug) r; > - if (pread64 (memfd, &r, sizeof (r), dyn[j].d_un.d_ptr) > + if (pread (memfd, &r, sizeof (r), dyn[j].d_un.d_ptr) OK. > != sizeof (r)) > - { > - error (0, 0, gettext ("cannot read r_debug")); > - return EXIT_FAILURE; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read r_debug")); OK. > > if (r.r_map != 0) > { > @@ -160,13 +148,10 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > } > else if (p[i].p_type == PT_INTERP) > { > - interp = alloca (p[i].p_filesz); > - if (pread64 (memfd, interp, p[i].p_filesz, offset + p[i].p_vaddr) > + interp = xmalloc (p[i].p_filesz); > + if (pread (memfd, interp, p[i].p_filesz, offset + p[i].p_vaddr) OK. > != p[i].p_filesz) > - { > - error (0, 0, gettext ("cannot read program interpreter")); > - return EXIT_FAILURE; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read program interpreter")); OK. > } > > if (list == 0) > @@ -174,14 +159,16 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > if (interp == NULL) > { > // XXX check whether the executable itself is the loader > - return EXIT_FAILURE; > + exit (EXIT_FAILURE); OK. > } > > // XXX perhaps try finding ld.so and _r_debug in it > - > - return EXIT_FAILURE; > + exit (EXIT_FAILURE); OK. > } > > + free (p); > + free (interp); OK. > + > /* Print the PID and program name first. */ > printf ("%lu:\t%s\n", (unsigned long int) pid, exe); > > @@ -192,47 +179,27 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > do > { > struct E(link_map) m; > - if (pread64 (memfd, &m, sizeof (m), list) != sizeof (m)) > - { > - error (0, 0, gettext ("cannot read link map")); > - status = EXIT_FAILURE; > - goto out; > - } > + if (pread (memfd, &m, sizeof (m), list) != sizeof (m)) > + error (EXIT_FAILURE, 0, gettext ("cannot read link map")); OK. > > EW(Addr) name_offset = m.l_name; > - again: OK. > while (1) > { > - ssize_t n = pread64 (memfd, tmpbuf.data, tmpbuf.length, name_offset); > + ssize_t n = pread (memfd, tmpbuf.data, tmpbuf.length, name_offset); OK. > if (n == -1) > - { > - error (0, 0, gettext ("cannot read object name")); > - status = EXIT_FAILURE; > - goto out; > - } > + error (EXIT_FAILURE, 0, gettext ("cannot read object name")); OK. > > if (memchr (tmpbuf.data, '\0', n) != NULL) > break; > > if (!scratch_buffer_grow (&tmpbuf)) > - { > - error (0, 0, gettext ("cannot allocate buffer for object name")); > - status = EXIT_FAILURE; > - goto out; > - } > + error (EXIT_FAILURE, 0, > + gettext ("cannot allocate buffer for object name")); OK. > } > > - if (((char *)tmpbuf.data)[0] == '\0' && name_offset == m.l_name > - && m.l_libname != 0) > - { > - /* Try the l_libname element. */ > - struct E(libname_list) ln; > - if (pread64 (memfd, &ln, sizeof (ln), m.l_libname) == sizeof (ln)) > - { > - name_offset = ln.name; > - goto again; > - } > - } > + /* The m.l_name and m.l_libname.name for loader linkmap points to same > + values (since BZ#387 fix). Trying to use l_libname name as the > + shared object name might lead to an infinite loop (BZ#18035). */ OK. Thanks for this! It's a nugget of gold for future maintainers. This also simplifies the heuristics. > > /* Skip over the executable. */ > if (((char *)tmpbuf.data)[0] != '\0') > @@ -242,7 +209,6 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > } > while (list != 0); > > - out: OK. > scratch_buffer_free (&tmpbuf); > return status; > } > diff --git a/elf/pldd.c b/elf/pldd.c > index f3fac4e487..69629bd5d2 100644 > --- a/elf/pldd.c > +++ b/elf/pldd.c > @@ -17,23 +17,17 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#include <alloca.h> > +#define _FILE_OFFSET_BITS 64 > + OK. > #include <argp.h> > -#include <assert.h> > #include <dirent.h> > -#include <elf.h> > -#include <errno.h> > #include <error.h> > #include <fcntl.h> > #include <libintl.h> > -#include <link.h> > -#include <stddef.h> > #include <stdio.h> > #include <stdlib.h> > -#include <string.h> > #include <unistd.h> > #include <sys/ptrace.h> > -#include <sys/stat.h> > #include <sys/wait.h> > #include <scratch_buffer.h> > > @@ -76,14 +70,9 @@ static struct argp argp = > options, parse_opt, args_doc, doc, NULL, more_help, NULL > }; > > -// File descriptor of /proc/*/mem file. > -static int memfd; > - > -/* Name of the executable */ > -static char *exe; OK. > > /* Local functions. */ > -static int get_process_info (int dfd, long int pid); > +static int get_process_info (const char *exe, int dfd, long int pid); OK. > static void wait_for_ptrace_stop (long int pid); > > > @@ -102,8 +91,10 @@ main (int argc, char *argv[]) > return 1; > } > > - assert (sizeof (pid_t) == sizeof (int) > - || sizeof (pid_t) == sizeof (long int)); > + _Static_assert (sizeof (pid_t) == sizeof (int) > + || sizeof (pid_t) == sizeof (long int), > + "sizeof (pid_t) != sizeof (int) or sizeof (long int)"); OK. > + > char *endp; > errno = 0; > long int pid = strtol (argv[remaining], &endp, 10); > @@ -119,25 +110,24 @@ main (int argc, char *argv[]) > if (dfd == -1) > error (EXIT_FAILURE, errno, gettext ("cannot open %s"), buf); > > - struct scratch_buffer exebuf; > - scratch_buffer_init (&exebuf); > + /* Name of the executable */ > + struct scratch_buffer exe; > + scratch_buffer_init (&exe); OK. > ssize_t nexe; > while ((nexe = readlinkat (dfd, "exe", > - exebuf.data, exebuf.length)) == exebuf.length) > + exe.data, exe.length)) == exe.length) > { > - if (!scratch_buffer_grow (&exebuf)) > + if (!scratch_buffer_grow (&exe)) > { > nexe = -1; > break; > } > } > if (nexe == -1) > - exe = (char *) "<program name undetermined>"; > + /* Default stack allocation is at least 1024. */ > + snprintf (exe.data, exe.length, "<program name undetermined>"); OK. > else > - { > - exe = exebuf.data; > - exe[nexe] = '\0'; > - } > + ((char*)exe.data)[nexe] = '\0'; OK. > > /* Stop all threads since otherwise the list of loaded modules might > change while we are reading it. */ > @@ -155,8 +145,8 @@ main (int argc, char *argv[]) > error (EXIT_FAILURE, errno, gettext ("cannot prepare reading %s/task"), > buf); > > - struct dirent64 *d; > - while ((d = readdir64 (dir)) != NULL) > + struct dirent *d; > + while ((d = readdir (dir)) != NULL) OK. > { > if (! isdigit (d->d_name[0])) > continue; > @@ -182,7 +172,7 @@ main (int argc, char *argv[]) > > wait_for_ptrace_stop (tid); > > - struct thread_list *newp = alloca (sizeof (*newp)); > + struct thread_list *newp = xmalloc (sizeof (*newp)); OK. > newp->tid = tid; > newp->next = thread_list; > thread_list = newp; > @@ -190,17 +180,22 @@ main (int argc, char *argv[]) > > closedir (dir); > > - int status = get_process_info (dfd, pid); > + if (thread_list == NULL) > + error (EXIT_FAILURE, 0, gettext ("no valid %s/task entries"), buf); > + > + int status = get_process_info (exe.data, dfd, pid); OK. > > - assert (thread_list != NULL); > do > { > ptrace (PTRACE_DETACH, thread_list->tid, NULL, NULL); > + struct thread_list *prev = thread_list; > thread_list = thread_list->next; > + free (prev); OK. > } > while (thread_list != NULL); > > close (dfd); > + scratch_buffer_free (&exe); OK. > > return status; > } > @@ -281,9 +276,10 @@ warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n\ > > > static int > -get_process_info (int dfd, long int pid) > +get_process_info (const char *exe, int dfd, long int pid) OK. > { > - memfd = openat (dfd, "mem", O_RDONLY); > + /* File descriptor of /proc/<pid>/mem file. */ > + int memfd = openat (dfd, "mem", O_RDONLY); OK. > if (memfd == -1) > goto no_info; > > @@ -333,9 +329,9 @@ get_process_info (int dfd, long int pid) > > int retval; > if (e_ident[EI_CLASS] == ELFCLASS32) > - retval = find_maps32 (pid, auxv, auxv_size); > + retval = find_maps32 (exe, memfd, pid, auxv, auxv_size); > else > - retval = find_maps64 (pid, auxv, auxv_size); > + retval = find_maps64 (exe, memfd, pid, auxv, auxv_size); OK. > > free (auxv); > close (memfd); > diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c > new file mode 100644 > index 0000000000..ed19cedd05 > --- /dev/null > +++ b/elf/tst-pldd.c > @@ -0,0 +1,118 @@ > +/* Basic tests for pldd program. OK. > + Copyright (C) 2019 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library 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 > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <stdio.h> > +#include <string.h> > +#include <unistd.h> > +#include <stdint.h> > +#include <libgen.h> > +#include <stdbool.h> > + > +#include <array_length.h> > +#include <gnu/lib-names.h> > + > +#include <support/subprocess.h> > +#include <support/capture_subprocess.h> > +#include <support/check.h> > + > +static void > +target_process (void *arg) > +{ > + pause (); > +} > + > +/* The test runs in a container because pldd does not support tracing > + a binary started by the loader iself (as with testrun.sh). */ > + > +static int > +do_test (void) > +{ > + /* Create a copy of current test to check with pldd. */ > + struct support_subprocess target = support_subprocess (target_process, NULL); > + > + /* Run 'pldd' on test subprocess. */ > + struct support_capture_subprocess pldd; > + { > + /* Three digits per byte plus null terminator. */ > + char pid[3 * sizeof (uint32_t) + 1]; > + snprintf (pid, array_length (pid), "%d", target.pid); > + > + const char prog[] = "/usr/bin/pldd"; > + > + pldd = support_capture_subprogram (prog, > + (char *const []) { (char *) prog, pid, NULL }); > + > + support_capture_subprocess_check (&pldd, "pldd", 0, sc_allow_stdout); > + } > + > + /* Check 'pldd' output. The test is expected to be linked against only > + loader and libc. */ > + { > + pid_t pid; > + char buffer[512]; > +#define STRINPUT(size) "%" # size "s" > + > + FILE *out = fmemopen (pldd.out.buffer, pldd.out.length, "r"); > + TEST_VERIFY (out != NULL); > + > + /* First line is in the form of <pid>: <full path of executable> */ > + TEST_COMPARE (fscanf (out, "%u: " STRINPUT (512), &pid, buffer), 2); > + > + TEST_COMPARE (pid, target.pid); > + TEST_COMPARE (strcmp (basename (buffer), "tst-pldd"), 0); > + > + /* It expects only one loader and libc loaded by the program. */ > + bool interpreter_found = false, libc_found = false; > + while (fgets (buffer, array_length (buffer), out) != NULL) > + { > + /* Ignore vDSO. */ > + if (buffer[0] != '/') > + continue; > + > + /* Remove newline so baseline (buffer) can compare against the > + LD_SO and LIBC_SO macros unmodified. */ > + if (buffer[strlen(buffer)-1] == '\n') > + buffer[strlen(buffer)-1] = '\0'; > + > + if (strcmp (basename (buffer), LD_SO) == 0) > + { > + TEST_COMPARE (interpreter_found, false); > + interpreter_found = true; > + continue; > + } > + > + if (strcmp (basename (buffer), LIBC_SO) == 0) > + { > + TEST_COMPARE (libc_found, false); > + libc_found = true; > + continue; > + } > + } > + TEST_COMPARE (interpreter_found, true); > + TEST_COMPARE (libc_found, true); > + > + fclose (out); > + } > + > + support_capture_subprocess_free (&pldd); > + support_process_terminate (&target); > + > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/elf/tst-pldd.root/tst-pldd.script b/elf/tst-pldd.root/tst-pldd.script > new file mode 100644 > index 0000000000..5a197b2ed0 > --- /dev/null > +++ b/elf/tst-pldd.root/tst-pldd.script > @@ -0,0 +1 @@ > +cp $B/elf/pldd $I/bin/pldd This is not required. pldd is already installed in the testroot.root by the test-in-container setup process. You don't need a *.root directory for this test. > -- Cheers, Carlos.
17.04.2019 16:42 Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > [...] > [BZ #18035] > * elf/Makefile (tests-container): Add tst-pldd. > * elf/pldd-xx.c: Use _Static_assert in of pldd_assert. > (E(find_maps)): Avoid use alloca, use default read file operations > instead of explicit LFS names, and fix infinite loop. > * elf/pldd.c: Explicit set _FILE_OFFSET_BITS, cleanup headers. > (get_process_info): Use _Static_assert instead of assert, use default > directory operations instead of explicit LFS names, and free some > leadek pointers. > * elf/tst-pldd.c: New file. This test (elf/tst-pldd.c) always times out for me, even with some large timeout factors. Having read the description of the bug and the patch I believe it's not a simple timeout but a permanent hang instead. Also please see below: > [...] > diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c > new file mode 100644 > index 0000000000..ed19cedd05 > --- /dev/null > +++ b/elf/tst-pldd.c > @@ -0,0 +1,118 @@ > + [...] > +static int > +do_test (void) > +{ > + /* Create a copy of current test to check with pldd. */ > + struct support_subprocess target = support_subprocess (target_process, > NULL); > + > + /* Run 'pldd' on test subprocess. */ > + struct support_capture_subprocess pldd; > + { > + /* Three digits per byte plus null terminator. */ > + char pid[3 * sizeof (uint32_t) + 1]; > + snprintf (pid, array_length (pid), "%d", target.pid); > + > + const char prog[] = "/usr/bin/pldd"; My guess is that the reason is that it launches and tests the system installed /usr/bin/pldd which most likely contains the bug which is fixed by this patch. Would you consider replacing "/usr/bin/pldd" with "./pldd" or whatever path is required to use the correct pldd? Thanks and regards, Rafal
* Rafal Luzynski: > 17.04.2019 16:42 Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >> [...] >> [BZ #18035] >> * elf/Makefile (tests-container): Add tst-pldd. >> + const char prog[] = "/usr/bin/pldd"; > > My guess is that the reason is that it launches and tests the system > installed /usr/bin/pldd which most likely contains the bug which is fixed > by this patch. Would you consider replacing "/usr/bin/pldd" with "./pldd" > or whatever path is required to use the correct pldd? Do you run the test using the test-in-container framework? It's supposed to run in a chroot. As discussed before, the makefile dependencies for the chroot generation are incorrect/incomplete, so perhaps pldd in the chroot was not updated properly. Thanks, Florian
25.04.2019 06:51 Florian Weimer <fweimer@redhat.com> wrote: > [...] > Do you run the test using the test-in-container framework? No. > It's supposed to run in a chroot. Sounds reasonable that it would work fine in a chroot environment. What about "make check", is it obsolete? Maybe this test should be skipped if executed outside the test-in-container framework? Regards, Rafal
* Rafal Luzynski: > 25.04.2019 06:51 Florian Weimer <fweimer@redhat.com> wrote: >> [...] >> Do you run the test using the test-in-container framework? > > No. Well, that explains it. >> It's supposed to run in a chroot. > > Sounds reasonable that it would work fine in a chroot environment. > What about "make check", is it obsolete? Maybe this test should > be skipped if executed outside the test-in-container framework? “make check” is supposed to take care of all that, and run these tests in a chroot if it's possible to create one. Thanks, Florian
On 25/04/2019 06:17, Florian Weimer wrote: > * Rafal Luzynski: > >> 25.04.2019 06:51 Florian Weimer <fweimer@redhat.com> wrote: >>> [...] >>> Do you run the test using the test-in-container framework? >> >> No. > > Well, that explains it. > >>> It's supposed to run in a chroot. >> >> Sounds reasonable that it would work fine in a chroot environment. >> What about "make check", is it obsolete? Maybe this test should >> be skipped if executed outside the test-in-container framework? > > “make check” is supposed to take care of all that, and run these tests > in a chroot if it's possible to create one. > Are you seeing hangouts while running with test-in-container framework as well? > As discussed before, the makefile dependencies for the chroot generation > are incorrect/incomplete, so perhaps pldd in the chroot was not updated > properly. I think I missed this discussion, what is missing here?
* Adhemerval Zanella: >> As discussed before, the makefile dependencies for the chroot generation >> are incorrect/incomplete, so perhaps pldd in the chroot was not updated >> properly. > > I think I missed this discussion, what is missing here? Maybe I misunderstood things, but this is what I took away from this discussion: How to debug test-in-container failures? <https://sourceware.org/ml/libc-alpha/2019-02/msg00267.html> Thanks, Florian
On Thu, Apr 25, 2019 at 5:22 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Adhemerval Zanella: > > >> As discussed before, the makefile dependencies for the chroot generation > >> are incorrect/incomplete, so perhaps pldd in the chroot was not updated > >> properly. > > > > I think I missed this discussion, what is missing here? > > Maybe I misunderstood things, but this is what I took away from this > discussion: > > How to debug test-in-container failures? > <https://sourceware.org/ml/libc-alpha/2019-02/msg00267.html> > I opened: https://sourceware.org/bugzilla/show_bug.cgi?id=24506 -- H.J.
25.04.2019 11:17 Florian Weimer <fweimer@redhat.com> wrote: > > * Rafal Luzynski: > > > 25.04.2019 06:51 Florian Weimer <fweimer@redhat.com> wrote: > >> [...] > >> Do you run the test using the test-in-container framework? > > > > No. > > Well, that explains it. Looking at your backports [1] [2] "(Backported without the test case..." I understand that glibc development is possible only on rather new system so I am moving to a newer machine. This works good in Fedora 30. Regards, Rafal [1] https://sourceware.org/ml/libc-stable/2019-04/msg00010.html [2] https://sourceware.org/ml/libc-stable/2019-04/msg00011.html
diff --git a/elf/Makefile b/elf/Makefile index 310a37cc13..0b4a877880 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -194,6 +194,7 @@ tests-internal += loadtest unload unload2 circleload1 \ tst-tls3 tst-tls6 tst-tls7 tst-tls8 tst-dlmopen2 \ tst-ptrguard1 tst-stackguard1 tst-libc_dlvsym \ tst-create_format1 +tests-container += tst-pldd ifeq ($(build-hardcoded-path-in-tests),yes) tests += tst-dlopen-aout tst-dlopen-aout-no-pie = yes diff --git a/elf/pldd-xx.c b/elf/pldd-xx.c index 547f840ee1..28a8659935 100644 --- a/elf/pldd-xx.c +++ b/elf/pldd-xx.c @@ -23,10 +23,6 @@ #define EW_(e, w, t) EW__(e, w, _##t) #define EW__(e, w, t) e##w##t -#define pldd_assert(name, exp) \ - typedef int __assert_##name[((exp) != 0) - 1] - - struct E(link_map) { EW(Addr) l_addr; @@ -39,12 +35,12 @@ struct E(link_map) EW(Addr) l_libname; }; #if CLASS == __ELF_NATIVE_CLASS -pldd_assert (l_addr, (offsetof (struct link_map, l_addr) - == offsetof (struct E(link_map), l_addr))); -pldd_assert (l_name, (offsetof (struct link_map, l_name) - == offsetof (struct E(link_map), l_name))); -pldd_assert (l_next, (offsetof (struct link_map, l_next) - == offsetof (struct E(link_map), l_next))); +_Static_assert (offsetof (struct link_map, l_addr) + == offsetof (struct E(link_map), l_addr), "l_addr"); +_Static_assert (offsetof (struct link_map, l_name) + == offsetof (struct E(link_map), l_name), "l_name"); +_Static_assert (offsetof (struct link_map, l_next) + == offsetof (struct E(link_map), l_next), "l_next"); #endif @@ -54,10 +50,10 @@ struct E(libname_list) EW(Addr) next; }; #if CLASS == __ELF_NATIVE_CLASS -pldd_assert (name, (offsetof (struct libname_list, name) - == offsetof (struct E(libname_list), name))); -pldd_assert (next, (offsetof (struct libname_list, next) - == offsetof (struct E(libname_list), next))); +_Static_assert (offsetof (struct libname_list, name) + == offsetof (struct E(libname_list), name), "name"); +_Static_assert (offsetof (struct libname_list, next) + == offsetof (struct E(libname_list), next), "next"); #endif struct E(r_debug) @@ -69,16 +65,17 @@ struct E(r_debug) EW(Addr) r_map; }; #if CLASS == __ELF_NATIVE_CLASS -pldd_assert (r_version, (offsetof (struct r_debug, r_version) - == offsetof (struct E(r_debug), r_version))); -pldd_assert (r_map, (offsetof (struct r_debug, r_map) - == offsetof (struct E(r_debug), r_map))); +_Static_assert (offsetof (struct r_debug, r_version) + == offsetof (struct E(r_debug), r_version), "r_version"); +_Static_assert (offsetof (struct r_debug, r_map) + == offsetof (struct E(r_debug), r_map), "r_map"); #endif static int -E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) +E(find_maps) (const char *exe, int memfd, pid_t pid, void *auxv, + size_t auxv_size) { EW(Addr) phdr = 0; unsigned int phnum = 0; @@ -104,12 +101,9 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) if (phdr == 0 || phnum == 0 || phent == 0) error (EXIT_FAILURE, 0, gettext ("cannot find program header of process")); - EW(Phdr) *p = alloca (phnum * phent); - if (pread64 (memfd, p, phnum * phent, phdr) != phnum * phent) - { - error (0, 0, gettext ("cannot read program header")); - return EXIT_FAILURE; - } + EW(Phdr) *p = xmalloc (phnum * phent); + if (pread (memfd, p, phnum * phent, phdr) != phnum * phent) + error (EXIT_FAILURE, 0, gettext ("cannot read program header")); /* Determine the load offset. We need this for interpreting the other program header entries so we do this in a separate loop. @@ -129,24 +123,18 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) if (p[i].p_type == PT_DYNAMIC) { EW(Dyn) *dyn = xmalloc (p[i].p_filesz); - if (pread64 (memfd, dyn, p[i].p_filesz, offset + p[i].p_vaddr) + if (pread (memfd, dyn, p[i].p_filesz, offset + p[i].p_vaddr) != p[i].p_filesz) - { - error (0, 0, gettext ("cannot read dynamic section")); - return EXIT_FAILURE; - } + error (EXIT_FAILURE, 0, gettext ("cannot read dynamic section")); /* Search for the DT_DEBUG entry. */ for (unsigned int j = 0; j < p[i].p_filesz / sizeof (EW(Dyn)); ++j) if (dyn[j].d_tag == DT_DEBUG && dyn[j].d_un.d_ptr != 0) { struct E(r_debug) r; - if (pread64 (memfd, &r, sizeof (r), dyn[j].d_un.d_ptr) + if (pread (memfd, &r, sizeof (r), dyn[j].d_un.d_ptr) != sizeof (r)) - { - error (0, 0, gettext ("cannot read r_debug")); - return EXIT_FAILURE; - } + error (EXIT_FAILURE, 0, gettext ("cannot read r_debug")); if (r.r_map != 0) { @@ -160,13 +148,10 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) } else if (p[i].p_type == PT_INTERP) { - interp = alloca (p[i].p_filesz); - if (pread64 (memfd, interp, p[i].p_filesz, offset + p[i].p_vaddr) + interp = xmalloc (p[i].p_filesz); + if (pread (memfd, interp, p[i].p_filesz, offset + p[i].p_vaddr) != p[i].p_filesz) - { - error (0, 0, gettext ("cannot read program interpreter")); - return EXIT_FAILURE; - } + error (EXIT_FAILURE, 0, gettext ("cannot read program interpreter")); } if (list == 0) @@ -174,14 +159,16 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) if (interp == NULL) { // XXX check whether the executable itself is the loader - return EXIT_FAILURE; + exit (EXIT_FAILURE); } // XXX perhaps try finding ld.so and _r_debug in it - - return EXIT_FAILURE; + exit (EXIT_FAILURE); } + free (p); + free (interp); + /* Print the PID and program name first. */ printf ("%lu:\t%s\n", (unsigned long int) pid, exe); @@ -192,47 +179,27 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) do { struct E(link_map) m; - if (pread64 (memfd, &m, sizeof (m), list) != sizeof (m)) - { - error (0, 0, gettext ("cannot read link map")); - status = EXIT_FAILURE; - goto out; - } + if (pread (memfd, &m, sizeof (m), list) != sizeof (m)) + error (EXIT_FAILURE, 0, gettext ("cannot read link map")); EW(Addr) name_offset = m.l_name; - again: while (1) { - ssize_t n = pread64 (memfd, tmpbuf.data, tmpbuf.length, name_offset); + ssize_t n = pread (memfd, tmpbuf.data, tmpbuf.length, name_offset); if (n == -1) - { - error (0, 0, gettext ("cannot read object name")); - status = EXIT_FAILURE; - goto out; - } + error (EXIT_FAILURE, 0, gettext ("cannot read object name")); if (memchr (tmpbuf.data, '\0', n) != NULL) break; if (!scratch_buffer_grow (&tmpbuf)) - { - error (0, 0, gettext ("cannot allocate buffer for object name")); - status = EXIT_FAILURE; - goto out; - } + error (EXIT_FAILURE, 0, + gettext ("cannot allocate buffer for object name")); } - if (((char *)tmpbuf.data)[0] == '\0' && name_offset == m.l_name - && m.l_libname != 0) - { - /* Try the l_libname element. */ - struct E(libname_list) ln; - if (pread64 (memfd, &ln, sizeof (ln), m.l_libname) == sizeof (ln)) - { - name_offset = ln.name; - goto again; - } - } + /* The m.l_name and m.l_libname.name for loader linkmap points to same + values (since BZ#387 fix). Trying to use l_libname name as the + shared object name might lead to an infinite loop (BZ#18035). */ /* Skip over the executable. */ if (((char *)tmpbuf.data)[0] != '\0') @@ -242,7 +209,6 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) } while (list != 0); - out: scratch_buffer_free (&tmpbuf); return status; } diff --git a/elf/pldd.c b/elf/pldd.c index f3fac4e487..69629bd5d2 100644 --- a/elf/pldd.c +++ b/elf/pldd.c @@ -17,23 +17,17 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <alloca.h> +#define _FILE_OFFSET_BITS 64 + #include <argp.h> -#include <assert.h> #include <dirent.h> -#include <elf.h> -#include <errno.h> #include <error.h> #include <fcntl.h> #include <libintl.h> -#include <link.h> -#include <stddef.h> #include <stdio.h> #include <stdlib.h> -#include <string.h> #include <unistd.h> #include <sys/ptrace.h> -#include <sys/stat.h> #include <sys/wait.h> #include <scratch_buffer.h> @@ -76,14 +70,9 @@ static struct argp argp = options, parse_opt, args_doc, doc, NULL, more_help, NULL }; -// File descriptor of /proc/*/mem file. -static int memfd; - -/* Name of the executable */ -static char *exe; /* Local functions. */ -static int get_process_info (int dfd, long int pid); +static int get_process_info (const char *exe, int dfd, long int pid); static void wait_for_ptrace_stop (long int pid); @@ -102,8 +91,10 @@ main (int argc, char *argv[]) return 1; } - assert (sizeof (pid_t) == sizeof (int) - || sizeof (pid_t) == sizeof (long int)); + _Static_assert (sizeof (pid_t) == sizeof (int) + || sizeof (pid_t) == sizeof (long int), + "sizeof (pid_t) != sizeof (int) or sizeof (long int)"); + char *endp; errno = 0; long int pid = strtol (argv[remaining], &endp, 10); @@ -119,25 +110,24 @@ main (int argc, char *argv[]) if (dfd == -1) error (EXIT_FAILURE, errno, gettext ("cannot open %s"), buf); - struct scratch_buffer exebuf; - scratch_buffer_init (&exebuf); + /* Name of the executable */ + struct scratch_buffer exe; + scratch_buffer_init (&exe); ssize_t nexe; while ((nexe = readlinkat (dfd, "exe", - exebuf.data, exebuf.length)) == exebuf.length) + exe.data, exe.length)) == exe.length) { - if (!scratch_buffer_grow (&exebuf)) + if (!scratch_buffer_grow (&exe)) { nexe = -1; break; } } if (nexe == -1) - exe = (char *) "<program name undetermined>"; + /* Default stack allocation is at least 1024. */ + snprintf (exe.data, exe.length, "<program name undetermined>"); else - { - exe = exebuf.data; - exe[nexe] = '\0'; - } + ((char*)exe.data)[nexe] = '\0'; /* Stop all threads since otherwise the list of loaded modules might change while we are reading it. */ @@ -155,8 +145,8 @@ main (int argc, char *argv[]) error (EXIT_FAILURE, errno, gettext ("cannot prepare reading %s/task"), buf); - struct dirent64 *d; - while ((d = readdir64 (dir)) != NULL) + struct dirent *d; + while ((d = readdir (dir)) != NULL) { if (! isdigit (d->d_name[0])) continue; @@ -182,7 +172,7 @@ main (int argc, char *argv[]) wait_for_ptrace_stop (tid); - struct thread_list *newp = alloca (sizeof (*newp)); + struct thread_list *newp = xmalloc (sizeof (*newp)); newp->tid = tid; newp->next = thread_list; thread_list = newp; @@ -190,17 +180,22 @@ main (int argc, char *argv[]) closedir (dir); - int status = get_process_info (dfd, pid); + if (thread_list == NULL) + error (EXIT_FAILURE, 0, gettext ("no valid %s/task entries"), buf); + + int status = get_process_info (exe.data, dfd, pid); - assert (thread_list != NULL); do { ptrace (PTRACE_DETACH, thread_list->tid, NULL, NULL); + struct thread_list *prev = thread_list; thread_list = thread_list->next; + free (prev); } while (thread_list != NULL); close (dfd); + scratch_buffer_free (&exe); return status; } @@ -281,9 +276,10 @@ warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n\ static int -get_process_info (int dfd, long int pid) +get_process_info (const char *exe, int dfd, long int pid) { - memfd = openat (dfd, "mem", O_RDONLY); + /* File descriptor of /proc/<pid>/mem file. */ + int memfd = openat (dfd, "mem", O_RDONLY); if (memfd == -1) goto no_info; @@ -333,9 +329,9 @@ get_process_info (int dfd, long int pid) int retval; if (e_ident[EI_CLASS] == ELFCLASS32) - retval = find_maps32 (pid, auxv, auxv_size); + retval = find_maps32 (exe, memfd, pid, auxv, auxv_size); else - retval = find_maps64 (pid, auxv, auxv_size); + retval = find_maps64 (exe, memfd, pid, auxv, auxv_size); free (auxv); close (memfd); diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c new file mode 100644 index 0000000000..ed19cedd05 --- /dev/null +++ b/elf/tst-pldd.c @@ -0,0 +1,118 @@ +/* Basic tests for pldd program. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library 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 + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <stdint.h> +#include <libgen.h> +#include <stdbool.h> + +#include <array_length.h> +#include <gnu/lib-names.h> + +#include <support/subprocess.h> +#include <support/capture_subprocess.h> +#include <support/check.h> + +static void +target_process (void *arg) +{ + pause (); +} + +/* The test runs in a container because pldd does not support tracing + a binary started by the loader iself (as with testrun.sh). */ + +static int +do_test (void) +{ + /* Create a copy of current test to check with pldd. */ + struct support_subprocess target = support_subprocess (target_process, NULL); + + /* Run 'pldd' on test subprocess. */ + struct support_capture_subprocess pldd; + { + /* Three digits per byte plus null terminator. */ + char pid[3 * sizeof (uint32_t) + 1]; + snprintf (pid, array_length (pid), "%d", target.pid); + + const char prog[] = "/usr/bin/pldd"; + + pldd = support_capture_subprogram (prog, + (char *const []) { (char *) prog, pid, NULL }); + + support_capture_subprocess_check (&pldd, "pldd", 0, sc_allow_stdout); + } + + /* Check 'pldd' output. The test is expected to be linked against only + loader and libc. */ + { + pid_t pid; + char buffer[512]; +#define STRINPUT(size) "%" # size "s" + + FILE *out = fmemopen (pldd.out.buffer, pldd.out.length, "r"); + TEST_VERIFY (out != NULL); + + /* First line is in the form of <pid>: <full path of executable> */ + TEST_COMPARE (fscanf (out, "%u: " STRINPUT (512), &pid, buffer), 2); + + TEST_COMPARE (pid, target.pid); + TEST_COMPARE (strcmp (basename (buffer), "tst-pldd"), 0); + + /* It expects only one loader and libc loaded by the program. */ + bool interpreter_found = false, libc_found = false; + while (fgets (buffer, array_length (buffer), out) != NULL) + { + /* Ignore vDSO. */ + if (buffer[0] != '/') + continue; + + /* Remove newline so baseline (buffer) can compare against the + LD_SO and LIBC_SO macros unmodified. */ + if (buffer[strlen(buffer)-1] == '\n') + buffer[strlen(buffer)-1] = '\0'; + + if (strcmp (basename (buffer), LD_SO) == 0) + { + TEST_COMPARE (interpreter_found, false); + interpreter_found = true; + continue; + } + + if (strcmp (basename (buffer), LIBC_SO) == 0) + { + TEST_COMPARE (libc_found, false); + libc_found = true; + continue; + } + } + TEST_COMPARE (interpreter_found, true); + TEST_COMPARE (libc_found, true); + + fclose (out); + } + + support_capture_subprocess_free (&pldd); + support_process_terminate (&target); + + return 0; +} + +#include <support/test-driver.c> diff --git a/elf/tst-pldd.root/tst-pldd.script b/elf/tst-pldd.root/tst-pldd.script new file mode 100644 index 0000000000..5a197b2ed0 --- /dev/null +++ b/elf/tst-pldd.root/tst-pldd.script @@ -0,0 +1 @@ +cp $B/elf/pldd $I/bin/pldd
Changes from previous version: - Removal of more braces. - Added a comment where the old code was removed to warn against looking at l_libname. - Adjusted test comment. - Remove file artifact. --- Since 9182aa67994 (Fix vDSO l_name for GDB's, BZ#387) the initial link_map for executable itself and loader will have both l_name and l_libname->name holding the same value due: elf/dl-object.c 95 new->l_name = *realname ? realname : (char *) newname->name + libname_len - 1; Since newname->name points to new->l_libname->name. This leads to pldd to an infinite call at: elf/pldd-xx.c 203 again: 204 while (1) 205 { 206 ssize_t n = pread64 (memfd, tmpbuf.data, tmpbuf.length, name_offset); 228 /* Try the l_libname element. */ 229 struct E(libname_list) ln; 230 if (pread64 (memfd, &ln, sizeof (ln), m.l_libname) == sizeof (ln)) 231 { 232 name_offset = ln.name; 233 goto again; 234 } Since the value at ln.name (l_libname->name) will be the same as previously read. The straightforward fix is just avoid the check and read the new list entry. I checked also against binaries issues with old loaders with fix for BZ#387, and pldd could dump the shared objects. Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, and powerpc64le-linux-gnu. [BZ #18035] * elf/Makefile (tests-container): Add tst-pldd. * elf/pldd-xx.c: Use _Static_assert in of pldd_assert. (E(find_maps)): Avoid use alloca, use default read file operations instead of explicit LFS names, and fix infinite loop. * elf/pldd.c: Explicit set _FILE_OFFSET_BITS, cleanup headers. (get_process_info): Use _Static_assert instead of assert, use default directory operations instead of explicit LFS names, and free some leadek pointers. * elf/tst-pldd.c: New file. * elf/tst-pldd.root/tst-pldd.script: Likewise. --- elf/Makefile | 1 + elf/pldd-xx.c | 114 ++++++++++------------------- elf/pldd.c | 64 ++++++++-------- elf/tst-pldd.c | 118 ++++++++++++++++++++++++++++++ elf/tst-pldd.root/tst-pldd.script | 1 + 5 files changed, 190 insertions(+), 108 deletions(-) create mode 100644 elf/tst-pldd.c create mode 100644 elf/tst-pldd.root/tst-pldd.script -- 2.17.1