Reopen output file when writing archive output [BZ #27270][BZ #27284]

Message ID 20210201053230.215708-1-siddhesh@gotplt.org
State New
Headers show
Series
  • Reopen output file when writing archive output [BZ #27270][BZ #27284]
Related show

Commit Message

Siddhesh Poyarekar Feb. 1, 2021, 5:32 a.m.
When saving an archive, the archive stream could have got closed due
to there being too many open files in the BFD cache, so the stream
being NULL does not indicate that the file does not exist.  Instead,
attempt to reopen the stream.

binutils/

	[BZ #27270]
	[BZ #27284]
	* ar.c (write_archive): Remove unnecessary NULL check for
	IARCH and try to reopen the archive stream.
	* arsup.c: Include libbfd.h.
	(ar_save): Attempt to reopen the archive stream.
---
 binutils/ar.c    | 2 +-
 binutils/arsup.c | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.29.2

Comments

H.J. Lu via Binutils Feb. 1, 2021, 8:16 a.m. | #1
On Mon, Feb 01, 2021 at 11:02:30AM +0530, Siddhesh Poyarekar wrote:
> --- a/binutils/ar.c

> +++ b/binutils/ar.c

> @@ -1303,7 +1303,7 @@ write_archive (bfd *iarch)

>  

>  #if !defined (_WIN32) || defined (__CYGWIN32__)

>    ofd = dup (ofd);

> -  if (iarch == NULL || iarch->iostream == NULL)

> +  if (iarch->iostream == NULL && bfd_open_file (iarch) == NULL)

>      skip_stat = TRUE;

>    else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)

>      bfd_fatal (old_name);


This isn't the right way to go about reopening a file, and you shouldn't
be including libbfd.h anywhere except in bfd/.  See the notice:
   (This include file is not for users of the library.)

In fact, the more I look at this code the less I like it.  I think
you've also broken _WIN32 builds as you won't be calling fstat but
will pass an uninitialised target_stat to smart_rename.  So in
partially fixing a potential security bug we've broken binutils in
multiple ways.  (It's only a partial fix because the ar -M mri mode
script support in arsup.c wasn't changed to use the new bfd_fdopenw.)

Here's what I just threw together, as yet untested.  Please review for
any security faults still present.  bfd_stat will do fstat on the file
descriptor in this usage.

diff --git a/binutils/ar.c b/binutils/ar.c
index 24ff0920f40..0ecfa337228 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -25,7 +25,6 @@
 
 #include "sysdep.h"
 #include "bfd.h"
-#include "libbfd.h"
 #include "libiberty.h"
 #include "progress.h"
 #include "getopt.h"
@@ -1255,10 +1254,8 @@ write_archive (bfd *iarch)
   bfd *contents_head = iarch->archive_next;
   int ofd = -1;
   struct stat target_stat;
-  bfd_boolean skip_stat = FALSE;
 
-  old_name = (char *) xmalloc (strlen (bfd_get_filename (iarch)) + 1);
-  strcpy (old_name, bfd_get_filename (iarch));
+  old_name = xstrdup (bfd_get_filename (iarch));
   new_name = make_tempname (old_name, &ofd);
 
   if (new_name == NULL)
@@ -1303,11 +1300,9 @@ write_archive (bfd *iarch)
 
 #if !defined (_WIN32) || defined (__CYGWIN32__)
   ofd = dup (ofd);
-  if (iarch == NULL || iarch->iostream == NULL)
-    skip_stat = TRUE;
-  else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)
-    bfd_fatal (old_name);
 #endif
+  if (ofd == -1 || bfd_stat (iarch, &target_stat) != 0)
+    bfd_fatal (old_name);
 
   if (!bfd_close (obfd))
     bfd_fatal (old_name);
@@ -1318,7 +1313,7 @@ write_archive (bfd *iarch)
   /* We don't care if this fails; we might be creating the archive.  */
   bfd_close (iarch);
 
-  if (smart_rename (new_name, old_name, ofd, skip_stat ? NULL : &target_stat, 0) != 0)
+  if (smart_rename (new_name, old_name, ofd, &target_stat, 0) != 0)
     xexit (1);
   free (old_name);
   free (new_name);
diff --git a/binutils/arsup.c b/binutils/arsup.c
index 837011bdfd2..d4c98995e28 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -42,6 +42,8 @@ extern int deterministic;
 
 static bfd *obfd;
 static char *real_name;
+static char *temp_name;
+static int real_ofd;
 static FILE *outfile;
 
 static void
@@ -149,27 +151,24 @@ maybequit (void)
 void
 ar_open (char *name, int t)
 {
-  char *tname;
-  const char *bname = lbasename (name);
-  real_name = name;
+  real_name = xstrdup (name);
+  temp_name = make_tempname (real_name, &real_ofd);
 
-  /* Prepend tmp- to the beginning, to avoid file-name clashes after
-     truncation on filesystems with limited namespaces (DOS).  */
-  if (asprintf (&tname, "%.*stmp-%s", (int) (bname - name), name, bname) == -1)
+  if (temp_name == NULL)
     {
-      fprintf (stderr, _("%s: Can't allocate memory for temp name (%s)\n"),
+      fprintf (stderr, _("%s: Can't open temporary file (%s)\n"),
 	       program_name, strerror(errno));
       maybequit ();
       return;
     }
 
-  obfd = bfd_openw (tname, NULL);
+  obfd = bfd_fdopenw (temp_name, NULL, real_ofd);
 
   if (!obfd)
     {
       fprintf (stderr,
 	       _("%s: Can't open output archive %s\n"),
-	       program_name,  tname);
+	       program_name, temp_name);
 
       maybequit ();
     }
@@ -344,10 +343,9 @@ ar_save (void)
     }
   else
     {
-      char *ofilename = xstrdup (bfd_get_filename (obfd));
       bfd_boolean skip_stat = FALSE;
       struct stat target_stat;
-      int ofd = -1;
+      int ofd = real_ofd;
 
       if (deterministic > 0)
         obfd->flags |= BFD_DETERMINISTIC_OUTPUT;
@@ -355,17 +353,18 @@ ar_save (void)
 #if !defined (_WIN32) || defined (__CYGWIN32__)
       /* It's OK to fail; at worst it will result in SMART_RENAME using a slow
          copy fallback to write the output.  */
-      ofd = dup (fileno ((FILE *) obfd->iostream));
-      if (lstat (real_name, &target_stat) != 0)
-	skip_stat = TRUE;
+      ofd = dup (ofd);
 #endif
-
       bfd_close (obfd);
 
-      smart_rename (ofilename, real_name, ofd,
+      if (ofd == -1 || fstat (ofd, &target_stat) != 0)
+	skip_stat = TRUE;
+
+      smart_rename (temp_name, real_name, ofd,
 		    skip_stat ? NULL : &target_stat, 0);
       obfd = 0;
-      free (ofilename);
+      free (temp_name);
+      free (real_name);
     }
 }
 
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 39a1ccbd7fe..0e1047e7482 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -20,7 +20,6 @@
 
 #include "sysdep.h"
 #include "bfd.h"
-#include "libbfd.h"
 #include "progress.h"
 #include "getopt.h"
 #include "libiberty.h"
@@ -3768,7 +3767,7 @@ copy_file (const char *input_filename, const char *output_filename, int ofd,
   /* To allow us to do "strip *" without dying on the first
      non-object file, failures are nonfatal.  */
   ibfd = bfd_openr (input_filename, input_target);
-  if (ibfd == NULL || fstat (fileno ((FILE *) ibfd->iostream), in_stat) != 0)
+  if (ibfd == NULL || bfd_stat (ibfd, in_stat) != 0)
     {
       bfd_nonfatal_message (input_filename, NULL, NULL, NULL);
       status = 1;



-- 
Alan Modra
Australia Development Lab, IBM
Siddhesh Poyarekar Feb. 1, 2021, 9:25 a.m. | #2
On 2/1/21 1:46 PM, Alan Modra wrote:
> On Mon, Feb 01, 2021 at 11:02:30AM +0530, Siddhesh Poyarekar wrote:

>> --- a/binutils/ar.c

>> +++ b/binutils/ar.c

>> @@ -1303,7 +1303,7 @@ write_archive (bfd *iarch)

>>   

>>   #if !defined (_WIN32) || defined (__CYGWIN32__)

>>     ofd = dup (ofd);

>> -  if (iarch == NULL || iarch->iostream == NULL)

>> +  if (iarch->iostream == NULL && bfd_open_file (iarch) == NULL)

>>       skip_stat = TRUE;

>>     else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)

>>       bfd_fatal (old_name);

> 

> This isn't the right way to go about reopening a file, and you shouldn't

> be including libbfd.h anywhere except in bfd/.  See the notice:

>     (This include file is not for users of the library.)


Ahh sorry, I didn't see that.

> In fact, the more I look at this code the less I like it.  I think

> you've also broken _WIN32 builds as you won't be calling fstat but

> will pass an uninitialised target_stat to smart_rename.  So in


WIN32 doesn't use the contents of target_stat, only the pointer to 
indicate whether the file exists, since that's all it needs.  Anyway, I 
agree it's future-proof to keep it initialized for WIN32 as well in the 
event that it uses the value in future.

> partially fixing a potential security bug we've broken binutils in

> multiple ways.  (It's only a partial fix because the ar -M mri mode

> script support in arsup.c wasn't changed to use the new bfd_fdopenw.)


That doesn't really affect CVE-2021-20197 (which is specifically about 
being able to trick smart_rename into setting permissions of arbitrary 
files via symlinks), but I agree it's a good cleanup.

> Here's what I just threw together, as yet untested.  Please review for

> any security faults still present.  bfd_stat will do fstat on the file

> descriptor in this usage.


I've tested this patch and can verify that it fixes the breakage.  It 
looks good to me, thanks for your help!

Siddhesh


> diff --git a/binutils/ar.c b/binutils/ar.c

> index 24ff0920f40..0ecfa337228 100644

> --- a/binutils/ar.c

> +++ b/binutils/ar.c

> @@ -25,7 +25,6 @@

>   

>   #include "sysdep.h"

>   #include "bfd.h"

> -#include "libbfd.h"

>   #include "libiberty.h"

>   #include "progress.h"

>   #include "getopt.h"

> @@ -1255,10 +1254,8 @@ write_archive (bfd *iarch)

>     bfd *contents_head = iarch->archive_next;

>     int ofd = -1;

>     struct stat target_stat;

> -  bfd_boolean skip_stat = FALSE;

>   

> -  old_name = (char *) xmalloc (strlen (bfd_get_filename (iarch)) + 1);

> -  strcpy (old_name, bfd_get_filename (iarch));

> +  old_name = xstrdup (bfd_get_filename (iarch));

>     new_name = make_tempname (old_name, &ofd);

>   

>     if (new_name == NULL)

> @@ -1303,11 +1300,9 @@ write_archive (bfd *iarch)

>   

>   #if !defined (_WIN32) || defined (__CYGWIN32__)

>     ofd = dup (ofd);

> -  if (iarch == NULL || iarch->iostream == NULL)

> -    skip_stat = TRUE;

> -  else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)

> -    bfd_fatal (old_name);

>   #endif

> +  if (ofd == -1 || bfd_stat (iarch, &target_stat) != 0)

> +    bfd_fatal (old_name);

>   

>     if (!bfd_close (obfd))

>       bfd_fatal (old_name);

> @@ -1318,7 +1313,7 @@ write_archive (bfd *iarch)

>     /* We don't care if this fails; we might be creating the archive.  */

>     bfd_close (iarch);

>   

> -  if (smart_rename (new_name, old_name, ofd, skip_stat ? NULL : &target_stat, 0) != 0)

> +  if (smart_rename (new_name, old_name, ofd, &target_stat, 0) != 0)

>       xexit (1);

>     free (old_name);

>     free (new_name);

> diff --git a/binutils/arsup.c b/binutils/arsup.c

> index 837011bdfd2..d4c98995e28 100644

> --- a/binutils/arsup.c

> +++ b/binutils/arsup.c

> @@ -42,6 +42,8 @@ extern int deterministic;

>   

>   static bfd *obfd;

>   static char *real_name;

> +static char *temp_name;

> +static int real_ofd;

>   static FILE *outfile;

>   

>   static void

> @@ -149,27 +151,24 @@ maybequit (void)

>   void

>   ar_open (char *name, int t)

>   {

> -  char *tname;

> -  const char *bname = lbasename (name);

> -  real_name = name;

> +  real_name = xstrdup (name);

> +  temp_name = make_tempname (real_name, &real_ofd);

>   

> -  /* Prepend tmp- to the beginning, to avoid file-name clashes after

> -     truncation on filesystems with limited namespaces (DOS).  */

> -  if (asprintf (&tname, "%.*stmp-%s", (int) (bname - name), name, bname) == -1)

> +  if (temp_name == NULL)

>       {

> -      fprintf (stderr, _("%s: Can't allocate memory for temp name (%s)\n"),

> +      fprintf (stderr, _("%s: Can't open temporary file (%s)\n"),

>   	       program_name, strerror(errno));

>         maybequit ();

>         return;

>       }

>   

> -  obfd = bfd_openw (tname, NULL);

> +  obfd = bfd_fdopenw (temp_name, NULL, real_ofd);

>   

>     if (!obfd)

>       {

>         fprintf (stderr,

>   	       _("%s: Can't open output archive %s\n"),

> -	       program_name,  tname);

> +	       program_name, temp_name);

>   

>         maybequit ();

>       }

> @@ -344,10 +343,9 @@ ar_save (void)

>       }

>     else

>       {

> -      char *ofilename = xstrdup (bfd_get_filename (obfd));

>         bfd_boolean skip_stat = FALSE;

>         struct stat target_stat;

> -      int ofd = -1;

> +      int ofd = real_ofd;

>   

>         if (deterministic > 0)

>           obfd->flags |= BFD_DETERMINISTIC_OUTPUT;

> @@ -355,17 +353,18 @@ ar_save (void)

>   #if !defined (_WIN32) || defined (__CYGWIN32__)

>         /* It's OK to fail; at worst it will result in SMART_RENAME using a slow

>            copy fallback to write the output.  */

> -      ofd = dup (fileno ((FILE *) obfd->iostream));

> -      if (lstat (real_name, &target_stat) != 0)

> -	skip_stat = TRUE;

> +      ofd = dup (ofd);

>   #endif

> -

>         bfd_close (obfd);

>   

> -      smart_rename (ofilename, real_name, ofd,

> +      if (ofd == -1 || fstat (ofd, &target_stat) != 0)

> +	skip_stat = TRUE;

> +

> +      smart_rename (temp_name, real_name, ofd,

>   		    skip_stat ? NULL : &target_stat, 0);

>         obfd = 0;

> -      free (ofilename);

> +      free (temp_name);

> +      free (real_name);

>       }

>   }

>   

> diff --git a/binutils/objcopy.c b/binutils/objcopy.c

> index 39a1ccbd7fe..0e1047e7482 100644

> --- a/binutils/objcopy.c

> +++ b/binutils/objcopy.c

> @@ -20,7 +20,6 @@

>   

>   #include "sysdep.h"

>   #include "bfd.h"

> -#include "libbfd.h"

>   #include "progress.h"

>   #include "getopt.h"

>   #include "libiberty.h"

> @@ -3768,7 +3767,7 @@ copy_file (const char *input_filename, const char *output_filename, int ofd,

>     /* To allow us to do "strip *" without dying on the first

>        non-object file, failures are nonfatal.  */

>     ibfd = bfd_openr (input_filename, input_target);

> -  if (ibfd == NULL || fstat (fileno ((FILE *) ibfd->iostream), in_stat) != 0)

> +  if (ibfd == NULL || bfd_stat (ibfd, in_stat) != 0)

>       {

>         bfd_nonfatal_message (input_filename, NULL, NULL, NULL);

>         status = 1;

> 

> 

>
H.J. Lu via Binutils Feb. 3, 2021, 2:57 a.m. | #3
On Mon, Feb 01, 2021 at 02:55:01PM +0530, Siddhesh Poyarekar wrote:
> > partially fixing a potential security bug we've broken binutils in

> > multiple ways.  (It's only a partial fix because the ar -M mri mode

> > script support in arsup.c wasn't changed to use the new bfd_fdopenw.)

> 

> That doesn't really affect CVE-2021-20197 (which is specifically about being

> able to trick smart_rename into setting permissions of arbitrary files via

> symlinks), but I agree it's a good cleanup.


OK..

> > Here's what I just threw together, as yet untested.  Please review for

> > any security faults still present.  bfd_stat will do fstat on the file

> > descriptor in this usage.


This is what I'm about to commit.  The arsup.c changes for ar -M
scripts in the previous patch I posted resulted in new archives being
created with mode 0600.

	PR 27270
	PR 27284
	PR 26945
	* ar.c: Don't include libbfd.h.
	(write_archive): Replace xmalloc+strcpy with xstrdup.  Use
	bfd_stat rather than fstat on iostream.  Move stat and fd tests
	outside of _WIN32 ifdef.  Delete skip_stat variable.
	* arsup.c (temp_name, real_ofd): New static variables.
	(ar_open): Use make_tempname and bfd_fdopenw.
	(ar_save): Adjust to suit ar_open changes.  Move stat output
	of _WIN32 ifdef.
	* objcopy.c: Don't include libbfd.h.
	(copy_file): Use bfd_stat.

diff --git a/binutils/ar.c b/binutils/ar.c
index 24ff0920f40..0ecfa337228 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -25,7 +25,6 @@
 
 #include "sysdep.h"
 #include "bfd.h"
-#include "libbfd.h"
 #include "libiberty.h"
 #include "progress.h"
 #include "getopt.h"
@@ -1255,10 +1254,8 @@ write_archive (bfd *iarch)
   bfd *contents_head = iarch->archive_next;
   int ofd = -1;
   struct stat target_stat;
-  bfd_boolean skip_stat = FALSE;
 
-  old_name = (char *) xmalloc (strlen (bfd_get_filename (iarch)) + 1);
-  strcpy (old_name, bfd_get_filename (iarch));
+  old_name = xstrdup (bfd_get_filename (iarch));
   new_name = make_tempname (old_name, &ofd);
 
   if (new_name == NULL)
@@ -1303,11 +1300,9 @@ write_archive (bfd *iarch)
 
 #if !defined (_WIN32) || defined (__CYGWIN32__)
   ofd = dup (ofd);
-  if (iarch == NULL || iarch->iostream == NULL)
-    skip_stat = TRUE;
-  else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)
-    bfd_fatal (old_name);
 #endif
+  if (ofd == -1 || bfd_stat (iarch, &target_stat) != 0)
+    bfd_fatal (old_name);
 
   if (!bfd_close (obfd))
     bfd_fatal (old_name);
@@ -1318,7 +1313,7 @@ write_archive (bfd *iarch)
   /* We don't care if this fails; we might be creating the archive.  */
   bfd_close (iarch);
 
-  if (smart_rename (new_name, old_name, ofd, skip_stat ? NULL : &target_stat, 0) != 0)
+  if (smart_rename (new_name, old_name, ofd, &target_stat, 0) != 0)
     xexit (1);
   free (old_name);
   free (new_name);
diff --git a/binutils/arsup.c b/binutils/arsup.c
index 837011bdfd2..a60629f6761 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -42,6 +42,8 @@ extern int deterministic;
 
 static bfd *obfd;
 static char *real_name;
+static char *temp_name;
+static int real_ofd;
 static FILE *outfile;
 
 static void
@@ -149,27 +151,24 @@ maybequit (void)
 void
 ar_open (char *name, int t)
 {
-  char *tname;
-  const char *bname = lbasename (name);
-  real_name = name;
+  real_name = xstrdup (name);
+  temp_name = make_tempname (real_name, &real_ofd);
 
-  /* Prepend tmp- to the beginning, to avoid file-name clashes after
-     truncation on filesystems with limited namespaces (DOS).  */
-  if (asprintf (&tname, "%.*stmp-%s", (int) (bname - name), name, bname) == -1)
+  if (temp_name == NULL)
     {
-      fprintf (stderr, _("%s: Can't allocate memory for temp name (%s)\n"),
+      fprintf (stderr, _("%s: Can't open temporary file (%s)\n"),
 	       program_name, strerror(errno));
       maybequit ();
       return;
     }
 
-  obfd = bfd_openw (tname, NULL);
+  obfd = bfd_fdopenw (temp_name, NULL, real_ofd);
 
   if (!obfd)
     {
       fprintf (stderr,
 	       _("%s: Can't open output archive %s\n"),
-	       program_name,  tname);
+	       program_name, temp_name);
 
       maybequit ();
     }
@@ -344,10 +343,9 @@ ar_save (void)
     }
   else
     {
-      char *ofilename = xstrdup (bfd_get_filename (obfd));
       bfd_boolean skip_stat = FALSE;
       struct stat target_stat;
-      int ofd = -1;
+      int ofd = real_ofd;
 
       if (deterministic > 0)
         obfd->flags |= BFD_DETERMINISTIC_OUTPUT;
@@ -355,17 +353,31 @@ ar_save (void)
 #if !defined (_WIN32) || defined (__CYGWIN32__)
       /* It's OK to fail; at worst it will result in SMART_RENAME using a slow
          copy fallback to write the output.  */
-      ofd = dup (fileno ((FILE *) obfd->iostream));
-      if (lstat (real_name, &target_stat) != 0)
-	skip_stat = TRUE;
+      ofd = dup (ofd);
 #endif
-
       bfd_close (obfd);
 
-      smart_rename (ofilename, real_name, ofd,
+      if (lstat (real_name, &target_stat) != 0)
+	{
+	  /* The temp file created in ar_open has mode 0600 as per mkstemp.
+	     Create the real empty output file here so smart_rename will
+	     update the mode according to the process umask.  */
+	  obfd = bfd_openw (real_name, NULL);
+	  if (obfd == NULL
+	      || bfd_stat (obfd, &target_stat) != 0)
+	    skip_stat = TRUE;
+	  if (obfd != NULL)
+	    {
+	      bfd_set_format (obfd, bfd_archive);
+	      bfd_close (obfd);
+	    }
+	}
+
+      smart_rename (temp_name, real_name, ofd,
 		    skip_stat ? NULL : &target_stat, 0);
       obfd = 0;
-      free (ofilename);
+      free (temp_name);
+      free (real_name);
     }
 }
 
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 39a1ccbd7fe..0e1047e7482 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -20,7 +20,6 @@
 
 #include "sysdep.h"
 #include "bfd.h"
-#include "libbfd.h"
 #include "progress.h"
 #include "getopt.h"
 #include "libiberty.h"
@@ -3768,7 +3767,7 @@ copy_file (const char *input_filename, const char *output_filename, int ofd,
   /* To allow us to do "strip *" without dying on the first
      non-object file, failures are nonfatal.  */
   ibfd = bfd_openr (input_filename, input_target);
-  if (ibfd == NULL || fstat (fileno ((FILE *) ibfd->iostream), in_stat) != 0)
+  if (ibfd == NULL || bfd_stat (ibfd, in_stat) != 0)
     {
       bfd_nonfatal_message (input_filename, NULL, NULL, NULL);
       status = 1;

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/binutils/ar.c b/binutils/ar.c
index 24ff0920f40..9644942ac24 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -1303,7 +1303,7 @@  write_archive (bfd *iarch)
 
 #if !defined (_WIN32) || defined (__CYGWIN32__)
   ofd = dup (ofd);
-  if (iarch == NULL || iarch->iostream == NULL)
+  if (iarch->iostream == NULL && bfd_open_file (iarch) == NULL)
     skip_stat = TRUE;
   else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)
     bfd_fatal (old_name);
diff --git a/binutils/arsup.c b/binutils/arsup.c
index 837011bdfd2..b305c753f73 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -27,6 +27,7 @@ 
 
 #include "sysdep.h"
 #include "bfd.h"
+#include "libbfd.h"
 #include "libiberty.h"
 #include "filenames.h"
 #include "bucomm.h"
@@ -353,9 +354,11 @@  ar_save (void)
         obfd->flags |= BFD_DETERMINISTIC_OUTPUT;
 
 #if !defined (_WIN32) || defined (__CYGWIN32__)
-      /* It's OK to fail; at worst it will result in SMART_RENAME using a slow
-         copy fallback to write the output.  */
-      ofd = dup (fileno ((FILE *) obfd->iostream));
+      /* Try to reopen the output BFD stream if it has closed.  It's OK for DUP
+	 to fail; at worst it will result in SMART_RENAME using a slow copy
+	 fallback to write the output.  */
+      if (obfd->iostream != NULL || bfd_open_file (obfd) != NULL)
+	ofd = dup (fileno ((FILE *) obfd->iostream));
       if (lstat (real_name, &target_stat) != 0)
 	skip_stat = TRUE;
 #endif