PR25993, read of freed memory

Message ID 20200519043205.GT1088@bubble.grove.modra.org
State New
Headers show
Series
  • PR25993, read of freed memory
Related show

Commit Message

ldmain.c:add_archive_element copies file name pointers from the bfd to
a lang_input_statement_type.
  input->filename = abfd->filename;
  input->local_sym_name = abfd->filename;
This results in stale pointers when twiddling the bfd filename in
places like the pe ld after_open.  So don't free the bfd filename,
and make copies using bfd_alloc memory that won't result in small
memory leaks that annoy memory checkers.

The patch that has already been applied for PR25993 can leave
local_sym_name dangling into freed memory, and I think it might even
be possible for ldlang.c:lookup_name to read this memory.

Since this patch touches gdb files, OK to apply there after the 9.2
release?  Note that I don't properly handle a NULL return from
bfd_set_filename (out of memory condition).  I went looking to see
what xstrprintf does, and found gdb gives an internal_error.  Really??

	PR 25993
bfd/
	* archive.c (_bfd_get_elt_at_filepos): Don't strdup filename,
	use bfd_set_filename.
	* elfcode.h (_bfd_elf_bfd_from_remote_memory): Likewise.
	* mach-o.c (bfd_mach_o_fat_member_init): Likewise.
	* opncls.c (bfd_fopen, bfd_openstreamr, bfd_openr_iovec, bfd_openw),
	(bfd_create): Likewise.
	(_bfd_delete_bfd): Don't free filename.
	(bfd_set_filename): Copy filename param to bfd_alloc'd memory,
	return pointer to the copy or NULL on alloc fail.
	* vms-lib.c (_bfd_vms_lib_get_module): Free newname and test
	result of bfd_set_filename.
	* bfd-in2.h: Regenerate.
gdb/
	* solib-darwin.c (darwin_bfd_open): Don't strdup pathname for
	bfd_set_filename.
	* solib-aix.c (solib_aix_bfd_open): Free name after calling
	bfd_set_filename.
	* symfile-mem.c (symbol_file_add_from_memory): Likewise.
ld/
	* emultempl/pe.em (gld_${EMULATION_NAME}_after_open): Don't copy
	other_bfd_filename for bfd_set_filename, and test result of
	bfd_set_filename call.  Don't create a new is->filename, simply
	copy from bfd filename.  Free new_name after bfd_set_filename.
	* emultempl/pep.em (gld_${EMULATION_NAME}_after_open): Likewise.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

Simon Marchi May 19, 2020, 1:27 p.m. | #1
On 2020-05-19 12:32 a.m., Alan Modra via Gdb-patches wrote:
> ldmain.c:add_archive_element copies file name pointers from the bfd to

> a lang_input_statement_type.

>   input->filename = abfd->filename;

>   input->local_sym_name = abfd->filename;

> This results in stale pointers when twiddling the bfd filename in

> places like the pe ld after_open.  So don't free the bfd filename,

> and make copies using bfd_alloc memory that won't result in small

> memory leaks that annoy memory checkers.

> 

> The patch that has already been applied for PR25993 can leave

> local_sym_name dangling into freed memory, and I think it might even

> be possible for ldlang.c:lookup_name to read this memory.

> 

> Since this patch touches gdb files, OK to apply there after the 9.2

> release?  Note that I don't properly handle a NULL return from

> bfd_set_filename (out of memory condition).  I went looking to see

> what xstrprintf does, and found gdb gives an internal_error.  Really??


Yes, really.  As far as I know there isn't really a contingency plan
in case an allocation fails, the plan is just to abort.  I'm not sure
what we could do that would be useful instead.  But I don't think I've
ever seen it happen anyway.

> 

> 	PR 25993

> bfd/

> 	* archive.c (_bfd_get_elt_at_filepos): Don't strdup filename,

> 	use bfd_set_filename.

> 	* elfcode.h (_bfd_elf_bfd_from_remote_memory): Likewise.

> 	* mach-o.c (bfd_mach_o_fat_member_init): Likewise.

> 	* opncls.c (bfd_fopen, bfd_openstreamr, bfd_openr_iovec, bfd_openw),

> 	(bfd_create): Likewise.

> 	(_bfd_delete_bfd): Don't free filename.

> 	(bfd_set_filename): Copy filename param to bfd_alloc'd memory,

> 	return pointer to the copy or NULL on alloc fail.

> 	* vms-lib.c (_bfd_vms_lib_get_module): Free newname and test

> 	result of bfd_set_filename.

