Re: [PATCH] Remove gdb_bfd_stash_filename to fix crash with fix of binutils/11983

[Pedro Alves <palves at redhat dot com>]

On 01/07/2014 12:35 PM, Pedro Alves wrote:
> On 01/06/2014 09:06 PM, Tom Tromey wrote:
>>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>>
>> Doug> I would prefer a new bfd routine to set the file name.
>> Doug> Then *it* is responsible for freeing the old name.
>>
>> Doug> Any reason to not go that route?
>>
>> It seems like a reasonable cleanup to me.
> 
> Hmm.  It actually seems odd and wrong to me for bfd (a library)
> to use xstrdup (a routine that aborts on error).  Actually,
> the only uses of xstrdup in bfd are exactly in the
> filename handling.  There are only 5 uses of xmalloc in bfd,
> and I'll guess they're a mistake where either bfd_malloc or
> bfd_alloc should be used instead.
> 
> gdb has been confused and went in circles, with bfd's filename
> ownership.  In some places, it ended up xmalloc/xstrdup'ing the
> filename instead of allocating it in the bfd's memory.
> That resulted in the invention of gdb_bfd_stash_filename
> https://sourceware.org/ml/gdb-patches/2012-07/msg00291.html
> as a workaround.
> 
> I think it'd be better to allocate the filename
> in the bfd's memory, like it used to be.  In bfd, most places
> already have the filename in some structure that is
> itself already allocated in the bfd (with bfd_alloc),
> it can just point the filename to that memory.  The problematic
> case of binutils/11983 that led to xstrdups was the bfd_open*
> routines, which were just copying the caller's pointer, which
> obviously couldn't have been in the bfd's memory, because, well,
> the caller doesn't have a bfd yet, it's asking bfd to create one.
> 
> So instead of xstrdup in those bfd_open* routines, we should
> IMO use bfd_alloc in that spot too instead, and handle memory
> errors gracefully in addition.  Actually as that's done more than
> a few times, I added a bfd_strdup function.  Then we make gdb do
> what it should have always done -- allocate the filename in
> the bfd's memory throughout.
> 
> WDYT?
> 
> Tested on x86_64 Fedora 17, gdb and binutils.
> 

> Subject: [PATCH] bfd/ 2014-01-07  Pedro Alves  <palves@redhat.com>

Bah, I noticed now that I sent the wrong patch.  That one didn't
even build...

Here's the right one.

-----
Subject: [PATCH] Switch back to allocating the bfd's filename in the bfd's
 memory.

bfd/
2014-01-07  Pedro Alves  <palves@redhat.com>

	PR binutils/11983
	* archive.c (_bfd_get_elt_at_filepos): Don't xstrdup the filename.
	On error, set the bfd's filename to a const string.
	* bfd-in2.h: Regenerate.
	* elfcode.h: Don't include libiberty.h.
	(bfd_from_remote_memory): Don't xstrdup the bfd's filename.
	* ieee.c: Don't include libiberty.h.
	(ieee_object_p): Don't xstrdup the bfd's filename.
	* mach-o.c (bfd_mach_o_fat_member_init): Don't xstrdup the
	architecture's printable name, it's const.  If allocating a
	filename, allocate it on the bfd's memory.
	* oasys.c: Don't include libiberty.h.
	(oasys_openr_next_archived_file): Don't xstrdup the bfd's
	filename.
	* opncls.c (_bfd_delete_bfd): Don't free the bfd's filename.
	(bfd_fopen, bfd_openstreamr, bfd_openr_iovec, bfd_openw)
	(bfd_create): Use bfd_strdup to copy the filename and handle
	memory error.
	(bfd_strdup): New function.
	* syms.c (_bfd_stab_section_find_nearest_line): Delete coment.
	* vms-lib.c: Don't include libiberty.h.
	(_bfd_vms_lib_get_module): Don't xstrdup the bfd's filename.

