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

[Pedro Alves <palves at redhat dot com>]

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>

	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..d76b782 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 = "<no 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..e5b43a1 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 (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