> 	* bfd-in2.h: Regenerate.

> gdb/

> 	* solib-darwin.c (darwin_bfd_open): Don't strdup pathname for

> 	bfd_set_filename.

> 	* solib-aix.c (solib_aix_bfd_open): Free name after calling

> 	bfd_set_filename.

> 	* symfile-mem.c (symbol_file_add_from_memory): Likewise.

> ld/

> 	* emultempl/pe.em (gld_${EMULATION_NAME}_after_open): Don't copy

> 	other_bfd_filename for bfd_set_filename, and test result of

> 	bfd_set_filename call.  Don't create a new is->filename, simply

> 	copy from bfd filename.  Free new_name after bfd_set_filename.

> 	* emultempl/pep.em (gld_${EMULATION_NAME}_after_open): Likewise.

> 

> diff --git a/bfd/archive.c b/bfd/archive.c

> index ff64727c44..1322977744 100644

> --- a/bfd/archive.c

> +++ b/bfd/archive.c

> @@ -737,8 +737,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)

>    else

>      {

>        n_bfd->origin = n_bfd->proxy_origin;

> -      n_bfd->filename = bfd_strdup (filename);

> -      if (n_bfd->filename == NULL)

> +      if (!bfd_set_filename (n_bfd, filename))

>  	goto out;

>      }

>  

> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h

> index ec23fd4987..44bca15d5a 100644

> --- a/bfd/bfd-in2.h

> +++ b/bfd/bfd-in2.h

> @@ -643,7 +643,7 @@ bfd_boolean bfd_fill_in_gnu_debuglink_section

>  

>  char *bfd_follow_build_id_debuglink (bfd *abfd, const char *dir);

>  

> -void bfd_set_filename (bfd *abfd, char *filename);

> +char *bfd_set_filename (bfd *abfd, const char *filename);


Should this return a `const char *`, just like bfd_get_filename?

I haven't inspected all call sites, but it sounds like the caller
shouldn't be able to modify the filename contents.

> diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c

> index 5da1214a25..ddb14fa81d 100644

> --- a/gdb/solib-aix.c

> +++ b/gdb/solib-aix.c

> @@ -637,10 +637,11 @@ solib_aix_bfd_open (const char *pathname)

>       along with appended parenthesized member name in order to allow commands

>       listing all shared libraries to display.  Otherwise, we would only be

>       displaying the name of the archive member object.  */

> -  bfd_set_filename (object_bfd.get (),

> -		    xstrprintf ("%s%s",

> -				bfd_get_filename (archive_bfd.get ()),

> -				sep));

> +  char *fname = xstrprintf ("%s%s",

> +			    bfd_get_filename (archive_bfd.get ()),

> +			    sep);

> +  bfd_set_filename (object_bfd.get (), fname);

> +  free (fname);


Since the string gets copied by bfd_set_filename, let's use std::string
to avoid having to free:

std::string fname = string_printf ("%s%s",
				   bfd_get_filename (archive_bfd.get ()),
				   sep);
bfd_set_filename (object_bfd.get (), fname.c_str ());

> diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c

> index e2d2e43d7f..18de07fd1c 100644

> --- a/gdb/symfile-mem.c

> +++ b/gdb/symfile-mem.c

> @@ -78,8 +78,7 @@ target_read_memory_bfd (bfd_vma memaddr, bfd_byte *myaddr, bfd_size_type len)

>     and read its in-core symbols out of inferior memory.  SIZE, if

>     non-zero, is the known size of the object.  TEMPL is a bfd

>     representing the target's format.  NAME is the name to use for this

> -   symbol file in messages; it can be NULL or a malloc-allocated string

> -   which will be attached to the BFD.  */

> +   symbol file in messages; it can be NULL or a malloc-allocated string.  */

>  static struct objfile *

>  symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,

>  			     size_t size, char *name, int from_tty)

> @@ -104,6 +103,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,

>    if (name == NULL)

>      name = xstrdup ("shared object read from target memory");

>    bfd_set_filename (nbfd, name);

> +  free (name);


It now doesn't really make sense for symbol_file_add_from_memory to accept a malloc-ed
string (the point was that before we gave ownership of that malloc-ed string to bfd).

Can you please change `char *name` to be `gdb::optional<std::string>`?

The caller that passes a name should use string_printf to build the string, as mentioned
above.  The caller that does not pass a name can pass `{}`, to pass an empty optional.

The `if (name == NULL)` would become something like:

  if (!name.has_value ())
    name.emplace ("shared object read from target memory");

