This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[patch 1/3] Refactor/simplify (+fix) svr4_current_sos


Hi,

I was always finding svr4_current_sos a bit difficult to read + modify.
Moreover there is a bug shown by the valgrind + non-testsuite testcase below.

Besides fixing the bug (which can probably never happen for real) which was
IMO caused by the coding style the patch also IMO makes the code much easier
to read and more suitable for further modifications in this patch series.

I did not try to make a real GDB testcase as this exploit does not make any
sense with other code implementations, such as the one proposed here.

Giving credit to Paul Pluzhnikov as svr4_free_library_list is from Paul's
patch.

No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu and with
gdbserver and with gdbserver+PIE.


Thanks,
Jan


/*
Invalid free() / delete / delete[]
   at: free (vg_replace_malloc.c:366)
   by: xfree (common-utils.c:109)
   by: do_my_cleanups (utils.c:515)
   by: do_cleanups (utils.c:497)
   by: throw_exception (exceptions.c:213)
   by: throw_it (exceptions.c:381)
   by: throw_error (exceptions.c:402)
   by: memory_error (corefile.c:223)
   by: read_memory (corefile.c:242)
   by: read_memory_unsigned_integer (corefile.c:331)
   by: solib_svr4_r_ldsomap (solib-svr4.c:817)
   by: svr4_current_sos (solib-svr4.c:1084)
   by: update_solib_list (solib.c:677)
 Address 0xde67880 is 0 bytes inside a block of size 40 free'd
   at: free (vg_replace_malloc.c:366)
   by: xfree (common-utils.c:109)
   by: svr4_free_so (solib-svr4.c:2060)
   by: free_so (solib.c:562)
   by: svr4_current_sos (solib-svr4.c:1041)
   by: update_solib_list (solib.c:677)
*/

#define _GNU_SOURCE
#include <link.h>
#include <assert.h>
#include <sys/mman.h>
#include <sys/user.h>
#include <errno.h>
#include <stddef.h>
#include <unistd.h>

int
main (void)
{
  uintptr_t elf_base = (uintptr_t) main & ~(PAGE_SIZE - 1);
  const Elf64_Ehdr *ehdr = (const void *) elf_base;
  const Elf64_Phdr *phdr = (const void *) elf_base + ehdr->e_phoff;
  Elf64_Dyn *dyn;
  struct r_debug *debugp;
  static struct link_map linkmap;
  void *mem;
  int i;

  assert (ehdr->e_phentsize == sizeof (*phdr));
  for (i = 0; i < ehdr->e_phnum; i++, phdr++)
    if (phdr->p_type == PT_DYNAMIC)
      break;
  assert (i < ehdr->e_phnum);

  for (dyn = (void *) phdr->p_vaddr; dyn->d_tag != DT_DEBUG; dyn++)
    assert (dyn->d_tag != DT_NULL);

  debugp = (void *) dyn->d_un.d_ptr;
  assert (debugp->r_version == 1);
  assert (debugp->r_state == RT_CONSISTENT);

  mem = mmap (NULL, 2 * PAGE_SIZE, PROT_READ | PROT_WRITE,
	      MAP_SHARED | MAP_ANONYMOUS, -1, 0);
  assert_perror (errno);
  assert (mem != NULL);
  i = munmap (mem, PAGE_SIZE);
  assert_perror (errno);
  assert (i == 0);
  mem += PAGE_SIZE;

  debugp = mem - offsetof (struct r_debug, r_map);
  debugp->r_map = &linkmap;
  
  dyn->d_un.d_ptr = (uintptr_t) debugp;

  pause ();
  return 0;
}

gdb/
2011-10-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Paul Pluzhnikov  <ppluzhnikov@google.com>

	* defs.h (struct so_list): New forward declaration.
	(make_cleanup_free_so): New declaration.
	* solib-svr4.c (ignore_first_link_map_entry): Remove.
	(svr4_free_so): Move here.  Handle NULL so->lm_info.
	(svr4_free_library_list): New.
	(svr4_read_so_list): New, moved here code from svr4_current_sos.
	Use more cleanups.  Use new parameter ignore_first instead of
	ignore_first_link_map_entry.
	(svr4_current_sos): New variable ignore_first, initialize it.  New
	variable back_to, use it for svr4_free_library_list protection.
	(svr4_free_so): Remove - move upwards.
	* utils.c: Include solist.h.
	(do_free_so, make_cleanup_free_so): New functions.

--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -371,6 +371,9 @@ extern struct cleanup *
 extern struct cleanup *make_cleanup_value_free_to_mark (struct value *);
 extern struct cleanup *make_cleanup_value_free (struct value *);
 
+struct so_list;
+extern struct cleanup *make_cleanup_free_so (struct so_list *so);
+
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_my_cleanup (struct cleanup **,
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -302,17 +302,6 @@ lm_name (struct so_list *so)
 				ptr_type);
 }
 
