Re: [PATCH v2] elf: Fix pldd (BZ#18035)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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
>