And then:

  bfd_set_filename (nbfd, name->c_str ());

Simon
On Tue, May 19, 2020 at 8:27 AM Simon Marchi <simark@simark.ca> wrote:
> Can you please change `char *name` to be `gdb::optional<std::string>`?

>

> The caller that passes a name should use string_printf to build the string, as mentioned

> above.  The caller that does not pass a name can pass `{}`, to pass an empty optional.


We may want to add C++17's std::nullopt to gdb::optional, to make it
clearer what's going on there. Then you could pass gdb::nullopt
instead of {}.

Christian
Simon Marchi May 19, 2020, 5:26 p.m. | #3
On 2020-05-19 1:25 p.m., Christian Biesinger wrote:
> On Tue, May 19, 2020 at 8:27 AM Simon Marchi <simark@simark.ca> wrote:

>> Can you please change `char *name` to be `gdb::optional<std::string>`?

>>

>> The caller that passes a name should use string_printf to build the string, as mentioned

>> above.  The caller that does not pass a name can pass `{}`, to pass an empty optional.

> 

> We may want to add C++17's std::nullopt to gdb::optional, to make it

> clearer what's going on there. Then you could pass gdb::nullopt

> instead of {}.


If it's technically possible, I completely agree.

Simon
On Tue, May 19, 2020 at 12:27 PM Simon Marchi <simark@simark.ca> wrote:
>

> On 2020-05-19 1:25 p.m., Christian Biesinger wrote:

> > On Tue, May 19, 2020 at 8:27 AM Simon Marchi <simark@simark.ca> wrote:

> >> Can you please change `char *name` to be `gdb::optional<std::string>`?

> >>

> >> The caller that passes a name should use string_printf to build the string, as mentioned

> >> above.  The caller that does not pass a name can pass `{}`, to pass an empty optional.

> >

> > We may want to add C++17's std::nullopt to gdb::optional, to make it

> > clearer what's going on there. Then you could pass gdb::nullopt

> > instead of {}.

>

> If it's technically possible, I completely agree.


Yep, it is. Sent a patch to add it.

Christian
On 5/19/20 2:27 PM, Simon Marchi wrote:
> It now doesn't really make sense for symbol_file_add_from_memory to accept a malloc-ed

> string (the point was that before we gave ownership of that malloc-ed string to bfd).

> 

> Can you please change `char *name` to be `gdb::optional<std::string>`?

> 

> The caller that passes a name should use string_printf to build the string, as mentioned

> above.  The caller that does not pass a name can pass `{}`, to pass an empty optional.


Wouldn't 'const char * or NULL' work?  Seems way simpler to me.

> 

> The `if (name == NULL)` would become something like:

> 

>   if (!name.has_value ())

>     name.emplace ("shared object read from target memory");

> 


This would just be

  if (name == NULL)
    name = "shared object read from target memory";

> And then:

> 

>   bfd_set_filename (nbfd, name->c_str ());



Thanks,
Pedro Alves
Simon Marchi May 20, 2020, 12:19 a.m. | #6
On 2020-05-19 7:40 p.m., Alan Modra wrote:
> On Tue, May 19, 2020 at 09:27:15AM -0400, Simon Marchi wrote:

>> On 2020-05-19 12:32 a.m., Alan Modra via Gdb-patches wrote:

>>> -void bfd_set_filename (bfd *abfd, char *filename);

>>> +char *bfd_set_filename (bfd *abfd, const char *filename);

>> Should this return a `const char *`, just like bfd_get_filename?

>>

>> I haven't inspected all call sites, but it sounds like the caller

>> shouldn't be able to modify the filename contents.

> Yes, I've updated the return type.  One minor change needed to

> mach-o.c.

> 

>> Since the string gets copied by bfd_set_filename, let's use std::string

>> to avoid having to free:

> Done, and symfile-mem.c updated as per down-thread suggestion to make

> name a const char*.

> 

> I've left the return status from bfd_set_filename in gdb unchecked,

> ie. the out-of-memory NULL return, since it seems to me that not

> getting the expected name change is a minor detail very likely to be

> lost in some later OOM.


That LGTM for the GDB side, but I get a build failure in bfd/archive.c.  I think
bfd-in2.h needs to be regenerated?  Or maybe in BFD you don't typically include
re-generated files in your patches?

Simon
On Tue, May 19, 2020 at 08:19:08PM -0400, Simon Marchi wrote:
> That LGTM for the GDB side, but I get a build failure in bfd/archive.c.  I think