-static int
-ignore_first_link_map_entry (struct so_list *so)
-{
-  /* Assume that everything is a library if the dynamic loader was loaded
-     late by a static executable.  */
-  if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
-    return 0;
-
-  return lm_prev (so) == 0;
-}
-
 /* Per pspace SVR4 specific data.  */
 
 struct svr4_info
@@ -942,6 +931,32 @@ open_symbol_file_object (void *from_ttyp)
   return 1;
 }
 
+/* Implementation for target_so_ops.free_so.  */
+
+static void
+svr4_free_so (struct so_list *so)
+{
+  if (so->lm_info)
+    xfree (so->lm_info->lm);
+  xfree (so->lm_info);
+}
+
+/* Free so_list built so far (called via cleanup).  */
+
+static void
+svr4_free_library_list (void *p_list)
+{
+  struct so_list *list = *(struct so_list **) p_list;
+
+  while (list != NULL)
+    {
+      struct so_list *next = list->next;
+
+      svr4_free_so (list);
+      list = next;
+    }
+}
+
 /* If no shared library information is available from the dynamic
    linker, build a fallback list from other sources.  */
 
@@ -971,47 +986,31 @@ svr4_default_sos (void)
   return new;
 }
 
-/* Implement the "current_sos" target_so_ops method.  */
+/* Read the whole inferior libraries chain starting at address LM.  Add the
+   entries to the tail referenced by LINK_PTR_PTR.  Ignore the first entry if
+   IGNORE_FIRST.  */
 
