From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16253 invoked by alias); 7 Jan 2014 13:55:25 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 16233 invoked by uid 89); 7 Jan 2014 13:55:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 07 Jan 2014 13:55:23 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s07DtHr4018462 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 Jan 2014 08:55:17 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s07DtCAe012433; Tue, 7 Jan 2014 08:55:13 -0500 Message-ID: <52CC0740.6040005@redhat.com> Date: Tue, 07 Jan 2014 13:55:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Binutils Development CC: Tom Tromey , Doug Evans , Hui Zhu , Sergio Durigan Junior , gdb-patches ml , Edjunior Barbosa Machado , Nick Clifton Subject: Re: [PATCH] Remove gdb_bfd_stash_filename to fix crash with fix of binutils/11983 References: <52C8358B.7080101@mentor.com> <52C97EC0.3080807@mentor.com> <87k3edseia.fsf@fleche.redhat.com> <52CA8A7F.7090907@mentor.com> <878uuslt1j.fsf@fleche.redhat.com> <52CBF47C.9010002@redhat.com> In-Reply-To: <52CBF47C.9010002@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-01/txt/msg00140.txt.bz2 On 01/07/2014 12:35 PM, Pedro Alves wrote: > On 01/06/2014 09:06 PM, Tom Tromey wrote: >>>>>>> "Doug" == Doug Evans 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 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 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 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 = ""; 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 (""); + nbfd->filename = ""; 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 <> 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