> bfd-in2.h needs to be regenerated?  Or maybe in BFD you don't typically include

> re-generated files in your patches?


Yes, I stripped that out when posting the patch.  Thanks for reviewing!

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/bfd/archive.c b/bfd/archive.c
index ff64727c44..1322977744 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -737,8 +737,7 @@  _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
   else
     {
       n_bfd->origin = n_bfd->proxy_origin;
-      n_bfd->filename = bfd_strdup (filename);
-      if (n_bfd->filename == NULL)
+      if (!bfd_set_filename (n_bfd, filename))
 	goto out;
     }
 
diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index ec23fd4987..44bca15d5a 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -643,7 +643,7 @@  bfd_boolean bfd_fill_in_gnu_debuglink_section
 
 char *bfd_follow_build_id_debuglink (bfd *abfd, const char *dir);
 
-void bfd_set_filename (bfd *abfd, char *filename);
+char *bfd_set_filename (bfd *abfd, const char *filename);
 
 /* Extracted from libbfd.c.  */
 
diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 68db3e9ee3..5e6b2a430f 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1680,7 +1680,6 @@  NAME(_bfd_elf,bfd_from_remote_memory)
   bfd_vma high_offset;
   bfd_vma shdr_end;
   bfd_vma loadbase;  /* Bytes.  */
-  char *filename;
   size_t amt;
   unsigned int opb = bfd_octets_per_byte (templ, NULL);
 
@@ -1894,22 +1893,14 @@  NAME(_bfd_elf,bfd_from_remote_memory)
       free (contents);
       return NULL;
     }
-  filename = bfd_strdup ("<in-memory>");
-  if (filename == NULL)
-    {
-      free (bim);
-      free (contents);
-      return NULL;
-    }
   nbfd = _bfd_new_bfd ();
-  if (nbfd == NULL)
+  if (nbfd == NULL
+      || !bfd_set_filename (nbfd, "<in-memory>"))
     {
-      free (filename);
       free (bim);
       free (contents);
       return NULL;
     }
-  nbfd->filename = filename;
   nbfd->xvec = templ->xvec;
   bim->size = high_offset;
   bim->buffer = contents;
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index 33bd81e121..ee8c6154b7 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -5578,21 +5578,18 @@  bfd_mach_o_fat_member_init (bfd *abfd,
   if (ap)
     {
       /* Use the architecture name if known.  */
-      filename = bfd_strdup (ap->printable_name);
-      if (filename == NULL)
-	return FALSE;
+      filename = bfd_set_filename (abfd, ap->printable_name);
     }
   else
     {
       /* Forge a uniq id.  */
-      const size_t namelen = 2 + 8 + 1 + 2 + 8 + 1;
-      filename = bfd_malloc (namelen);
-      if (filename == NULL)
-	return FALSE;
-      snprintf (filename, namelen, "0x%lx-0x%lx",
+      char buf[2 + 8 + 1 + 2 + 8 + 1];
+      snprintf (buf, sizeof (buf), "0x%lx-0x%lx",
 		entry->cputype, entry->cpusubtype);
+      filename = bfd_set_filename (abfd, buf);
     }
-  bfd_set_filename (abfd, filename);
+  if (!filename)
+    return FALSE;
 
   areltdata = bfd_zmalloc (sizeof (struct areltdata));
   if (areltdata == NULL)
diff --git a/bfd/opncls.c b/bfd/opncls.c
index 5d3437d382..6c2bec4520 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -126,7 +126,6 @@  _bfd_delete_bfd (bfd *abfd)
       objalloc_free ((struct objalloc *) abfd->memory);
     }
 
-  free ((char *) bfd_get_filename (abfd));
   free (abfd->arelt_data);
   free (abfd);
 }
