| Message ID | 20190416212713.24659-2-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] support: Add support_capture_subprogram | expand |
On 4/16/19 5:27 PM, Adhemerval Zanella wrote: > 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: Thanks for tracking this down! Please post v2: - consider removal of more braces. - add a comment where the old code was removed to warn against looking at l_libname and the problem therein. - consider adjusted comment. > > 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. This has a lot of unrelated cleanup to pldd which does much more than just fix pldd. However, the cleanups are all OK IMO, and if someone needs to backport just the required fix, they can do the removal of t > --- > elf/Makefile | 1 + > elf/pldd-xx.c | 106 ++++++++++----------------- > elf/pldd.c | 64 ++++++++-------- > elf/tst-pldd.c | 118 ++++++++++++++++++++++++++++++ > elf/tst-pldd.root/tst-pldd.script | 1 + > posix/tst-pldd.c | 9 +++ > 6 files changed, 196 insertions(+), 103 deletions(-) > create mode 100644 elf/tst-pldd.c > create mode 100644 elf/tst-pldd.root/tst-pldd.script > create mode 100644 posix/tst-pldd.c > > 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..53858b9932 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) > { > 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,11 +123,10 @@ 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. Consider removing braces? > } > > /* Search for the DT_DEBUG entry. */ > @@ -141,11 +134,10 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) > 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. Consider removing braces? > } > > if (r.r_map != 0) > @@ -160,12 +152,11 @@ 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. Consider removing braces? > } > } > > @@ -174,14 +165,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. Both were changed from alloca to xmalloc. > + > /* Print the PID and program name first. */ > printf ("%lu:\t%s\n", (unsigned long int) pid, exe); > > @@ -192,46 +185,22 @@ 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: > 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")); 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; > - } > - } > - > - 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; > - } > + error (EXIT_FAILURE, 0, > + gettext ("cannot allocate buffer for object name")); This is the real fix right? We remove the iteration into l_libname? Why was this code ever needed in the first place? > } > > /* Skip over the executable. */ > @@ -242,7 +211,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 > + > #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> OK. > #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); > ssize_t nexe; > while ((nexe = readlinkat (dfd, "exe", > - exebuf.data, exebuf.length)) == exebuf.length) > + exe.data, exe.length)) == exe.length) OK. > { > - if (!scratch_buffer_grow (&exebuf)) > + if (!scratch_buffer_grow (&exe)) OK. > { > 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) > { > 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) > { > - 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); OK. > > free (auxv); > close (memfd); > diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c > new file mode 100644 > index 0000000000..9b568ca4eb > --- /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 on container because currently pldd does not support trace > + a binary issues with loader itself (as with testrun.sh). */ Suggest: /* 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> OK. > 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 OK. > diff --git a/posix/tst-pldd.c b/posix/tst-pldd.c > new file mode 100644 > index 0000000000..edff4b9d07 > --- /dev/null > +++ b/posix/tst-pldd.c > @@ -0,0 +1,9 @@ > +#include <unistd.h> > +#include <stdio.h> > + > +int main (int argc, char *argv[]) > +{ > + printf ("pid=%d\n", (int) getpid()); > + pause (); > + return 0; > +} OK. > -- Cheers, Carlos.
On 17/04/2019 01:05, Carlos O'Donell wrote: > On 4/16/19 5:27 PM, Adhemerval Zanella wrote: >> 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: > > Thanks for tracking this down! > > Please post v2: > - consider removal of more braces. > - add a comment where the old code was removed to warn against looking > at l_libname and the problem therein. > - consider adjusted comment. I will sent it, thanks for reviewing it. > >> >> 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. > > This has a lot of unrelated cleanup to pldd which does much more than just > fix pldd. However, the cleanups are all OK IMO, and if someone needs to > backport just the required fix, they can do the removal of t Right, the fix itself is quite simple for a backport (more below). > > >> --- >> elf/Makefile | 1 + >> elf/pldd-xx.c | 106 ++++++++++----------------- >> elf/pldd.c | 64 ++++++++-------- >> elf/tst-pldd.c | 118 ++++++++++++++++++++++++++++++ >> elf/tst-pldd.root/tst-pldd.script | 1 + >> posix/tst-pldd.c | 9 +++ >> 6 files changed, 196 insertions(+), 103 deletions(-) >> create mode 100644 elf/tst-pldd.c >> create mode 100644 elf/tst-pldd.root/tst-pldd.script >> create mode 100644 posix/tst-pldd.c >> >> 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..53858b9932 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) >> { >> 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,11 +123,10 @@ 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. Consider removing braces? Ack. > >> } >> /* Search for the DT_DEBUG entry. */ >> @@ -141,11 +134,10 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) >> 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. Consider removing braces? Ack. > >> } >> if (r.r_map != 0) >> @@ -160,12 +152,11 @@ 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. Consider removing braces? Ack. > >> } >> } >> @@ -174,14 +165,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. Both were changed from alloca to xmalloc. > >> + >> /* Print the PID and program name first. */ >> printf ("%lu:\t%s\n", (unsigned long int) pid, exe); >> @@ -192,46 +185,22 @@ 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: >> 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")); > > 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; >> - } >> - } >> - >> - 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; >> - } >> + error (EXIT_FAILURE, 0, >> + gettext ("cannot allocate buffer for object name")); > > This is the real fix right? We remove the iteration into l_libname? > Why was this code ever needed in the first place? Yes, the core issue is l_name and l_libname->name for loaders linkmap points to same value since BZ#387 fix. This makes 'm.l_libname' being the same address as the m.l_name, thus creating an infinite loop. A more self-contained patch would be: --- diff --git a/elf/pldd-xx.c b/elf/pldd-xx.c index 547f840ee1..995a731d43 100644 --- a/elf/pldd-xx.c +++ b/elf/pldd-xx.c @@ -200,7 +200,6 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) } EW(Addr) name_offset = m.l_name; - again: while (1) { ssize_t n = pread64 (memfd, tmpbuf.data, tmpbuf.length, name_offset); @@ -222,18 +221,6 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) } } - 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; - } - } - /* Skip over the executable. */ if (((char *)tmpbuf.data)[0] != '\0') printf ("%s\n", (char *)tmpbuf.data); --- > >> } >> /* Skip over the executable. */ >> @@ -242,7 +211,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 >> + >> #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> > > OK. > >> #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); >> ssize_t nexe; >> while ((nexe = readlinkat (dfd, "exe", >> - exebuf.data, exebuf.length)) == exebuf.length) >> + exe.data, exe.length)) == exe.length) > > OK. > >> { >> - if (!scratch_buffer_grow (&exebuf)) >> + if (!scratch_buffer_grow (&exe)) > > OK. > >> { >> 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) >> { >> 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) >> { >> - 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); > > OK. > >> free (auxv); >> close (memfd); >> diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c >> new file mode 100644 >> index 0000000000..9b568ca4eb >> --- /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 on container because currently pldd does not support trace >> + a binary issues with loader itself (as with testrun.sh). */ > > Suggest: > > /* The test runs in a container because pldd does not support tracing > a binary started by the loader iself (as with testrun.sh). */ Ack. > >> + >> +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> > > OK. > >> 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 > > OK. > >> diff --git a/posix/tst-pldd.c b/posix/tst-pldd.c >> new file mode 100644 >> index 0000000000..edff4b9d07 >> --- /dev/null >> +++ b/posix/tst-pldd.c >> @@ -0,0 +1,9 @@ >> +#include <unistd.h> >> +#include <stdio.h> >> + >> +int main (int argc, char *argv[]) >> +{ >> + printf ("pid=%d\n", (int) getpid()); >> + pause (); >> + return 0; >> +} > > OK. This is in fact an artifact that should not be on this patch, I will remove it.
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..53858b9932 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,11 +123,10 @@ 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. */ @@ -141,11 +134,10 @@ E(find_maps) (pid_t pid, void *auxv, size_t auxv_size) 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,12 +152,11 @@ 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")); } } @@ -174,14 +165,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,46 +185,22 @@ 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; - } - } - - 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; - } + error (EXIT_FAILURE, 0, + gettext ("cannot allocate buffer for object name")); } /* Skip over the executable. */ @@ -242,7 +211,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..9b568ca4eb --- /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 on container because currently pldd does not support trace + a binary issues with loader itself (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 diff --git a/posix/tst-pldd.c b/posix/tst-pldd.c new file mode 100644 index 0000000000..edff4b9d07 --- /dev/null +++ b/posix/tst-pldd.c @@ -0,0 +1,9 @@ +#include <unistd.h> +#include <stdio.h> + +int main (int argc, char *argv[]) +{ + printf ("pid=%d\n", (int) getpid()); + pause (); + return 0; +}
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 | 106 ++++++++++----------------- elf/pldd.c | 64 ++++++++-------- elf/tst-pldd.c | 118 ++++++++++++++++++++++++++++++ elf/tst-pldd.root/tst-pldd.script | 1 + posix/tst-pldd.c | 9 +++ 6 files changed, 196 insertions(+), 103 deletions(-) create mode 100644 elf/tst-pldd.c create mode 100644 elf/tst-pldd.root/tst-pldd.script create mode 100644 posix/tst-pldd.c -- 2.17.1