This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Remove gdb_bfd_stash_filename to fix crash with fix of binutils/11983
- From: Pedro Alves <palves at redhat dot com>
- To: Binutils Development <binutils at sourceware dot org>
- Cc: Tom Tromey <tromey at redhat dot com>, Doug Evans <dje at google dot com>, Hui Zhu <hui_zhu at mentor dot com>, Sergio Durigan Junior <sergiodj at redhat dot com>, gdb-patches ml <gdb-patches at sourceware dot org>, Edjunior Barbosa Machado <emachado at linux dot vnet dot ibm dot com>, Nick Clifton <nickc at redhat dot com>
- Date: Tue, 07 Jan 2014 13:55:12 +0000
- Subject: Re: [PATCH] Remove gdb_bfd_stash_filename to fix crash with fix of binutils/11983
- Authentication-results: sourceware.org; auth=none
- References: <52C8358B dot 7080101 at mentor dot com> <m3mwja5v01 dot fsf at redhat dot com> <52C97EC0 dot 3080807 at mentor dot com> <87k3edseia dot fsf at fleche dot redhat dot com> <52CA8A7F dot 7090907 at mentor dot com> <CADPb22QzyGuTEERURKao991F4jttvMqSrxodQz3JU2MLNgQ=tg at mail dot gmail dot com> <878uuslt1j dot fsf at fleche dot redhat dot com> <52CBF47C dot 9010002 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