@@ -232,8 +231,7 @@  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 = bfd_strdup (filename);
-  if (nbfd->filename == NULL)
+  if (!bfd_set_filename (nbfd, filename))
     {
       fclose (nbfd->iostream);
       _bfd_delete_bfd (nbfd);
@@ -406,8 +404,7 @@  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 = bfd_strdup (filename);
-  if (nbfd->filename == NULL)
+  if (!bfd_set_filename (nbfd, filename))
     {
       _bfd_delete_bfd (nbfd);
       return NULL;
@@ -607,8 +604,7 @@  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 = bfd_strdup (filename);
-  if (nbfd->filename == NULL)
+  if (!bfd_set_filename (nbfd, filename))
     {
       _bfd_delete_bfd (nbfd);
       return NULL;
@@ -679,8 +675,7 @@  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 = bfd_strdup (filename);
-  if (nbfd->filename == NULL)
+  if (!bfd_set_filename (nbfd, filename))
     {
       _bfd_delete_bfd (nbfd);
       return NULL;
@@ -824,8 +819,7 @@  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 = bfd_strdup (filename);
-  if (nbfd->filename == NULL)
+  if (!bfd_set_filename (nbfd, filename))
     {
       _bfd_delete_bfd (nbfd);
       return NULL;
@@ -2040,17 +2034,23 @@  FUNCTION
 	bfd_set_filename
 
 SYNOPSIS
-	void bfd_set_filename (bfd *abfd, char *filename);
+	char *bfd_set_filename (bfd *abfd, const char *filename);
 
 DESCRIPTION
-	Set the filename of @var{abfd}.  The old filename, if any, is freed.
-	@var{filename} must be allocated using @code{xmalloc}.  After
-	this call, it is owned @var{abfd}.
+	Set the filename of @var{abfd}, copying the FILENAME parameter to
+	bfd_alloc'd memory owned by @var{abfd}.  Returns a pointer the
+	newly allocated name, or NULL if the allocation failed.
 */
 
-void
-bfd_set_filename (bfd *abfd, char *filename)
+char *
+bfd_set_filename (bfd *abfd, const char *filename)
 {
-  free ((char *) abfd->filename);
-  abfd->filename = filename;
+  size_t len = strlen (filename) + 1;
+  char *n = bfd_alloc (abfd, len);
+  if (n)
+    {
+      memcpy (n, filename, len);
+      abfd->filename = n;
+    }
+  return n;
 }
diff --git a/bfd/vms-lib.c b/bfd/vms-lib.c
index 9504cf4976..6a8a76ecb0 100644
--- a/bfd/vms-lib.c
+++ b/bfd/vms-lib.c
@@ -1452,6 +1452,12 @@  _bfd_vms_lib_get_module (bfd *abfd, unsigned int modidx)
       break;
     }
   bfd_set_filename (res, newname);
+  free (newname);
+  if (bfd_get_filename (res) == NULL)
+    {
+      bfd_close (res);
+      return NULL;
+    }
 
   tdata->cache[modidx] = res;
 
diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index 5da1214a25..ddb14fa81d 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -637,10 +637,11 @@  solib_aix_bfd_open (const char *pathname)
      along with appended parenthesized member name in order to allow commands
      listing all shared libraries to display.  Otherwise, we would only be
      displaying the name of the archive member object.  */
-  bfd_set_filename (object_bfd.get (),
-		    xstrprintf ("%s%s",
-				bfd_get_filename (archive_bfd.get ()),
-				sep));
+  char *fname = xstrprintf ("%s%s",
+			    bfd_get_filename (archive_bfd.get ()),
+			    sep);
+  bfd_set_filename (object_bfd.get (), fname);
+  free (fname);
 
   return object_bfd;
 }
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index 03d91d1690..498d6e2073 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -662,7 +662,7 @@  darwin_bfd_open (const 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.  */
-  bfd_set_filename (res.get (), xstrdup (pathname));
+  bfd_set_filename (res.get (), pathname);
 
   return res;
 }
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index e2d2e43d7f..18de07fd1c 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -78,8 +78,7 @@  target_read_memory_bfd (bfd_vma memaddr, bfd_byte *myaddr, bfd_size_type len)
    and read its in-core symbols out of inferior memory.  SIZE, if
    non-zero, is the known size of the object.  TEMPL is a bfd
    representing the target's format.  NAME is the name to use for this
-   symbol file in messages; it can be NULL or a malloc-allocated string
-   which will be attached to the BFD.  */
+   symbol file in messages; it can be NULL or a malloc-allocated string.  */
 static struct objfile *
 symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
 			     size_t size, char *name, int from_tty)
@@ -104,6 +103,7 @@  symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
   if (name == NULL)
     name = xstrdup ("shared object read from target memory");
   bfd_set_filename (nbfd, name);
+  free (name);
 
   if (!bfd_check_format (nbfd, bfd_object))
     error (_("Got object file from memory but can't read symbols: %s."),
diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index fe65d2b266..8c5ee76233 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -1523,7 +1523,6 @@  gld_${EMULATION_NAME}_after_open (void)
 			struct bfd_symbol *s;
 			struct bfd_link_hash_entry * blhe;
 			const char *other_bfd_filename;
-			char *n;
 
 			s = (relocs[i]->sym_ptr_ptr)[0];
 
@@ -1550,9 +1549,9 @@  gld_${EMULATION_NAME}_after_open (void)
 			  continue;
 
 			/* Rename this implib to match the other one.  */
-			n = xmalloc (strlen (other_bfd_filename) + 1);
-			strcpy (n, other_bfd_filename);
-			bfd_set_filename (is->the_bfd->my_archive, n);
+			if (!bfd_set_filename (is->the_bfd->my_archive,
+					       other_bfd_filename))
+			  einfo ("%F%P: %pB: %E\n", is->the_bfd);
 		      }
 
 		    free (relocs);
@@ -1655,28 +1654,14 @@  gld_${EMULATION_NAME}_after_open (void)
 		else /* sentinel */
 		  seq = 'c';
 
-		/* PR 25993: It is possible that is->the_bfd-filename == is->filename.
-		   In which case calling bfd_set_filename on one will free the memory
-		   pointed to by the other.  */
-		if (is->filename == bfd_get_filename (is->the_bfd))
-		  {
-		    new_name = xmalloc (strlen (is->filename) + 3);
-		    sprintf (new_name, "%s.%c", is->filename, seq);
-		    bfd_set_filename (is->the_bfd, new_name);
-		    is->filename = new_name;
-		  }
-		else
-		  {
-		    new_name
-		      = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
-		    sprintf (new_name, "%s.%c",
-			     bfd_get_filename (is->the_bfd), seq);
-		    bfd_set_filename (is->the_bfd, new_name);
-
-		    new_name = xmalloc (strlen (is->filename) + 3);
-		    sprintf (new_name, "%s.%c", is->filename, seq);
-		    is->filename = new_name;
-		  }
+		new_name
+		  = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
+		sprintf (new_name, "%s.%c",
+			 bfd_get_filename (is->the_bfd), seq);
+		is->filename = bfd_set_filename (is->the_bfd, new_name);
+		free (new_name);
+		if (!is->filename)
+		  einfo ("%F%P: %pB: %E\n", is->the_bfd);
 	      }
 	  }
       }
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index 699b86501c..ea8e768ea9 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -1491,7 +1491,6 @@  gld_${EMULATION_NAME}_after_open (void)
 			struct bfd_symbol *s;
 			struct bfd_link_hash_entry * blhe;
 			const char *other_bfd_filename;