gdb/
2014-01-07  Pedro Alves  <palves@redhat.com>

	PR binutils/11983
	* gdb_bfd.c (gdb_bfd_strdup): New function.
	* gdb_bfd.h (gdb_bfd_strdup): Declare.
	* solib-aix.c (solib_aix_bfd_open): Don't free the bfd's previous
	filename.  Use gdb_bfd_strdup instead of xstrdup.
	* solib-spu.c (spu_bfd_open): Likewise.
	* spu-linux-nat.c (spu_bfd_open): Likewise.
	* symfile-mem.c (symbol_file_add_from_memory): Don't free the
	bfd's previous filename.  If passed a filename, dup it into bfd's
	memory.  If not, set the bfd's filename to a const read only
	string.
	(add_vsyscall_page): Free filename.
	* solib-darwin.c (darwin_bfd_open): Don't free the bfd's previous
	filename.  Use gdb_bfd_strdup instead of xstrdup.
---
 bfd/archive.c       |  3 ++-
 bfd/bfd-in2.h       |  4 +++-
 bfd/elfcode.h       |  3 +--
 bfd/ieee.c          |  3 +--
 bfd/mach-o.c        |  4 ++--
 bfd/oasys.c         |  3 +--
 bfd/opncls.c        | 63 +++++++++++++++++++++++++++++++++++++++++++++++------
 bfd/syms.c          |  3 ---
 bfd/vms-lib.c       |  3 +--
 gdb/gdb_bfd.c       | 14 ++++++++++++
 gdb/gdb_bfd.h       |  4 ++++
 gdb/solib-aix.c     |  3 +--
 gdb/solib-darwin.c  |  3 +--
 gdb/solib-spu.c     |  3 +--
 gdb/spu-linux-nat.c |  3 +--
 gdb/symfile-mem.c   |  6 ++---
 16 files changed, 92 insertions(+), 33 deletions(-)

diff --git a/bfd/archive.c b/bfd/archive.c
index dc39751..8f59395 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -705,7 +705,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
   else
     {
       n_nfd->origin = n_nfd->proxy_origin;
-      n_nfd->filename = xstrdup (filename);
+      n_nfd->filename = filename;
     }
 
   n_nfd->arelt_data = new_areldata;
@@ -718,6 +718,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
 
   free (new_areldata);
   n_nfd->arelt_data = NULL;
+  n_nfd->filename = "<out of memory>";
   return NULL;
 }
 
diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 71996db..46eb7d1 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -1029,7 +1029,7 @@ bfd *bfd_openr (const char *filename, const char *target);
 
 bfd *bfd_fdopenr (const char *filename, const char *target, int fd);
 
