From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32681 invoked by alias); 7 Jan 2014 12:35:20 -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 32662 invoked by uid 89); 7 Jan 2014 12:35:19 -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 12:35:17 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s07CZB9X008250 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 Jan 2014 07:35:12 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s07CZ8j9023951; Tue, 7 Jan 2014 07:35:09 -0500 Message-ID: <52CBF47C.9010002@redhat.com> Date: Tue, 07 Jan 2014 12:35: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: Tom Tromey CC: Doug Evans , Hui Zhu , Sergio Durigan Junior , gdb-patches ml , Edjunior Barbosa Machado , Nick Clifton , Binutils Development 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> In-Reply-To: <878uuslt1j.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-01/txt/msg00135.txt.bz2 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 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..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 = ""; 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..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