-			char *n;
 
 			s = (relocs[i]->sym_ptr_ptr)[0];
 
@@ -1518,9 +1517,9 @@  gld_${EMULATION_NAME}_after_open (void)
 			  continue;
 
 			/* Rename this implib to match the other one.  */
-			n = xmalloc (strlen (other_bfd_filename) + 1);
-			strcpy (n, other_bfd_filename);
-			bfd_set_filename (is->the_bfd->my_archive, n);
+			if (!bfd_set_filename (is->the_bfd->my_archive,
+					       other_bfd_filename))
+			  einfo ("%F%P: %pB: %E\n", is->the_bfd);
 		      }
 
 		    free (relocs);
@@ -1623,28 +1622,14 @@  gld_${EMULATION_NAME}_after_open (void)
 		else /* sentinel */
 		  seq = 'c';
 
-		/* PR 25993: It is possible that is->the_bfd-filename == is->filename.
-		   In which case calling bfd_set_filename on one will free the memory
-		   pointed to by the other.  */
-		if (is->filename == bfd_get_filename (is->the_bfd))
-		  {
-		    new_name = xmalloc (strlen (is->filename) + 3);
-		    sprintf (new_name, "%s.%c", is->filename, seq);
-		    bfd_set_filename (is->the_bfd, new_name);
-		    is->filename = new_name;
-		  }
-		else
-		  {
-		    new_name
-		      = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
-		    sprintf (new_name, "%s.%c",
-			     bfd_get_filename (is->the_bfd), seq);
-		    bfd_set_filename (is->the_bfd, new_name);
-
-		    new_name = xmalloc (strlen (is->filename) + 3);
-		    sprintf (new_name, "%s.%c", is->filename, seq);
-		    is->filename = new_name;
-		  }
+		new_name
+		  = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
+		sprintf (new_name, "%s.%c",
+			 bfd_get_filename (is->the_bfd), seq);
+		is->filename = bfd_set_filename (is->the_bfd, new_name);
+		free (new_name);
+		if (!is->filename)
+		  einfo ("%F%P: %pB: %E\n", is->the_bfd);
 	      }
 	  }
       }