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

Alan Modra amodra@gmail.com
Mon Feb 1 08:16:14 GMT 2021


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


More information about the Binutils mailing list