-bfd *bfd_openstreamr (const char *, const char *, void *);
+bfd *bfd_openstreamr (const char * filename, const char * target, void * stream);
 
 bfd *bfd_openr_iovec (const char *filename, const char *target,
     void *(*open_func) (struct bfd *nbfd,
@@ -1060,6 +1060,8 @@ bfd_boolean bfd_make_readable (bfd *abfd);
 
 void *bfd_alloc (bfd *abfd, bfd_size_type wanted);
 
+char *bfd_strdup (bfd *abfd, const char *str);
+
 void *bfd_zalloc (bfd *abfd, bfd_size_type wanted);
 
 unsigned long bfd_calc_gnu_debuglink_crc32
diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 0328748..c3135f5 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -71,7 +71,6 @@
 #include "bfdlink.h"
 #include "libbfd.h"
 #include "elf-bfd.h"
-#include "libiberty.h"
 
 /* Renaming structures, typedefs, macros and functions to be size-specific.  */
 #define Elf_External_Ehdr	NAME(Elf,External_Ehdr)
@@ -1803,7 +1802,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
       bfd_set_error (bfd_error_no_memory);
       return NULL;
     }
-  nbfd->filename = xstrdup ("<in-memory>");
+  nbfd->filename = "<in-memory>";
   nbfd->xvec = templ->xvec;
   bim->size = contents_size;
   bim->buffer = contents;
diff --git a/bfd/ieee.c b/bfd/ieee.c
index e1734ec..237736c 100644
--- a/bfd/ieee.c
+++ b/bfd/ieee.c
@@ -33,7 +33,6 @@
 #include "ieee.h"
 #include "libieee.h"
 #include "safe-ctype.h"
-#include "libiberty.h"
 
 struct output_buffer_struct
 {
@@ -1823,7 +1822,7 @@ ieee_object_p (bfd *abfd)
     goto got_wrong_format;
   ieee->mb.module_name = read_id (&(ieee->h));
   if (abfd->filename == (const char *) NULL)
-    abfd->filename = xstrdup (ieee->mb.module_name);
+    abfd->filename = ieee->mb.module_name;
 
   /* Determine the architecture and machine type of the object file.  */
   {
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index 6640a6a..ffe7332 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -4353,13 +4353,13 @@ bfd_mach_o_fat_member_init (bfd *abfd,
   if (ap)
     {
       /* Use the architecture name if known.  */
-      abfd->filename = xstrdup (ap->printable_name);
+      abfd->filename = ap->printable_name;
     }
   else
     {
       /* Forge a uniq id.  */
       const size_t namelen = 2 + 8 + 1 + 2 + 8 + 1;
-      char *name = xmalloc (namelen);
+      char *name = bfd_alloc (abfd, namelen);
       snprintf (name, namelen, "0x%lx-0x%lx",
                 entry->cputype, entry->cpusubtype);
       abfd->filename = name;
diff --git a/bfd/oasys.c b/bfd/oasys.c
index b8e457e..671d4c7 100644
--- a/bfd/oasys.c
+++ b/bfd/oasys.c
@@ -26,7 +26,6 @@
 #include "libbfd.h"
 #include "oasys.h"
 #include "liboasys.h"
-#include "libiberty.h"
 
 /* Read in all the section data and relocation stuff too.  */
 
@@ -1117,7 +1116,7 @@ oasys_openr_next_archived_file (bfd *arch, bfd *prev)
 	{
 	  p->abfd = _bfd_create_empty_archive_element_shell (arch);
 	  p->abfd->origin = p->pos;
-	  p->abfd->filename = xstrdup (p->name);
+	  p->abfd->filename = p->name;
 
 	  /* Fixup a pointer to this element for the member.  */
 	  p->abfd->arelt_data = (void *) p;
diff --git a/bfd/opncls.c b/bfd/opncls.c
index 54744ce..4ef474e 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -123,8 +123,6 @@ _bfd_delete_bfd (bfd *abfd)
       objalloc_free ((struct objalloc *) abfd->memory);
     }
 
-  if (abfd->filename)
-    free ((char *) abfd->filename);
   free (abfd->arelt_data);
   free (abfd);
 }
@@ -228,7 +226,12 @@ bfd_fopen (const char *filename, const char *target, const char *mode, int fd)
 
   /* PR 11983: Do not cache the original filename, but
      rather make a copy - the original might go away.  */
-  nbfd->filename = xstrdup (filename);
+  nbfd->filename = bfd_strdup (nbfd, filename);
+  if (nbfd->filename == NULL)
+    {
+      _bfd_delete_bfd (nbfd);
+      return NULL;
+    }
 
   /* Figure out whether the user is opening the file for reading,
      writing, or both, by looking at the MODE argument.  */
@@ -395,7 +398,13 @@ bfd_openstreamr (const char *filename, const char *target, void *streamarg)
   nbfd->iostream = stream;
   /* PR 11983: Do not cache the original filename, but
      rather make a copy - the original might go away.  */
-  nbfd->filename = xstrdup (filename);
+  nbfd->filename = bfd_strdup (nbfd, filename);
+  if (nbfd->filename == NULL)
+    {
+      _bfd_delete_bfd (nbfd);
+      return NULL;
+    }
+
   nbfd->direction = read_direction;
 
   if (! bfd_cache_init (nbfd))
@@ -589,7 +598,12 @@ bfd_openr_iovec (const char *filename, const char *target,
 
   /* PR 11983: Do not cache the original filename, but
      rather make a copy - the original might go away.  */
-  nbfd->filename = xstrdup (filename);
+  nbfd->filename = bfd_strdup (nbfd, filename);
+  if (nbfd->filename == NULL)
+    {
+      _bfd_delete_bfd (nbfd);
+      return NULL;
+    }
   nbfd->direction = read_direction;
 
   /* `open_p (...)' would get expanded by an the open(2) syscall macro.  */
@@ -656,7 +670,12 @@ bfd_openw (const char *filename, const char *target)
 
   /* PR 11983: Do not cache the original filename, but
      rather make a copy - the original might go away.  */
-  nbfd->filename = xstrdup (filename);
+  nbfd->filename = bfd_strdup (nbfd, filename);
+  if (nbfd->filename == NULL)
+    {
+      _bfd_delete_bfd (nbfd);
+      return NULL;
+    }
   nbfd->direction = write_direction;
 
   if (bfd_open_file (nbfd) == NULL)
@@ -808,7 +827,12 @@ bfd_create (const char *filename, bfd *templ)
     return NULL;
   /* PR 11983: Do not cache the original filename, but
      rather make a copy - the original might go away.  */
-  nbfd->filename = xstrdup (filename);
+  nbfd->filename = bfd_strdup (nbfd, filename);
+  if (nbfd->filename == NULL)
+    {
+      _bfd_delete_bfd (nbfd);
+      return NULL;
+    }
   if (templ)
     nbfd->xvec = templ->xvec;
   nbfd->direction = no_direction;
@@ -991,6 +1015,31 @@ bfd_alloc2 (bfd *abfd, bfd_size_type nmemb, bfd_size_type size)
 
 /*
 FUNCTION
+	bfd_strdup
+
+SYNOPSIS
+	char *bfd_strdup (bfd *abfd, const char *str);
+
+DESCRIPTION
+        Copy a string into memory attached to <<abfd>> and return a
+        pointer to it.
+*/
+
+char *
+bfd_strdup (bfd *abfd, const char *str)
+{
+  bfd_size_type size;
+  char *p;
+
+  size = strlen (str) + 1;
+  p = bfd_alloc (abfd, size);
+  if (p != NULL)
+    memcpy (p, str, size);
+  return p;
+}
+
+/*
+FUNCTION
 	bfd_zalloc
 
 SYNOPSIS
diff --git a/bfd/syms.c b/bfd/syms.c
index 27b40eb..aa4718f 100644
--- a/bfd/syms.c
+++ b/bfd/syms.c
@@ -1383,9 +1383,6 @@ _bfd_stab_section_find_nearest_line (bfd *abfd,
 	{
 	  size_t len;
 
-	  /* Don't free info->filename here.  objdump and other
-	     apps keep a copy of a previously returned file name
-	     pointer.  */
 	  len = strlen (file_name) + 1;
 	  info->filename = (char *) bfd_alloc (abfd, dirlen + len);
 	  if (info->filename == NULL)
diff --git a/bfd/vms-lib.c b/bfd/vms-lib.c
index 407c186..afbb54a 100644
--- a/bfd/vms-lib.c
+++ b/bfd/vms-lib.c
@@ -25,7 +25,6 @@
 #include "libbfd.h"
 #include "safe-ctype.h"
 #include "bfdver.h"
-#include "libiberty.h"
 #include "vms.h"
 #include "vms/lbr.h"
 #include "vms/dcx.h"
@@ -1377,7 +1376,7 @@ _bfd_vms_lib_get_module (bfd *abfd, unsigned int modidx)
     default:
       break;
     }
-  res->filename = xstrdup (name);
+  res->filename = name;
 
   tdata->cache[modidx] = res;
 
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 5230d21..9d84815 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -634,6 +634,20 @@ gdb_bfd_requires_relocations (bfd *abfd)
   return gdata->needs_relocations;
 }
 
+/* See gdb_bfd.h.  */
+
+char *
+gdb_bfd_strdup (bfd *abfd, const char *str)
+{
+  char *p;
+
+  p = bfd_strdup (abfd, str);
+  if (p == NULL)
+    malloc_failure (0);
+
+  return p;
+}
+
 
 
 /* A callback for htab_traverse that prints a single BFD.  */
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index d415005..502c131 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -70,6 +70,10 @@ const gdb_byte *gdb_bfd_map_section (asection *section, bfd_size_type *size);
 
 int gdb_bfd_crc (struct bfd *abfd, unsigned long *crc_out);
 
+/* A wrapper for bfd_strdup that never returns NULL.  */
+
+char *gdb_bfd_strdup (bfd *abfd, const char *str);
+
 
 
 /* A wrapper for bfd_fopen that initializes the gdb-specific reference
diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index fefa51f..9191667 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -728,8 +728,7 @@ solib_aix_bfd_open (char *pathname)
      to allow commands listing all shared libraries to display that
      synthetic name.  Otherwise, we would only be displaying the name
      of the archive member object.  */
-  xfree (bfd_get_filename (object_bfd));
-  object_bfd->filename = xstrdup (pathname);
+  object_bfd->filename = gdb_bfd_strdup (object_bfd, pathname);
 
   gdb_bfd_unref (archive_bfd);
   do_cleanups (cleanup);
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index ba807a2..af8275b 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -621,8 +621,7 @@ darwin_bfd_open (char *pathname)
   /* The current filename for fat-binary BFDs is a name generated
      by BFD, usually a string containing the name of the architecture.
      Reset its value to the actual filename.  */
-  xfree (bfd_get_filename (res));
-  res->filename = xstrdup (pathname);
+  res->filename = gdb_bfd_strdup (res, pathname);
 
   gdb_bfd_unref (abfd);
   return res;
diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
index abb8c15..2dedd5a 100644
--- a/gdb/solib-spu.c
+++ b/gdb/solib-spu.c
@@ -382,8 +382,7 @@ spu_bfd_open (char *pathname)
 
 	  strcat (buf, original_name);
 
-	  xfree ((char *)abfd->filename);
-	  abfd->filename = xstrdup (buf);
+	  abfd->filename = gdb_bfd_strdup (abfd, buf);
 	}
     }
 
diff --git a/gdb/spu-linux-nat.c b/gdb/spu-linux-nat.c
index e9b155b..1c8c5c7 100644
--- a/gdb/spu-linux-nat.c
+++ b/gdb/spu-linux-nat.c
@@ -341,8 +341,7 @@ spu_bfd_open (ULONGEST addr)
 	  bfd_get_section_contents (nbfd, spu_name, buf, 20, sect_size - 20);
 	  buf[sect_size - 20] = '\0';
 
-	  xfree ((char *)nbfd->filename);
-	  nbfd->filename = xstrdup (buf);
+	  nbfd->filename = gdb_bfd_strdup (nbfd, buf);
 	}
     }
 
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index e3230de..652c032 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -101,11 +101,10 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name,
     error (_("Failed to read a valid object file image from memory."));
 
   gdb_bfd_ref (nbfd);
-  xfree (bfd_get_filename (nbfd));
   if (name == NULL)
-    nbfd->filename = xstrdup ("shared object read from target memory");
+    nbfd->filename = "shared object read from target memory";
   else
-    nbfd->filename = name;
+    nbfd->filename = gdb_bfd_strdup (nbfd, name);
 
   cleanup = make_cleanup_bfd_unref (nbfd);
 
@@ -225,6 +224,7 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
       args.from_tty = 0;
       catch_exceptions (current_uiout, symbol_file_add_from_memory_wrapper,
 			&args, RETURN_MASK_ALL);
+      xfree (args.name);
     }
 }
 
-- 
1.7.11.7