-static struct so_list *
-svr4_current_sos (void)
+static void
+svr4_read_so_list (CORE_ADDR lm, struct so_list ***link_ptr_ptr,
+		   int ignore_first)
 {
-  CORE_ADDR lm, prev_lm;
-  struct so_list *head = 0;
-  struct so_list **link_ptr = &head;
-  CORE_ADDR ldsomap = 0;
-  struct svr4_info *info;
-
-  info = get_svr4_info ();
-
-  /* Always locate the debug struct, in case it has moved.  */
-  info->debug_base = 0;
-  locate_base (info);
-
-  /* If we can't find the dynamic linker's base structure, this
-     must not be a dynamically linked executable.  Hmm.  */
-  if (! info->debug_base)
-    return svr4_default_sos ();
+  CORE_ADDR prev_lm = 0, next_lm;
 
-  /* Walk the inferior's link map list, and build our list of
-     `struct so_list' nodes.  */
-  prev_lm = 0;
-  lm = solib_svr4_r_map (info);
-
-  while (lm)
+  for (; lm != 0; prev_lm = lm, lm = next_lm)
     {
       struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
-      struct so_list *new = XZALLOC (struct so_list);
-      struct cleanup *old_chain = make_cleanup (xfree, new);
-      CORE_ADDR next_lm;
+      struct so_list *new;
+      struct cleanup *old_chain;
+      int errcode;
+      char *buffer;
 
-      new->lm_info = xmalloc (sizeof (struct lm_info));
-      make_cleanup (xfree, new->lm_info);
+      new = XZALLOC (struct so_list);
+      old_chain = make_cleanup_free_so (new);
 
-      new->lm_info->l_addr = (CORE_ADDR)-1;
+      new->lm_info = xmalloc (sizeof (struct lm_info));
+      new->lm_info->l_addr = (CORE_ADDR) -1;
       new->lm_info->lm_addr = lm;
       new->lm_info->lm = xzalloc (lmo->link_map_size);
-      make_cleanup (xfree, new->lm_info->lm);
 
       read_memory (lm, new->lm_info->lm, lmo->link_map_size);
 
@@ -1020,8 +1019,8 @@ svr4_current_sos (void)
       if (lm_prev (new) != prev_lm)
 	{
 	  warning (_("Corrupted shared library list"));
-	  free_so (new);
-	  next_lm = 0;
+	  do_cleanups (old_chain);
+	  break;
 	}
 
       /* For SVR4 versions, the first entry in the link map is for the
@@ -1029,58 +1028,93 @@ svr4_current_sos (void)
          SVR4, it has no name.  For others (Solaris 2.3 for example), it
          does have a name, so we can no longer use a missing name to
          decide when to ignore it.  */
-      else if (ignore_first_link_map_entry (new) && ldsomap == 0)
+      if (ignore_first && lm_prev (new) == 0)
 	{
+	  struct svr4_info *info = get_svr4_info ();
+
 	  info->main_lm_addr = new->lm_info->lm_addr;
-	  free_so (new);
+	  do_cleanups (old_chain);
+	  continue;
 	}
-      else
-	{
-	  int errcode;
-	  char *buffer;
-
-	  /* Extract this shared object's name.  */
-	  target_read_string (lm_name (new), &buffer,
-			      SO_NAME_MAX_PATH_SIZE - 1, &errcode);
-	  if (errcode != 0)
-	    warning (_("Can't read pathname for load map: %s."),
-		     safe_strerror (errcode));
-	  else
-	    {
-	      strncpy (new->so_name, buffer, SO_NAME_MAX_PATH_SIZE - 1);
-	      new->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
-	      strcpy (new->so_original_name, new->so_name);
-	    }
-	  xfree (buffer);
 
-	  /* If this entry has no name, or its name matches the name
-	     for the main executable, don't include it in the list.  */
-	  if (! new->so_name[0]
-	      || match_main (new->so_name))
-	    free_so (new);
-	  else
-	    {
-	      new->next = 0;
-	      *link_ptr = new;
-	      link_ptr = &new->next;
-	    }
+      /* Extract this shared object's name.  */
+      target_read_string (lm_name (new), &buffer,
+			  SO_NAME_MAX_PATH_SIZE - 1, &errcode);
+      if (errcode != 0)
+	{
+	  warning (_("Can't read pathname for load map: %s."),
+		   safe_strerror (errcode));
+	  do_cleanups (old_chain);
+	  continue;
 	}
 
-      prev_lm = lm;
-      lm = next_lm;
+      strncpy (new->so_name, buffer, SO_NAME_MAX_PATH_SIZE - 1);
+      new->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
+      strcpy (new->so_original_name, new->so_name);
+      xfree (buffer);
 
-      /* On Solaris, the dynamic linker is not in the normal list of
-	 shared objects, so make sure we pick it up too.  Having
-	 symbol information for the dynamic linker is quite crucial
-	 for skipping dynamic linker resolver code.  */
-      if (lm == 0 && ldsomap == 0)
+      /* If this entry has no name, or its name matches the name
+	 for the main executable, don't include it in the list.  */
+      if (! new->so_name[0] || match_main (new->so_name))
 	{
-	  lm = ldsomap = solib_svr4_r_ldsomap (info);
-	  prev_lm = 0;
+	  do_cleanups (old_chain);
+	  continue;
 	}
 
       discard_cleanups (old_chain);
+      new->next = 0;
+      **link_ptr_ptr = new;
+      *link_ptr_ptr = &new->next;
     }
+}
+
+/* Implement the "current_sos" target_so_ops method.  */
+
+static struct so_list *
+svr4_current_sos (void)
+{
+  CORE_ADDR lm;
+  struct so_list *head = NULL;
+  struct so_list **link_ptr = &head;
+  struct svr4_info *info;
+  struct cleanup *back_to;
+  int ignore_first;
+
+  info = get_svr4_info ();
+
+  /* Always locate the debug struct, in case it has moved.  */
+  info->debug_base = 0;
+  locate_base (info);
+
+  /* If we can't find the dynamic linker's base structure, this
+     must not be a dynamically linked executable.  Hmm.  */
+  if (! info->debug_base)
+    return svr4_default_sos ();
+
+  /* Assume that everything is a library if the dynamic loader was loaded
+     late by a static executable.  */
+  if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
+    ignore_first = 0;
+  else
+    ignore_first = 1;
+
+  back_to = make_cleanup (svr4_free_library_list, &head);
+
+  /* Walk the inferior's link map list, and build our list of
+     `struct so_list' nodes.  */
+  lm = solib_svr4_r_map (info);
+  if (lm)
+    svr4_read_so_list (lm, &link_ptr, ignore_first);
+
+  /* On Solaris, the dynamic linker is not in the normal list of
+     shared objects, so make sure we pick it up too.  Having
+     symbol information for the dynamic linker is quite crucial
+     for skipping dynamic linker resolver code.  */
+  lm = solib_svr4_r_ldsomap (info);
+  if (lm)
+    svr4_read_so_list (lm, &link_ptr, 0);
+
+  discard_cleanups (back_to);
 
   if (head == NULL)
     return svr4_default_sos ();
@@ -2048,14 +2082,6 @@ svr4_clear_solib (void)
   info->debug_loader_name = NULL;
 }
 
-static void
-svr4_free_so (struct so_list *so)
-{
-  xfree (so->lm_info->lm);
-  xfree (so->lm_info);
-}
-
-
 /* Clear any bits of ADDR that wouldn't fit in a target-format
    data pointer.  "Data pointer" here refers to whatever sort of
    address the dynamic linker uses to manage its sections.  At the
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -59,6 +59,7 @@
 #include "gdbcore.h"
 #include "top.h"
 #include "main.h"
+#include "solist.h"
 
 #include "inferior.h"		/* for signed_pointer_to_address */
 
@@ -464,6 +465,24 @@ make_cleanup_value_free (struct value *value)
   return make_my_cleanup (&cleanup_chain, do_value_free, value);
 }
 
+/* Helper for make_cleanup_free_so.  */
+
+static void
+do_free_so (void *arg)
+{
+  struct so_list *so = arg;
+
+  free_so (so);
+}
+
+/* Make cleanup handler calling free_so for SO.  */
+
+struct cleanup *
+make_cleanup_free_so (struct so_list *so)
+{
+  return make_my_cleanup (&cleanup_chain, do_free_so, so);
+}
+
 struct cleanup *
 make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
 		  void *arg,  void (*free_arg) (void *))


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]