[PATCH] binutils: Avoid renaming over existing files

[PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

Renaming over existing files needs additional care to restore
permissions and ownership, which may not always succeed.
Additionally, other properties of the file such as extended attributes
may be lost, making the operation flaky.

For predictable results, resort to rename() only if the file does not
exist, otherwise copy the file contents into the existing file.  This
ensures that no additional tricks are needed to retain file
properties.

This also allows dropping of the redundant set_times on the tmpfile in
objcopy/strip since now we no longer rename over existing files.

binutils/

	* ar.c (write_archive): Remove TARGET_STAT.  Adjust call to
	SMART_RENAME.
	* arsup.c (ar_save): Likewise.
	* objcopy (strip_main): Don't copy TMPFD.  Don't set times on
	temporary file and adjust call to SMART_RENAME.
	(copy_main): Likewise.
	* rename.c [!S_ISLNK]: Remove definitions.
	(try_preserve_permissions): Remove function.
	(smart_rename): Remove FD, PRESERVE_DATES arguments.  Use
	rename system call only if TO does not exist.
	* bucomm.h (smart_rename): Adjust declaration.
---
 binutils/ar.c      |  9 +----
 binutils/arsup.c   | 13 +------
 binutils/bucomm.h  |  2 +-
 binutils/objcopy.c | 42 ++++----------------
 binutils/rename.c  | 95 +++++-----------------------------------------
 5 files changed, 19 insertions(+), 142 deletions(-)

diff --git a/binutils/ar.c b/binutils/ar.c
index 0ecfa337228..44df48c5c67 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -1253,7 +1253,6 @@ write_archive (bfd *iarch)
   char *old_name, *new_name;
   bfd *contents_head = iarch->archive_next;
   int ofd = -1;
-  struct stat target_stat;
 
   old_name = xstrdup (bfd_get_filename (iarch));
   new_name = make_tempname (old_name, &ofd);
@@ -1298,12 +1297,6 @@ write_archive (bfd *iarch)
   if (!bfd_set_archive_head (obfd, contents_head))
     bfd_fatal (old_name);
 
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-  ofd = dup (ofd);
-#endif
-  if (ofd == -1 || bfd_stat (iarch, &target_stat) != 0)
-    bfd_fatal (old_name);
-
   if (!bfd_close (obfd))
     bfd_fatal (old_name);
 
@@ -1313,7 +1306,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, &target_stat, 0) != 0)
+  if (smart_rename (new_name, old_name, NULL) != 0)
     xexit (1);
   free (old_name);
   free (new_name);
diff --git a/binutils/arsup.c b/binutils/arsup.c
index fa7706f79e5..f7ce8f0bc82 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -343,18 +343,11 @@ ar_save (void)
     }
   else
     {
-      bfd_boolean skip_stat = FALSE;
       struct stat target_stat;
-      int ofd = real_ofd;
 
       if (deterministic > 0)
         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 (ofd);
-#endif
       bfd_close (obfd);
 
       if (stat (real_name, &target_stat) != 0)
@@ -363,9 +356,6 @@ ar_save (void)
 	     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);
@@ -373,8 +363,7 @@ ar_save (void)
 	    }
 	}
 
-      smart_rename (temp_name, real_name, ofd,
-		    skip_stat ? NULL : &target_stat, 0);
+      smart_rename (temp_name, real_name, NULL);
       obfd = 0;
       free (temp_name);
       free (real_name);
diff --git a/binutils/bucomm.h b/binutils/bucomm.h
index 7a0adfae565..aa7e33d8cd1 100644
--- a/binutils/bucomm.h
+++ b/binutils/bucomm.h
@@ -71,7 +71,7 @@ extern void print_version (const char *);
 /* In rename.c.  */
 extern void set_times (const char *, const struct stat *);
 
-extern int smart_rename (const char *, const char *, int, struct stat *, int);
+extern int smart_rename (const char *, const char *, struct stat *);
 
 
 /* In libiberty.  */
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 0e1047e7482..378ee1535f3 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4832,7 +4832,6 @@ strip_main (int argc, char *argv[])
       struct stat statbuf;
       char *tmpname;
       int tmpfd = -1;
-      int copyfd = -1;
 
       if (get_file_size (argv[i]) < 1)
 	{
@@ -4846,12 +4845,7 @@ strip_main (int argc, char *argv[])
       else
 	tmpname = output_file;
 
-      if (tmpname == NULL
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-	  /* Retain a copy of TMPFD since we will need it for SMART_RENAME.  */
-	  || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
-#endif
-      )
+      if (tmpname == NULL)
 	{
 	  bfd_nonfatal_message (argv[i], NULL, NULL,
 				_("could not create temporary file to hold stripped copy"));
@@ -4864,23 +4858,15 @@ strip_main (int argc, char *argv[])
 		 output_target, NULL);
       if (status == 0)
 	{
-	  if (preserve_dates)
-	    set_times (tmpname, &statbuf);
 	  if (output_file != tmpname)
 	    status = (smart_rename (tmpname,
 				    output_file ? output_file : argv[i],
-				    copyfd, &statbuf, preserve_dates) != 0);
+				    preserve_dates ? &statbuf : NULL) != 0);
 	  if (status == 0)
 	    status = hold_status;
 	}
       else
-	{
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-	  if (copyfd >= 0)
-	    close (copyfd);
-#endif
-	  unlink_if_ordinary (tmpname);
-	}
+	unlink_if_ordinary (tmpname);
       if (output_file != tmpname)
 	free (tmpname);
     }
@@ -5088,7 +5074,6 @@ copy_main (int argc, char *argv[])
   bfd_boolean use_globalize = FALSE;
   bfd_boolean use_keep_global = FALSE;
   int c, tmpfd = -1;
-  int copyfd = -1;
   struct stat statbuf;
   const bfd_arch_info_type *input_arch = NULL;
 
@@ -5933,12 +5918,7 @@ copy_main (int argc, char *argv[])
   else
     tmpname = output_filename;
 
-  if (tmpname == NULL
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-      /* Retain a copy of TMPFD since we will need it for SMART_RENAME.  */
-      || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
-#endif
-  )
+  if (tmpname == NULL)
     {
       fatal (_("warning: could not create temporary file whilst copying '%s', (error: %s)"),
 	     input_filename, strerror (errno));
@@ -5948,20 +5928,12 @@ copy_main (int argc, char *argv[])
 	     output_target, input_arch);
   if (status == 0)
     {
-      if (preserve_dates)
-	set_times (tmpname, &statbuf);
       if (tmpname != output_filename)
-	status = (smart_rename (tmpname, input_filename, copyfd, &statbuf,
-				preserve_dates) != 0);
+	status = (smart_rename (tmpname, input_filename,
+				preserve_dates ? &statbuf : NULL) != 0);
     }
   else
-    {
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-      if (copyfd >= 0)
-	close (copyfd);
-#endif
-      unlink_if_ordinary (tmpname);
-    }
+    unlink_if_ordinary (tmpname);
 
   if (tmpname != output_filename)
     free (tmpname);
diff --git a/binutils/rename.c b/binutils/rename.c
index e36b75132de..2ff092ee22b 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -122,61 +122,13 @@ set_times (const char *destination, const struct stat *statbuf)
     non_fatal (_("%s: cannot set time: %s"), destination, strerror (errno));
 }
 
-#ifndef S_ISLNK
-#ifdef S_IFLNK
-#define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
-#else
-#define S_ISLNK(m) 0
-#define lstat stat
-#endif
-#endif
-
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-/* Try to preserve the permission bits and ownership of an existing file when
-   rename overwrites it.  FD is the file being renamed and TARGET_STAT has the
-   status of the file that was overwritten.  */
-static void
-try_preserve_permissions (int fd, struct stat *target_stat)
-{
-  struct stat from_stat;
-  int ret = 0;
-
-  if (fstat (fd, &from_stat) != 0)
-    return;
-
-  int from_mode = from_stat.st_mode & 0777;
-  int to_mode = target_stat->st_mode & 0777;
-
-  /* Fix up permissions before we potentially lose ownership with fchown.
-     Clear the setxid bits because in case the fchown below fails then we don't
-     want to end up with a sxid file owned by the invoking user.  If the user
-     hasn't changed or if fchown succeeded, we add back the sxid bits at the
-     end.  */
-  if (from_mode != to_mode)
-    fchmod (fd, to_mode);
-
-  /* Fix up ownership, this will clear the setxid bits.  */
-  if (from_stat.st_uid != target_stat->st_uid
-      || from_stat.st_gid != target_stat->st_gid)
-    ret = fchown (fd, target_stat->st_uid, target_stat->st_gid);
-
-  /* Fix up the sxid bits if either the fchown wasn't needed or it
-     succeeded.  */
-  if (ret == 0)
-    fchmod (fd, target_stat->st_mode & 07777);
-}
-#endif
-
-/* Rename FROM to TO, copying if TO is either a link or is not a regular file.
-   FD is an open file descriptor pointing to FROM that we can use to safely fix
-   up permissions of the file after renaming.  TARGET_STAT has the file status
-   that is used to fix up permissions and timestamps after rename.  Return 0 if
-   ok, -1 if error and FD is closed before returning.  */
+/* Rename FROM to TO, copying if TO exists.  TARGET_STAT has the file status
+   that, if non-NULL, is used to fix up timestamps after rename.  Return 0 if
+   ok, -1 if error.  */
 
 int
-smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
-	      struct stat *target_stat ATTRIBUTE_UNUSED,
-	      int preserve_dates ATTRIBUTE_UNUSED)
+smart_rename (const char *from, const char *to,
+	      struct stat *target_stat ATTRIBUTE_UNUSED)
 {
   int ret = 0;
   struct stat to_stat;
@@ -199,37 +151,10 @@ smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
       unlink (from);
     }
 #else
-  /* Avoid a full copy and use rename if we can fix up permissions of the
-     file after renaming, i.e.:
-
-     - TO is not a symbolic link
-     - TO is a regular file with only one hard link
-     - We have permission to write to TO
-     - FD is available to safely fix up permissions to be the same as the file
-       we overwrote with the rename.
-
-     Note though that the actual file on disk that TARGET_STAT describes may
-     have changed and we're only trying to preserve the status we know about.
-     At no point do we try to interact with the new file changes, so there can
-     only be two outcomes, i.e. either the external file change survives
-     without knowledge of our change (if it happens after the rename syscall)
-     or our rename and permissions fixup survive without any knowledge of the
-     external change.  */
-  if (! exists
-      || (fd >= 0
-	  && !S_ISLNK (to_stat.st_mode)
-	  && S_ISREG (to_stat.st_mode)
-	  && (to_stat.st_mode & S_IWUSR)
-	  && to_stat.st_nlink == 1)
-      )
+  /* Avoid a full copy and use rename if TO does not exist.  */
+  if (!exists)
     {
-      ret = rename (from, to);
-      if (ret == 0)
-	{
-	  if (exists && target_stat != NULL)
-	    try_preserve_permissions (fd, target_stat);
-	}
-      else
+      if ((ret = rename (from, to)) != 0)
 	{
 	  /* We have to clean up here.  */
 	  non_fatal (_("unable to rename '%s'; reason: %s"), to, strerror (errno));
@@ -242,12 +167,10 @@ smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
       if (ret != 0)
 	non_fatal (_("unable to copy file '%s'; reason: %s"), to, strerror (errno));
 
-      if (preserve_dates && target_stat != NULL)
+      if (target_stat != NULL)
 	set_times (to, target_stat);
       unlink (from);
     }
-  if (fd >= 0)
-    close (fd);
 #endif /* _WIN32 && !__CYGWIN32__ */
 
   return ret;
-- 
2.29.2

Re: [PATCH] binutils: Avoid renaming over existing files

[Fangrui Song]

On 2021-02-09, Siddhesh Poyarekar wrote:
>Renaming over existing files needs additional care to restore
>permissions and ownership, which may not always succeed.
>Additionally, other properties of the file such as extended attributes
>may be lost, making the operation flaky.
>
>For predictable results, resort to rename() only if the file does not
>exist, otherwise copy the file contents into the existing file.  This
>ensures that no additional tricks are needed to retain file
>properties.
>
>This also allows dropping of the redundant set_times on the tmpfile in
>objcopy/strip since now we no longer rename over existing files.

Thanks for the patch! I am doing similar thing in llvm-objcopy as some
users want to preserve owner/group for an in-place operation like `strip exe`.

I observe that `strip a -o b` when b is present keeps the traditional
behavior (b's owner/group is the effective user/group. That is nice,
truncate+overwrite should probably only apply for in-place operations),
but they probably should be clarified.

[PING][PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

Ping!


On 2/9/21 12:47 AM, Siddhesh Poyarekar wrote:
> Renaming over existing files needs additional care to restore
> permissions and ownership, which may not always succeed.
> Additionally, other properties of the file such as extended attributes
> may be lost, making the operation flaky.
> 
> For predictable results, resort to rename() only if the file does not
> exist, otherwise copy the file contents into the existing file.  This
> ensures that no additional tricks are needed to retain file
> properties.
> 
> This also allows dropping of the redundant set_times on the tmpfile in
> objcopy/strip since now we no longer rename over existing files.
> 
> binutils/
> 
> 	* ar.c (write_archive): Remove TARGET_STAT.  Adjust call to
> 	SMART_RENAME.
> 	* arsup.c (ar_save): Likewise.
> 	* objcopy (strip_main): Don't copy TMPFD.  Don't set times on
> 	temporary file and adjust call to SMART_RENAME.
> 	(copy_main): Likewise.
> 	* rename.c [!S_ISLNK]: Remove definitions.
> 	(try_preserve_permissions): Remove function.
> 	(smart_rename): Remove FD, PRESERVE_DATES arguments.  Use
> 	rename system call only if TO does not exist.
> 	* bucomm.h (smart_rename): Adjust declaration.
> ---
>   binutils/ar.c      |  9 +----
>   binutils/arsup.c   | 13 +------
>   binutils/bucomm.h  |  2 +-
>   binutils/objcopy.c | 42 ++++----------------
>   binutils/rename.c  | 95 +++++-----------------------------------------
>   5 files changed, 19 insertions(+), 142 deletions(-)
> 
> diff --git a/binutils/ar.c b/binutils/ar.c
> index 0ecfa337228..44df48c5c67 100644
> --- a/binutils/ar.c
> +++ b/binutils/ar.c
> @@ -1253,7 +1253,6 @@ write_archive (bfd *iarch)
>     char *old_name, *new_name;
>     bfd *contents_head = iarch->archive_next;
>     int ofd = -1;
> -  struct stat target_stat;
>   
>     old_name = xstrdup (bfd_get_filename (iarch));
>     new_name = make_tempname (old_name, &ofd);
> @@ -1298,12 +1297,6 @@ write_archive (bfd *iarch)
>     if (!bfd_set_archive_head (obfd, contents_head))
>       bfd_fatal (old_name);
>   
> -#if !defined (_WIN32) || defined (__CYGWIN32__)
> -  ofd = dup (ofd);
> -#endif
> -  if (ofd == -1 || bfd_stat (iarch, &target_stat) != 0)
> -    bfd_fatal (old_name);
> -
>     if (!bfd_close (obfd))
>       bfd_fatal (old_name);
>   
> @@ -1313,7 +1306,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, &target_stat, 0) != 0)
> +  if (smart_rename (new_name, old_name, NULL) != 0)
>       xexit (1);
>     free (old_name);
>     free (new_name);
> diff --git a/binutils/arsup.c b/binutils/arsup.c
> index fa7706f79e5..f7ce8f0bc82 100644
> --- a/binutils/arsup.c
> +++ b/binutils/arsup.c
> @@ -343,18 +343,11 @@ ar_save (void)
>       }
>     else
>       {
> -      bfd_boolean skip_stat = FALSE;
>         struct stat target_stat;
> -      int ofd = real_ofd;
>   
>         if (deterministic > 0)
>           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 (ofd);
> -#endif
>         bfd_close (obfd);
>   
>         if (stat (real_name, &target_stat) != 0)
> @@ -363,9 +356,6 @@ ar_save (void)
>   	     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);
> @@ -373,8 +363,7 @@ ar_save (void)
>   	    }
>   	}
>   
> -      smart_rename (temp_name, real_name, ofd,
> -		    skip_stat ? NULL : &target_stat, 0);
> +      smart_rename (temp_name, real_name, NULL);
>         obfd = 0;
>         free (temp_name);
>         free (real_name);
> diff --git a/binutils/bucomm.h b/binutils/bucomm.h
> index 7a0adfae565..aa7e33d8cd1 100644
> --- a/binutils/bucomm.h
> +++ b/binutils/bucomm.h
> @@ -71,7 +71,7 @@ extern void print_version (const char *);
>   /* In rename.c.  */
>   extern void set_times (const char *, const struct stat *);
>   
> -extern int smart_rename (const char *, const char *, int, struct stat *, int);
> +extern int smart_rename (const char *, const char *, struct stat *);
>   
>   
>   /* In libiberty.  */
> diff --git a/binutils/objcopy.c b/binutils/objcopy.c
> index 0e1047e7482..378ee1535f3 100644
> --- a/binutils/objcopy.c
> +++ b/binutils/objcopy.c
> @@ -4832,7 +4832,6 @@ strip_main (int argc, char *argv[])
>         struct stat statbuf;
>         char *tmpname;
>         int tmpfd = -1;
> -      int copyfd = -1;
>   
>         if (get_file_size (argv[i]) < 1)
>   	{
> @@ -4846,12 +4845,7 @@ strip_main (int argc, char *argv[])
>         else
>   	tmpname = output_file;
>   
> -      if (tmpname == NULL
> -#if !defined (_WIN32) || defined (__CYGWIN32__)
> -	  /* Retain a copy of TMPFD since we will need it for SMART_RENAME.  */
> -	  || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
> -#endif
> -      )
> +      if (tmpname == NULL)
>   	{
>   	  bfd_nonfatal_message (argv[i], NULL, NULL,
>   				_("could not create temporary file to hold stripped copy"));
> @@ -4864,23 +4858,15 @@ strip_main (int argc, char *argv[])
>   		 output_target, NULL);
>         if (status == 0)
>   	{
> -	  if (preserve_dates)
> -	    set_times (tmpname, &statbuf);
>   	  if (output_file != tmpname)
>   	    status = (smart_rename (tmpname,
>   				    output_file ? output_file : argv[i],
> -				    copyfd, &statbuf, preserve_dates) != 0);
> +				    preserve_dates ? &statbuf : NULL) != 0);
>   	  if (status == 0)
>   	    status = hold_status;
>   	}
>         else
> -	{
> -#if !defined (_WIN32) || defined (__CYGWIN32__)
> -	  if (copyfd >= 0)
> -	    close (copyfd);
> -#endif
> -	  unlink_if_ordinary (tmpname);
> -	}
> +	unlink_if_ordinary (tmpname);
>         if (output_file != tmpname)
>   	free (tmpname);
>       }
> @@ -5088,7 +5074,6 @@ copy_main (int argc, char *argv[])
>     bfd_boolean use_globalize = FALSE;
>     bfd_boolean use_keep_global = FALSE;
>     int c, tmpfd = -1;
> -  int copyfd = -1;
>     struct stat statbuf;
>     const bfd_arch_info_type *input_arch = NULL;
>   
> @@ -5933,12 +5918,7 @@ copy_main (int argc, char *argv[])
>     else
>       tmpname = output_filename;
>   
> -  if (tmpname == NULL
> -#if !defined (_WIN32) || defined (__CYGWIN32__)
> -      /* Retain a copy of TMPFD since we will need it for SMART_RENAME.  */
> -      || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
> -#endif
> -  )
> +  if (tmpname == NULL)
>       {
>         fatal (_("warning: could not create temporary file whilst copying '%s', (error: %s)"),
>   	     input_filename, strerror (errno));
> @@ -5948,20 +5928,12 @@ copy_main (int argc, char *argv[])
>   	     output_target, input_arch);
>     if (status == 0)
>       {
> -      if (preserve_dates)
> -	set_times (tmpname, &statbuf);
>         if (tmpname != output_filename)
> -	status = (smart_rename (tmpname, input_filename, copyfd, &statbuf,
> -				preserve_dates) != 0);
> +	status = (smart_rename (tmpname, input_filename,
> +				preserve_dates ? &statbuf : NULL) != 0);
>       }
>     else
> -    {
> -#if !defined (_WIN32) || defined (__CYGWIN32__)
> -      if (copyfd >= 0)
> -	close (copyfd);
> -#endif
> -      unlink_if_ordinary (tmpname);
> -    }
> +    unlink_if_ordinary (tmpname);
>   
>     if (tmpname != output_filename)
>       free (tmpname);
> diff --git a/binutils/rename.c b/binutils/rename.c
> index e36b75132de..2ff092ee22b 100644
> --- a/binutils/rename.c
> +++ b/binutils/rename.c
> @@ -122,61 +122,13 @@ set_times (const char *destination, const struct stat *statbuf)
>       non_fatal (_("%s: cannot set time: %s"), destination, strerror (errno));
>   }
>   
> -#ifndef S_ISLNK
> -#ifdef S_IFLNK
> -#define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
> -#else
> -#define S_ISLNK(m) 0
> -#define lstat stat
> -#endif
> -#endif
> -
> -#if !defined (_WIN32) || defined (__CYGWIN32__)
> -/* Try to preserve the permission bits and ownership of an existing file when
> -   rename overwrites it.  FD is the file being renamed and TARGET_STAT has the
> -   status of the file that was overwritten.  */
> -static void
> -try_preserve_permissions (int fd, struct stat *target_stat)
> -{
> -  struct stat from_stat;
> -  int ret = 0;
> -
> -  if (fstat (fd, &from_stat) != 0)
> -    return;
> -
> -  int from_mode = from_stat.st_mode & 0777;
> -  int to_mode = target_stat->st_mode & 0777;
> -
> -  /* Fix up permissions before we potentially lose ownership with fchown.
> -     Clear the setxid bits because in case the fchown below fails then we don't
> -     want to end up with a sxid file owned by the invoking user.  If the user
> -     hasn't changed or if fchown succeeded, we add back the sxid bits at the
> -     end.  */
> -  if (from_mode != to_mode)
> -    fchmod (fd, to_mode);
> -
> -  /* Fix up ownership, this will clear the setxid bits.  */
> -  if (from_stat.st_uid != target_stat->st_uid
> -      || from_stat.st_gid != target_stat->st_gid)
> -    ret = fchown (fd, target_stat->st_uid, target_stat->st_gid);
> -
> -  /* Fix up the sxid bits if either the fchown wasn't needed or it
> -     succeeded.  */
> -  if (ret == 0)
> -    fchmod (fd, target_stat->st_mode & 07777);
> -}
> -#endif
> -
> -/* Rename FROM to TO, copying if TO is either a link or is not a regular file.
> -   FD is an open file descriptor pointing to FROM that we can use to safely fix
> -   up permissions of the file after renaming.  TARGET_STAT has the file status
> -   that is used to fix up permissions and timestamps after rename.  Return 0 if
> -   ok, -1 if error and FD is closed before returning.  */
> +/* Rename FROM to TO, copying if TO exists.  TARGET_STAT has the file status
> +   that, if non-NULL, is used to fix up timestamps after rename.  Return 0 if
> +   ok, -1 if error.  */
>   
>   int
> -smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
> -	      struct stat *target_stat ATTRIBUTE_UNUSED,
> -	      int preserve_dates ATTRIBUTE_UNUSED)
> +smart_rename (const char *from, const char *to,
> +	      struct stat *target_stat ATTRIBUTE_UNUSED)
>   {
>     int ret = 0;
>     struct stat to_stat;
> @@ -199,37 +151,10 @@ smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
>         unlink (from);
>       }
>   #else
> -  /* Avoid a full copy and use rename if we can fix up permissions of the
> -     file after renaming, i.e.:
> -
> -     - TO is not a symbolic link
> -     - TO is a regular file with only one hard link
> -     - We have permission to write to TO
> -     - FD is available to safely fix up permissions to be the same as the file
> -       we overwrote with the rename.
> -
> -     Note though that the actual file on disk that TARGET_STAT describes may
> -     have changed and we're only trying to preserve the status we know about.
> -     At no point do we try to interact with the new file changes, so there can
> -     only be two outcomes, i.e. either the external file change survives
> -     without knowledge of our change (if it happens after the rename syscall)
> -     or our rename and permissions fixup survive without any knowledge of the
> -     external change.  */
> -  if (! exists
> -      || (fd >= 0
> -	  && !S_ISLNK (to_stat.st_mode)
> -	  && S_ISREG (to_stat.st_mode)
> -	  && (to_stat.st_mode & S_IWUSR)
> -	  && to_stat.st_nlink == 1)
> -      )
> +  /* Avoid a full copy and use rename if TO does not exist.  */
> +  if (!exists)
>       {
> -      ret = rename (from, to);
> -      if (ret == 0)
> -	{
> -	  if (exists && target_stat != NULL)
> -	    try_preserve_permissions (fd, target_stat);
> -	}
> -      else
> +      if ((ret = rename (from, to)) != 0)
>   	{
>   	  /* We have to clean up here.  */
>   	  non_fatal (_("unable to rename '%s'; reason: %s"), to, strerror (errno));
> @@ -242,12 +167,10 @@ smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
>         if (ret != 0)
>   	non_fatal (_("unable to copy file '%s'; reason: %s"), to, strerror (errno));
>   
> -      if (preserve_dates && target_stat != NULL)
> +      if (target_stat != NULL)
>   	set_times (to, target_stat);
>         unlink (from);
>       }
> -  if (fd >= 0)
> -    close (fd);
>   #endif /* _WIN32 && !__CYGWIN32__ */
>   
>     return ret;
> 

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Michael Matz]

Hello,

On Mon, 15 Feb 2021, Siddhesh Poyarekar wrote:

> Ping!
> 
> On 2/9/21 12:47 AM, Siddhesh Poyarekar wrote:
> > Renaming over existing files needs additional care to restore
> > permissions and ownership, which may not always succeed.
> > Additionally, other properties of the file such as extended attributes
> > may be lost, making the operation flaky.
> > 
> > For predictable results, resort to rename() only if the file does not
> > exist, otherwise copy the file contents into the existing file.  This
> > ensures that no additional tricks are needed to retain file
> > properties.
> > 
> > This also allows dropping of the redundant set_times on the tmpfile in
> > objcopy/strip since now we no longer rename over existing files.

What happens when stripping executables that are currently executing?
You can't open them for writing (ETXTBSY), but you can delete/rename them.  
(There are other reasons why nominally writable files can't be opened for 
writing while delete/rename is possible)
Does the behaviour change with your patch?


Ciao,
Michael.

> > 
> > binutils/
> > 
> > 	* ar.c (write_archive): Remove TARGET_STAT.  Adjust call to
> > 	SMART_RENAME.
> > 	* arsup.c (ar_save): Likewise.
> > 	* objcopy (strip_main): Don't copy TMPFD.  Don't set times on
> > 	temporary file and adjust call to SMART_RENAME.
> > 	(copy_main): Likewise.
> > 	* rename.c [!S_ISLNK]: Remove definitions.
> > 	(try_preserve_permissions): Remove function.
> > 	(smart_rename): Remove FD, PRESERVE_DATES arguments.  Use
> > 	rename system call only if TO does not exist.
> > 	* bucomm.h (smart_rename): Adjust declaration.
> > ---
> >   binutils/ar.c      |  9 +----
> >   binutils/arsup.c   | 13 +------
> >   binutils/bucomm.h  |  2 +-
> >   binutils/objcopy.c | 42 ++++----------------
> >   binutils/rename.c  | 95 +++++-----------------------------------------
> >   5 files changed, 19 insertions(+), 142 deletions(-)
> > 
> > diff --git a/binutils/ar.c b/binutils/ar.c
> > index 0ecfa337228..44df48c5c67 100644
> > --- a/binutils/ar.c
> > +++ b/binutils/ar.c
> > @@ -1253,7 +1253,6 @@ write_archive (bfd *iarch)
> >     char *old_name, *new_name;
> >     bfd *contents_head = iarch->archive_next;
> >     int ofd = -1;
> > -  struct stat target_stat;
> >       old_name = xstrdup (bfd_get_filename (iarch));
> >     new_name = make_tempname (old_name, &ofd);
> > @@ -1298,12 +1297,6 @@ write_archive (bfd *iarch)
> >     if (!bfd_set_archive_head (obfd, contents_head))
> >       bfd_fatal (old_name);
> >   -#if !defined (_WIN32) || defined (__CYGWIN32__)
> > -  ofd = dup (ofd);
> > -#endif
> > -  if (ofd == -1 || bfd_stat (iarch, &target_stat) != 0)
> > -    bfd_fatal (old_name);
> > -
> >     if (!bfd_close (obfd))
> >       bfd_fatal (old_name);
> >   @@ -1313,7 +1306,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, &target_stat, 0) != 0)
> > +  if (smart_rename (new_name, old_name, NULL) != 0)
> >       xexit (1);
> >     free (old_name);
> >     free (new_name);
> > diff --git a/binutils/arsup.c b/binutils/arsup.c
> > index fa7706f79e5..f7ce8f0bc82 100644
> > --- a/binutils/arsup.c
> > +++ b/binutils/arsup.c
> > @@ -343,18 +343,11 @@ ar_save (void)
> >       }
> >     else
> >       {
> > -      bfd_boolean skip_stat = FALSE;
> >         struct stat target_stat;
> > -      int ofd = real_ofd;
> >           if (deterministic > 0)
> >           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 (ofd);
> > -#endif
> >         bfd_close (obfd);
> >           if (stat (real_name, &target_stat) != 0)
> > @@ -363,9 +356,6 @@ ar_save (void)
> >   	     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);
> > @@ -373,8 +363,7 @@ ar_save (void)
> >   	    }
> >   	}
> >   -      smart_rename (temp_name, real_name, ofd,
> > -		    skip_stat ? NULL : &target_stat, 0);
> > +      smart_rename (temp_name, real_name, NULL);
> >         obfd = 0;
> >         free (temp_name);
> >         free (real_name);
> > diff --git a/binutils/bucomm.h b/binutils/bucomm.h
> > index 7a0adfae565..aa7e33d8cd1 100644
> > --- a/binutils/bucomm.h
> > +++ b/binutils/bucomm.h
> > @@ -71,7 +71,7 @@ extern void print_version (const char *);
> >   /* In rename.c.  */
> >   extern void set_times (const char *, const struct stat *);
> >   -extern int smart_rename (const char *, const char *, int, struct stat *,
> > int);
> > +extern int smart_rename (const char *, const char *, struct stat *);
> >       /* In libiberty.  */
> > diff --git a/binutils/objcopy.c b/binutils/objcopy.c
> > index 0e1047e7482..378ee1535f3 100644
> > --- a/binutils/objcopy.c
> > +++ b/binutils/objcopy.c
> > @@ -4832,7 +4832,6 @@ strip_main (int argc, char *argv[])
> >         struct stat statbuf;
> >         char *tmpname;
> >         int tmpfd = -1;
> > -      int copyfd = -1;
> >           if (get_file_size (argv[i]) < 1)
> >   	{
> > @@ -4846,12 +4845,7 @@ strip_main (int argc, char *argv[])
> >         else
> >   	tmpname = output_file;
> >   -      if (tmpname == NULL
> > -#if !defined (_WIN32) || defined (__CYGWIN32__)
> > -	  /* Retain a copy of TMPFD since we will need it for SMART_RENAME.
> > */
> > -	  || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
> > -#endif
> > -      )
> > +      if (tmpname == NULL)
> >   	{
> >   	  bfd_nonfatal_message (argv[i], NULL, NULL,
> >   				_("could not create temporary file to hold
> > stripped copy"));
> > @@ -4864,23 +4858,15 @@ strip_main (int argc, char *argv[])
> >   		 output_target, NULL);
> >         if (status == 0)
> >   	{
> > -	  if (preserve_dates)
> > -	    set_times (tmpname, &statbuf);
> >   	  if (output_file != tmpname)
> >   	    status = (smart_rename (tmpname,
> >   				    output_file ? output_file : argv[i],
> > -				    copyfd, &statbuf, preserve_dates) != 0);
> > +				    preserve_dates ? &statbuf : NULL) != 0);
> >   	  if (status == 0)
> >   	    status = hold_status;
> >   	}
> >         else
> > -	{
> > -#if !defined (_WIN32) || defined (__CYGWIN32__)
> > -	  if (copyfd >= 0)
> > -	    close (copyfd);
> > -#endif
> > -	  unlink_if_ordinary (tmpname);
> > -	}
> > +	unlink_if_ordinary (tmpname);
> >         if (output_file != tmpname)
> >   	free (tmpname);
> >       }
> > @@ -5088,7 +5074,6 @@ copy_main (int argc, char *argv[])
> >     bfd_boolean use_globalize = FALSE;
> >     bfd_boolean use_keep_global = FALSE;
> >     int c, tmpfd = -1;
> > -  int copyfd = -1;
> >     struct stat statbuf;
> >     const bfd_arch_info_type *input_arch = NULL;
> >   @@ -5933,12 +5918,7 @@ copy_main (int argc, char *argv[])
> >     else
> >       tmpname = output_filename;
> >   -  if (tmpname == NULL
> > -#if !defined (_WIN32) || defined (__CYGWIN32__)
> > -      /* Retain a copy of TMPFD since we will need it for SMART_RENAME.  */
> > -      || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
> > -#endif
> > -  )
> > +  if (tmpname == NULL)
> >       {
> >         fatal (_("warning: could not create temporary file whilst copying
> > '%s', (error: %s)"),
> >   	     input_filename, strerror (errno));
> > @@ -5948,20 +5928,12 @@ copy_main (int argc, char *argv[])
> >   	     output_target, input_arch);
> >     if (status == 0)
> >       {
> > -      if (preserve_dates)
> > -	set_times (tmpname, &statbuf);
> >         if (tmpname != output_filename)
> > -	status = (smart_rename (tmpname, input_filename, copyfd, &statbuf,
> > -				preserve_dates) != 0);
> > +	status = (smart_rename (tmpname, input_filename,
> > +				preserve_dates ? &statbuf : NULL) != 0);
> >       }
> >     else
> > -    {
> > -#if !defined (_WIN32) || defined (__CYGWIN32__)
> > -      if (copyfd >= 0)
> > -	close (copyfd);
> > -#endif
> > -      unlink_if_ordinary (tmpname);
> > -    }
> > +    unlink_if_ordinary (tmpname);
> >       if (tmpname != output_filename)
> >       free (tmpname);
> > diff --git a/binutils/rename.c b/binutils/rename.c
> > index e36b75132de..2ff092ee22b 100644
> > --- a/binutils/rename.c
> > +++ b/binutils/rename.c
> > @@ -122,61 +122,13 @@ set_times (const char *destination, const struct stat
> > *statbuf)
> >       non_fatal (_("%s: cannot set time: %s"), destination, strerror
> > (errno));
> >   }
> >   -#ifndef S_ISLNK
> > -#ifdef S_IFLNK
> > -#define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
> > -#else
> > -#define S_ISLNK(m) 0
> > -#define lstat stat
> > -#endif
> > -#endif
> > -
> > -#if !defined (_WIN32) || defined (__CYGWIN32__)
> > -/* Try to preserve the permission bits and ownership of an existing file
> > when
> > -   rename overwrites it.  FD is the file being renamed and TARGET_STAT has
> > the
> > -   status of the file that was overwritten.  */
> > -static void
> > -try_preserve_permissions (int fd, struct stat *target_stat)
> > -{
> > -  struct stat from_stat;
> > -  int ret = 0;
> > -
> > -  if (fstat (fd, &from_stat) != 0)
> > -    return;
> > -
> > -  int from_mode = from_stat.st_mode & 0777;
> > -  int to_mode = target_stat->st_mode & 0777;
> > -
> > -  /* Fix up permissions before we potentially lose ownership with fchown.
> > -     Clear the setxid bits because in case the fchown below fails then we
> > don't
> > -     want to end up with a sxid file owned by the invoking user.  If the
> > user
> > -     hasn't changed or if fchown succeeded, we add back the sxid bits at
> > the
> > -     end.  */
> > -  if (from_mode != to_mode)
> > -    fchmod (fd, to_mode);
> > -
> > -  /* Fix up ownership, this will clear the setxid bits.  */
> > -  if (from_stat.st_uid != target_stat->st_uid
> > -      || from_stat.st_gid != target_stat->st_gid)
> > -    ret = fchown (fd, target_stat->st_uid, target_stat->st_gid);
> > -
> > -  /* Fix up the sxid bits if either the fchown wasn't needed or it
> > -     succeeded.  */
> > -  if (ret == 0)
> > -    fchmod (fd, target_stat->st_mode & 07777);
> > -}
> > -#endif
> > -
> > -/* Rename FROM to TO, copying if TO is either a link or is not a regular
> > file.
> > -   FD is an open file descriptor pointing to FROM that we can use to safely
> > fix
> > -   up permissions of the file after renaming.  TARGET_STAT has the file
> > status
> > -   that is used to fix up permissions and timestamps after rename.  Return
> > 0 if
> > -   ok, -1 if error and FD is closed before returning.  */
> > +/* Rename FROM to TO, copying if TO exists.  TARGET_STAT has the file
> > status
> > +   that, if non-NULL, is used to fix up timestamps after rename.  Return 0
> > if
> > +   ok, -1 if error.  */
> >     int
> > -smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
> > -	      struct stat *target_stat ATTRIBUTE_UNUSED,
> > -	      int preserve_dates ATTRIBUTE_UNUSED)
> > +smart_rename (const char *from, const char *to,
> > +	      struct stat *target_stat ATTRIBUTE_UNUSED)
> >   {
> >     int ret = 0;
> >     struct stat to_stat;
> > @@ -199,37 +151,10 @@ smart_rename (const char *from, const char *to, int fd
> > ATTRIBUTE_UNUSED,
> >         unlink (from);
> >       }
> >   #else
> > -  /* Avoid a full copy and use rename if we can fix up permissions of the
> > -     file after renaming, i.e.:
> > -
> > -     - TO is not a symbolic link
> > -     - TO is a regular file with only one hard link
> > -     - We have permission to write to TO
> > -     - FD is available to safely fix up permissions to be the same as the
> > file
> > -       we overwrote with the rename.
> > -
> > -     Note though that the actual file on disk that TARGET_STAT describes
> > may
> > -     have changed and we're only trying to preserve the status we know
> > about.
> > -     At no point do we try to interact with the new file changes, so there
> > can
> > -     only be two outcomes, i.e. either the external file change survives
> > -     without knowledge of our change (if it happens after the rename
> > syscall)
> > -     or our rename and permissions fixup survive without any knowledge of
> > the
> > -     external change.  */
> > -  if (! exists
> > -      || (fd >= 0
> > -	  && !S_ISLNK (to_stat.st_mode)
> > -	  && S_ISREG (to_stat.st_mode)
> > -	  && (to_stat.st_mode & S_IWUSR)
> > -	  && to_stat.st_nlink == 1)
> > -      )
> > +  /* Avoid a full copy and use rename if TO does not exist.  */
> > +  if (!exists)
> >       {
> > -      ret = rename (from, to);
> > -      if (ret == 0)
> > -	{
> > -	  if (exists && target_stat != NULL)
> > -	    try_preserve_permissions (fd, target_stat);
> > -	}
> > -      else
> > +      if ((ret = rename (from, to)) != 0)
> >   	{
> >   	  /* We have to clean up here.  */
> >   	  non_fatal (_("unable to rename '%s'; reason: %s"), to, strerror
> > (errno));
> > @@ -242,12 +167,10 @@ smart_rename (const char *from, const char *to, int fd
> > ATTRIBUTE_UNUSED,
> >         if (ret != 0)
> >   	non_fatal (_("unable to copy file '%s'; reason: %s"), to, strerror
> > (errno));
> >   -      if (preserve_dates && target_stat != NULL)
> > +      if (target_stat != NULL)
> >   	set_times (to, target_stat);
> >         unlink (from);
> >       }
> > -  if (fd >= 0)
> > -    close (fd);
> >   #endif /* _WIN32 && !__CYGWIN32__ */
> >       return ret;
> > 
> 

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

On 2/15/21 9:29 PM, Michael Matz wrote:
> What happens when stripping executables that are currently executing?
> You can't open them for writing (ETXTBSY), but you can delete/rename them.
> (There are other reasons why nominally writable files can't be opened for
> writing while delete/rename is possible)
> Does the behaviour change with your patch?

strip (or objcopy, etc.) will fail for existing files if the write fails 
for any reason, including ETXTBSY.  It's a tradeoff to preserve 
permissions and attributes; attempting to restore those is an endless 
race since there will always be some that binutils will miss.

Siddhesh

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Nick Clifton]

Hi Siddhesh,

Sorry for the long delay in looking at this patch.

>> binutils/
>>
>>     * ar.c (write_archive): Remove TARGET_STAT.  Adjust call to
>>     SMART_RENAME.
>>     * arsup.c (ar_save): Likewise.
>>     * objcopy (strip_main): Don't copy TMPFD.  Don't set times on
>>     temporary file and adjust call to SMART_RENAME.
>>     (copy_main): Likewise.
>>     * rename.c [!S_ISLNK]: Remove definitions.
>>     (try_preserve_permissions): Remove function.
>>     (smart_rename): Remove FD, PRESERVE_DATES arguments.  Use
>>     rename system call only if TO does not exist.
>>     * bucomm.h (smart_rename): Adjust declaration.
>> ---

Approved - please apply.

Cheers
   Nick

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

On 2/19/21 12:50 AM, Nick Clifton wrote:
> Hi Siddhesh,
> 
> Sorry for the long delay in looking at this patch.
> 
>>> binutils/
>>>
>>>     * ar.c (write_archive): Remove TARGET_STAT.  Adjust call to
>>>     SMART_RENAME.
>>>     * arsup.c (ar_save): Likewise.
>>>     * objcopy (strip_main): Don't copy TMPFD.  Don't set times on
>>>     temporary file and adjust call to SMART_RENAME.
>>>     (copy_main): Likewise.
>>>     * rename.c [!S_ISLNK]: Remove definitions.
>>>     (try_preserve_permissions): Remove function.
>>>     (smart_rename): Remove FD, PRESERVE_DATES arguments.  Use
>>>     rename system call only if TO does not exist.
>>>     * bucomm.h (smart_rename): Adjust declaration.
>>> ---
> 
> Approved - please apply.

Thanks, pushed.

Siddhesh

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Matthias Klose]

On 2/19/21 3:37 AM, Siddhesh Poyarekar wrote:
> On 2/19/21 12:50 AM, Nick Clifton wrote:
>> Hi Siddhesh,
>>
>> Sorry for the long delay in looking at this patch.
>>
>>>> binutils/
>>>>
>>>>     * ar.c (write_archive): Remove TARGET_STAT.  Adjust call to
>>>>     SMART_RENAME.
>>>>     * arsup.c (ar_save): Likewise.
>>>>     * objcopy (strip_main): Don't copy TMPFD.  Don't set times on
>>>>     temporary file and adjust call to SMART_RENAME.
>>>>     (copy_main): Likewise.
>>>>     * rename.c [!S_ISLNK]: Remove definitions.
>>>>     (try_preserve_permissions): Remove function.
>>>>     (smart_rename): Remove FD, PRESERVE_DATES arguments.  Use
>>>>     rename system call only if TO does not exist.
>>>>     * bucomm.h (smart_rename): Adjust declaration.
>>>> ---
>>
>> Approved - please apply.
> 
> Thanks, pushed.
> 
> Siddhesh

what's the plan for the 2.36 branch?

Matthias

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

On 2/19/21 1:14 PM, Matthias Klose wrote:
> On 2/19/21 3:37 AM, Siddhesh Poyarekar wrote:
>> On 2/19/21 12:50 AM, Nick Clifton wrote:
>>> Hi Siddhesh,
>>>
>>> Sorry for the long delay in looking at this patch.
>>>
>>>>> binutils/
>>>>>
>>>>>      * ar.c (write_archive): Remove TARGET_STAT.  Adjust call to
>>>>>      SMART_RENAME.
>>>>>      * arsup.c (ar_save): Likewise.
>>>>>      * objcopy (strip_main): Don't copy TMPFD.  Don't set times on
>>>>>      temporary file and adjust call to SMART_RENAME.
>>>>>      (copy_main): Likewise.
>>>>>      * rename.c [!S_ISLNK]: Remove definitions.
>>>>>      (try_preserve_permissions): Remove function.
>>>>>      (smart_rename): Remove FD, PRESERVE_DATES arguments.  Use
>>>>>      rename system call only if TO does not exist.
>>>>>      * bucomm.h (smart_rename): Adjust declaration.
>>>>> ---
>>>
>>> Approved - please apply.
>>
>> Thanks, pushed.
>>
>> Siddhesh
> 
> what's the plan for the 2.36 branch?

I don't know.  Nick, Alan, should this be backported to 2.36?

Siddhesh

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Nick Clifton]

Hi Siddhesh,

> I don't know.  Nick, Alan, should this be backported to 2.36?

Yes please.

Cheers
   Nick

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

On 2/19/21 10:15 PM, Nick Clifton wrote:
> Hi Siddhesh,
> 
>> I don't know.  Nick, Alan, should this be backported to 2.36?
> 
> Yes please.

Thanks, backported and pushed to binutils-2_36-branch.

Siddhesh

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Alan Modra]

On Mon, Feb 22, 2021 at 09:00:42PM +0530, Siddhesh Poyarekar wrote:
> On 2/19/21 10:15 PM, Nick Clifton wrote:
> > Hi Siddhesh,
> > 
> > > I don't know.  Nick, Alan, should this be backported to 2.36?
> > 
> > Yes please.
> 
> Thanks, backported and pushed to binutils-2_36-branch.

See pr27456.  It seems to me that smart_rename now will always be
copying the temp file contents to the output file in the non-windows
case, and that we can make it do the same for windows too.  I'm
testing the following.

	PR 27456
	* rename.c: Tidy throughout.
	(smart_rename): Always copy.  Remove windows specific code.

diff --git a/binutils/rename.c b/binutils/rename.c
index 2ff092ee22..72a9323d72 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -24,14 +24,9 @@
 
 #ifdef HAVE_GOOD_UTIME_H
 #include <utime.h>
-#else /* ! HAVE_GOOD_UTIME_H */
-#ifdef HAVE_UTIMES
+#elif defined HAVE_UTIMES
 #include <sys/time.h>
-#endif /* HAVE_UTIMES */
-#endif /* ! HAVE_GOOD_UTIME_H */
-
-#if ! defined (_WIN32) || defined (__CYGWIN32__)
-static int simple_copy (const char *, const char *);
+#endif
 
 /* The number of bytes to copy at once.  */
 #define COPY_BUF 8192
@@ -82,7 +77,6 @@ simple_copy (const char *from, const char *to)
     }
   return 0;
 }
-#endif /* __CYGWIN32__ or not _WIN32 */
 
 /* Set the times of the file DESTINATION to be the same as those in
    STATBUF.  */
@@ -91,87 +85,52 @@ void
 set_times (const char *destination, const struct stat *statbuf)
 {
   int result;
-
-  {
 #ifdef HAVE_GOOD_UTIME_H
-    struct utimbuf tb;
-
-    tb.actime = statbuf->st_atime;
-    tb.modtime = statbuf->st_mtime;
-    result = utime (destination, &tb);
-#else /* ! HAVE_GOOD_UTIME_H */
-#ifndef HAVE_UTIMES
-    long tb[2];
-
-    tb[0] = statbuf->st_atime;
-    tb[1] = statbuf->st_mtime;
-    result = utime (destination, tb);
-#else /* HAVE_UTIMES */
-    struct timeval tv[2];
-
-    tv[0].tv_sec = statbuf->st_atime;
-    tv[0].tv_usec = 0;
-    tv[1].tv_sec = statbuf->st_mtime;
-    tv[1].tv_usec = 0;
-    result = utimes (destination, tv);
-#endif /* HAVE_UTIMES */
-#endif /* ! HAVE_GOOD_UTIME_H */
-  }
+  struct utimbuf tb;
+
+  tb.actime = statbuf->st_atime;
+  tb.modtime = statbuf->st_mtime;
+  result = utime (destination, &tb);
+#elif defined HAVE_UTIMES
+  struct timeval tv[2];
+
+  tv[0].tv_sec = statbuf->st_atime;
+  tv[0].tv_usec = 0;
+  tv[1].tv_sec = statbuf->st_mtime;
+  tv[1].tv_usec = 0;
+  result = utimes (destination, tv);
+#else
+  long tb[2];
+
+  tb[0] = statbuf->st_atime;
+  tb[1] = statbuf->st_mtime;
+  result = utime (destination, tb);
+#endif
 
   if (result != 0)
     non_fatal (_("%s: cannot set time: %s"), destination, strerror (errno));
 }
 
-/* Rename FROM to TO, copying if TO exists.  TARGET_STAT has the file status
-   that, if non-NULL, is used to fix up timestamps after rename.  Return 0 if
-   ok, -1 if error.  */
+/* Copy FROM to TO.  TARGET_STAT has the file status that, if non-NULL,
+   is used to fix up timestamps.  Return 0 if ok, -1 if error.
+   At one time this function renamed files, but file permissions are
+   tricky to update given the number of different schemes used by
+   various systems.  So now we just copy.  */
 
 int
 smart_rename (const char *from, const char *to,
-	      struct stat *target_stat ATTRIBUTE_UNUSED)
+	      struct stat *target_stat)
 {
-  int ret = 0;
-  struct stat to_stat;
-  bfd_boolean exists;
-
-  exists = lstat (to, &to_stat) == 0;
-
-#if defined (_WIN32) && !defined (__CYGWIN32__)
-  /* Win32, unlike unix, will not erase `to' in `rename(from, to)' but
-     fail instead.  Also, chown is not present.  */
-
-  if (exists)
-    remove (to);
+  int ret;
 
-  ret = rename (from, to);
+  ret = simple_copy (from, to);
   if (ret != 0)
-    {
-      /* We have to clean up here.  */
-      non_fatal (_("unable to rename '%s'; reason: %s"), to, strerror (errno));
-      unlink (from);
-    }
-#else
-  /* Avoid a full copy and use rename if TO does not exist.  */
-  if (!exists)
-    {
-      if ((ret = rename (from, to)) != 0)
-	{
-	  /* We have to clean up here.  */
-	  non_fatal (_("unable to rename '%s'; reason: %s"), to, strerror (errno));
-	  unlink (from);
-	}
-    }
-  else
-    {
-      ret = simple_copy (from, to);
-      if (ret != 0)
-	non_fatal (_("unable to copy file '%s'; reason: %s"), to, strerror (errno));
+    non_fatal (_("unable to copy file '%s'; reason: %s"),
+	       to, strerror (errno));
 
-      if (target_stat != NULL)
-	set_times (to, target_stat);
-      unlink (from);
-    }
-#endif /* _WIN32 && !__CYGWIN32__ */
+  if (target_stat != NULL)
+    set_times (to, target_stat);
+  unlink (from);
 
   return ret;
 }


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

On 2/23/21 5:30 AM, Alan Modra wrote:
> On Mon, Feb 22, 2021 at 09:00:42PM +0530, Siddhesh Poyarekar wrote:
>> On 2/19/21 10:15 PM, Nick Clifton wrote:
>>> Hi Siddhesh,
>>>
>>>> I don't know.  Nick, Alan, should this be backported to 2.36?
>>>
>>> Yes please.
>>
>> Thanks, backported and pushed to binutils-2_36-branch.
> 
> See pr27456.  It seems to me that smart_rename now will always be
> copying the temp file contents to the output file in the non-windows
> case, and that we can make it do the same for windows too.  I'm
> testing the following.

Indeed, I had forgotten that an empty output file always gets created.

Thanks,
Siddhesh

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Fangrui Song]

On 2021-02-23, Siddhesh Poyarekar wrote:
>On 2/23/21 5:30 AM, Alan Modra wrote:
>>On Mon, Feb 22, 2021 at 09:00:42PM +0530, Siddhesh Poyarekar wrote:
>>>On 2/19/21 10:15 PM, Nick Clifton wrote:
>>>>Hi Siddhesh,
>>>>
>>>>>I don't know.  Nick, Alan, should this be backported to 2.36?
>>>>
>>>>Yes please.
>>>
>>>Thanks, backported and pushed to binutils-2_36-branch.
>>
>>See pr27456.  It seems to me that smart_rename now will always be
>>copying the temp file contents to the output file in the non-windows
>>case, and that we can make it do the same for windows too.  I'm
>>testing the following.
>
>Indeed, I had forgotten that an empty output file always gets created.
>
>Thanks,
>Siddhesh

On latest commit, there is a behavior difference. I don't know whether it is intended.

# S_ISUID and S_ISGID bits are cleared
chmod 7777 a.o; ls -l a.o; ~/Dev/binutils-gdb/Debug/binutils/strip-new -p a.o; ls -l a.o

vs system

# S_ISUID and S_ISGID bits are preserved
chmod 7777 a.o; ls -l a.o; strip -p a.o; ls -l a.o

Re: [PING][PATCH] binutils: Avoid renaming over existing files

[Siddhesh Poyarekar]

On 2/23/21 10:14 AM, Fangrui Song wrote:
> On latest commit, there is a behavior difference. I don't know whether 
> it is intended.
> 
> # S_ISUID and S_ISGID bits are cleared
> chmod 7777 a.o; ls -l a.o; ~/Dev/binutils-gdb/Debug/binutils/strip-new 
> -p a.o; ls -l a.o
> 
> vs system
> 
> # S_ISUID and S_ISGID bits are preserved
> chmod 7777 a.o; ls -l a.o; strip -p a.o; ls -l a.o

Hmm, so writing to a SUID/SGID file clears those bits; that's news to 
me.  It ought to be safe for binutils to attempt to only restore the 
permission bits of the destination right after the copy.  I'll share a 
patch shortly.

Siddhesh

[PATCH] binutils: Attempt to retain permissions on copying file

[Siddhesh Poyarekar]

Writing into an existing file clears its S_ISUID and S_ISGID bits.
Attempt to restore those permission bits but don't fail if it doesn't
work.

Also, since the output file always exists (all callers create an empty
file before calling smart_rename), open the file without any
permission hints or O_CREAT.

binutils/

	* rename.c (simple_copy): Don't use O_CREAT.
	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:
	Attempt to retain permission bits.
---

Tested on x86_64 to verify that it retains setuid/setgid bits directly
as well as via symlink.  The patch goes on top of Alan Modra's patch to
tidy up for Windows and unconditionally call simple_copy so that this
(hopefully) doesn't break WIN32 again:

https://sourceware.org/pipermail/binutils/2021-February/115472.html

 binutils/rename.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/binutils/rename.c b/binutils/rename.c
index 72a9323d72c..103f92866cd 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -41,14 +41,19 @@ simple_copy (const char *from, const char *to)
   int saved;
   char buf[COPY_BUF];
 
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+  /* Note permissions of the destination file before it is written to so that
+     we can try to restore it later.  */
+  struct stat to_stat;
+  mode_t to_mode = 0;
+  if (stat (to, &to_stat) == 0)
+    to_mode = to_stat.st_mode;
+#endif
+
   fromfd = open (from, O_RDONLY | O_BINARY);
   if (fromfd < 0)
     return -1;
-#ifdef O_CREAT
-  tofd = open (to, O_CREAT | O_WRONLY | O_TRUNC | O_BINARY, 0777);
-#else
-  tofd = creat (to, 0777);
-#endif
+  tofd = open (to, O_WRONLY | O_TRUNC | O_BINARY);
   if (tofd < 0)
     {
       saved = errno;
@@ -67,7 +72,16 @@ simple_copy (const char *from, const char *to)
 	  return -1;
 	}
     }
+
   saved = errno;
+
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+  /* Writing to a setuid/setgid file clears the S_ISUID and S_ISGID bits.
+     Try to restore them (ignore failure) before closing the file.  */
+  if (to_mode > 0)
+    fchmod (tofd, to_mode);
+#endif
+
   close (fromfd);
   close (tofd);
   if (nread < 0)
-- 
2.29.2

Re: [PATCH] binutils: Attempt to retain permissions on copying file

[Alan Modra]

On Tue, Feb 23, 2021 at 12:22:59PM +0530, Siddhesh Poyarekar wrote:
> Writing into an existing file clears its S_ISUID and S_ISGID bits.
> Attempt to restore those permission bits but don't fail if it doesn't
> work.
> 
> Also, since the output file always exists (all callers create an empty
> file before calling smart_rename), open the file without any
> permission hints or O_CREAT.
> 
> binutils/
> 
> 	* rename.c (simple_copy): Don't use O_CREAT.
> 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:
> 	Attempt to retain permission bits.

Thanks, I'll fold this into a followup patch of mine that makes use of
the temp file descriptor in smart_rename rather than reopening the
file.  I don't believe there is a security issue in reopening the
file, but this way there is one less directory operation.  I'm also
going to make use of the target_stat we already have rather than
calling stat again.

	* bucomm.h (smart_rename): Update prototype.
	* rename.c (smart_rename): Add fromfd and preserve_dates params.
	Pass fromfd and target_stat to simple_copy.  Call set_times
	when preserve_dates.
	(simple_copy): Accept fromfd rather than from filename.  Add
	target_stat param.  Rewind fromfd rather than opening.  Open
	"to" file without O_CREAT.  Try to preserve S_ISUID and S_ISGID.
	* ar.c (write_archive): Rename ofd to tmpfd.  Dup tmpfd before
	closing output temp file, and pass tmpfd to smart_rename.
	* arsup.c (temp_fd): Rename from real_fd.
	(ar_save): Dup temp_fd and pass to smart_rename.
	* objcopy.c (strip_main, copy_main): Likewise, and pass
	preserve_dates.

diff --git a/binutils/ar.c b/binutils/ar.c
index 44df48c5c6..fb19b14fec 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -1252,21 +1252,21 @@ write_archive (bfd *iarch)
   bfd *obfd;
   char *old_name, *new_name;
   bfd *contents_head = iarch->archive_next;
-  int ofd = -1;
+  int tmpfd = -1;
 
   old_name = xstrdup (bfd_get_filename (iarch));
-  new_name = make_tempname (old_name, &ofd);
+  new_name = make_tempname (old_name, &tmpfd);
 
   if (new_name == NULL)
     bfd_fatal (_("could not create temporary file whilst writing archive"));
 
   output_filename = new_name;
 
-  obfd = bfd_fdopenw (new_name, bfd_get_target (iarch), ofd);
+  obfd = bfd_fdopenw (new_name, bfd_get_target (iarch), tmpfd);
 
   if (obfd == NULL)
     {
-      close (ofd);
+      close (tmpfd);
       bfd_fatal (old_name);
     }
 
@@ -1297,6 +1297,7 @@ write_archive (bfd *iarch)
   if (!bfd_set_archive_head (obfd, contents_head))
     bfd_fatal (old_name);
 
+  tmpfd = dup (tmpfd);
   if (!bfd_close (obfd))
     bfd_fatal (old_name);
 
@@ -1306,7 +1307,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, NULL) != 0)
+  if (smart_rename (new_name, old_name, tmpfd, NULL, FALSE) != 0)
     xexit (1);
   free (old_name);
   free (new_name);
diff --git a/binutils/arsup.c b/binutils/arsup.c
index f7ce8f0bc8..9982484dbe 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -43,7 +43,7 @@ extern int deterministic;
 static bfd *obfd;
 static char *real_name;
 static char *temp_name;
-static int real_ofd;
+static int temp_fd;
 static FILE *outfile;
 
 static void
@@ -152,7 +152,7 @@ void
 ar_open (char *name, int t)
 {
   real_name = xstrdup (name);
-  temp_name = make_tempname (real_name, &real_ofd);
+  temp_name = make_tempname (real_name, &temp_fd);
 
   if (temp_name == NULL)
     {
@@ -162,7 +162,7 @@ ar_open (char *name, int t)
       return;
     }
 
-  obfd = bfd_fdopenw (temp_name, NULL, real_ofd);
+  obfd = bfd_fdopenw (temp_name, NULL, temp_fd);
 
   if (!obfd)
     {
@@ -348,6 +348,7 @@ ar_save (void)
       if (deterministic > 0)
         obfd->flags |= BFD_DETERMINISTIC_OUTPUT;
 
+      temp_fd = dup (temp_fd);
       bfd_close (obfd);
 
       if (stat (real_name, &target_stat) != 0)
@@ -363,7 +364,7 @@ ar_save (void)
 	    }
 	}
 
-      smart_rename (temp_name, real_name, NULL);
+      smart_rename (temp_name, real_name, temp_fd, NULL, FALSE);
       obfd = 0;
       free (temp_name);
       free (real_name);
diff --git a/binutils/bucomm.h b/binutils/bucomm.h
index aa7e33d8cd..f1ae47fa1b 100644
--- a/binutils/bucomm.h
+++ b/binutils/bucomm.h
@@ -71,7 +71,8 @@ extern void print_version (const char *);
 /* In rename.c.  */
 extern void set_times (const char *, const struct stat *);
 
-extern int smart_rename (const char *, const char *, struct stat *);
+extern int smart_rename (const char *, const char *, int,
+			 struct stat *, bfd_boolean);
 
 
 /* In libiberty.  */
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index abbcb7c519..90ae0bd46b 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4837,6 +4837,7 @@ strip_main (int argc, char *argv[])
       struct stat statbuf;
       char *tmpname;
       int tmpfd = -1;
+      int copyfd = -1;
 
       if (get_file_size (argv[i]) < 1)
 	{
@@ -4846,7 +4847,11 @@ strip_main (int argc, char *argv[])
 
       if (output_file == NULL
 	  || filename_cmp (argv[i], output_file) == 0)
-	tmpname = make_tempname (argv[i], &tmpfd);
+	{
+	  tmpname = make_tempname (argv[i], &tmpfd);
+	  if (tmpfd >= 0)
+	    copyfd = dup (tmpfd);
+	}
       else
 	tmpname = output_file;
 
@@ -4864,14 +4869,18 @@ strip_main (int argc, char *argv[])
       if (status == 0)
 	{
 	  if (output_file != tmpname)
-	    status = (smart_rename (tmpname,
-				    output_file ? output_file : argv[i],
-				    preserve_dates ? &statbuf : NULL) != 0);
+	    status = smart_rename (tmpname,
+				   output_file ? output_file : argv[i],
+				   copyfd, &statbuf, preserve_dates) != 0;
 	  if (status == 0)
 	    status = hold_status;
 	}
       else
-	unlink_if_ordinary (tmpname);
+	{
+	  if (copyfd >= 0)
+	    close (copyfd);
+	  unlink_if_ordinary (tmpname);
+	}
       if (output_file != tmpname)
 	free (tmpname);
     }
@@ -5078,7 +5087,9 @@ copy_main (int argc, char *argv[])
   bfd_boolean formats_info = FALSE;
   bfd_boolean use_globalize = FALSE;
   bfd_boolean use_keep_global = FALSE;
-  int c, tmpfd = -1;
+  int c;
+  int tmpfd = -1;
+  int copyfd;
   struct stat statbuf;
   const bfd_arch_info_type *input_arch = NULL;
 
@@ -5916,10 +5927,15 @@ copy_main (int argc, char *argv[])
     }
 
   /* If there is no destination file, or the source and destination files
-     are the same, then create a temp and rename the result into the input.  */
+     are the same, then create a temp and copy the result into the input.  */
+  copyfd = -1;
   if (output_filename == NULL
       || filename_cmp (input_filename, output_filename) == 0)
-    tmpname = make_tempname (input_filename, &tmpfd);
+    {
+      tmpname = make_tempname (input_filename, &tmpfd);
+      if (tmpfd >= 0)
+	copyfd = dup (tmpfd);
+    }
   else
     tmpname = output_filename;
 
@@ -5934,11 +5950,15 @@ copy_main (int argc, char *argv[])
   if (status == 0)
     {
       if (tmpname != output_filename)
-	status = (smart_rename (tmpname, input_filename,
-				preserve_dates ? &statbuf : NULL) != 0);
+	status = smart_rename (tmpname, input_filename, copyfd,
+			       &statbuf, preserve_dates) != 0;
     }
   else
-    unlink_if_ordinary (tmpname);
+    {
+      if (copyfd >= 0)
+	close (copyfd);
+      unlink_if_ordinary (tmpname);
+    }
 
   if (tmpname != output_filename)
     free (tmpname);
diff --git a/binutils/rename.c b/binutils/rename.c
index 72a9323d72..f688f350d5 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -31,24 +31,21 @@
 /* The number of bytes to copy at once.  */
 #define COPY_BUF 8192
 
-/* Copy file FROM to file TO, performing no translations.
+/* Copy file FROMFD to file TO, performing no translations.
    Return 0 if ok, -1 if error.  */
 
 static int
-simple_copy (const char *from, const char *to)
+simple_copy (int fromfd, const char *to, struct stat *target_stat)
 {
-  int fromfd, tofd, nread;
+  int tofd, nread;
   int saved;
   char buf[COPY_BUF];
 
-  fromfd = open (from, O_RDONLY | O_BINARY);
-  if (fromfd < 0)
+  if (fromfd < 0
+      || lseek (fromfd, 0, SEEK_SET) != 0)
     return -1;
-#ifdef O_CREAT
-  tofd = open (to, O_CREAT | O_WRONLY | O_TRUNC | O_BINARY, 0777);
-#else
-  tofd = creat (to, 0777);
-#endif
+
+  tofd = open (to, O_WRONLY | O_TRUNC | O_BINARY);
   if (tofd < 0)
     {
       saved = errno;
@@ -56,6 +53,7 @@ simple_copy (const char *from, const char *to)
       errno = saved;
       return -1;
     }
+
   while ((nread = read (fromfd, buf, sizeof buf)) > 0)
     {
       if (write (tofd, buf, nread) != nread)
@@ -67,7 +65,16 @@ simple_copy (const char *from, const char *to)
 	  return -1;
 	}
     }
+
   saved = errno;
+
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+  /* Writing to a setuid/setgid file may clear S_ISUID and S_ISGID.
+     Try to restore them, ignoring failure.  */
+  if (target_stat != NULL)
+    fchmod (tofd, target_stat->st_mode);
+#endif
+
   close (fromfd);
   close (tofd);
   if (nread < 0)
@@ -118,17 +125,17 @@ set_times (const char *destination, const struct stat *statbuf)
    various systems.  So now we just copy.  */
 
 int
-smart_rename (const char *from, const char *to,
-	      struct stat *target_stat)
+smart_rename (const char *from, const char *to, int fromfd,
+	      struct stat *target_stat, bfd_boolean preserve_dates)
 {
   int ret;
 
-  ret = simple_copy (from, to);
+  ret = simple_copy (fromfd, to, target_stat);
   if (ret != 0)
     non_fatal (_("unable to copy file '%s'; reason: %s"),
 	       to, strerror (errno));
 
-  if (target_stat != NULL)
+  if (preserve_dates)
     set_times (to, target_stat);
   unlink (from);
 


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] binutils: Attempt to retain permissions on copying file

[Marco Atzeri]

On 23.02.2021 07:52, Siddhesh Poyarekar wrote:
> Writing into an existing file clears its S_ISUID and S_ISGID bits.
> Attempt to restore those permission bits but don't fail if it doesn't
> work.
> 
> Also, since the output file always exists (all callers create an empty
> file before calling smart_rename), open the file without any
> permission hints or O_CREAT.
> 
> binutils/
> 
> 	* rename.c (simple_copy): Don't use O_CREAT.
> 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:
> 	Attempt to retain permission bits.


pay attention that __CYGWIN32__ is deprecated and
instead should be used __CYGWIN__

it exists only for obsolete compatibility

on 32bit platform

$ gcc -E -dM - </dev/null | grep CYGWIN
#define __CYGWIN__ 1
#define __CYGWIN32__ 1

on 64 bit

$ gcc -E -dM - </dev/null | grep CYGWIN
#define __CYGWIN__ 1

Re: [PATCH] binutils: Attempt to retain permissions on copying file

[Siddhesh Poyarekar]

On 2/23/21 5:26 PM, Alan Modra wrote:
> On Tue, Feb 23, 2021 at 12:22:59PM +0530, Siddhesh Poyarekar wrote:
>> Writing into an existing file clears its S_ISUID and S_ISGID bits.
>> Attempt to restore those permission bits but don't fail if it doesn't
>> work.
>>
>> Also, since the output file always exists (all callers create an empty
>> file before calling smart_rename), open the file without any
>> permission hints or O_CREAT.
>>
>> binutils/
>>
>> 	* rename.c (simple_copy): Don't use O_CREAT.
>> 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:
>> 	Attempt to retain permission bits.
> 
> Thanks, I'll fold this into a followup patch of mine that makes use of
> the temp file descriptor in smart_rename rather than reopening the
> file.  I don't believe there is a security issue in reopening the
> file, but this way there is one less directory operation.  I'm also
> going to make use of the target_stat we already have rather than
> calling stat again.

I just tested tip and it looks in good shape.  I suppose this ought to 
go into 2.36 too since the simple_copy patch went in there too.

Thanks,
Siddhesh

Re: [PATCH] binutils: Attempt to retain permissions on copying file

[Alan Modra]

On Wed, Feb 24, 2021 at 03:57:10PM +0530, Siddhesh Poyarekar wrote:
> On 2/23/21 5:26 PM, Alan Modra wrote:
> > On Tue, Feb 23, 2021 at 12:22:59PM +0530, Siddhesh Poyarekar wrote:
> > > Writing into an existing file clears its S_ISUID and S_ISGID bits.
> > > Attempt to restore those permission bits but don't fail if it doesn't
> > > work.
> > > 
> > > Also, since the output file always exists (all callers create an empty
> > > file before calling smart_rename), open the file without any
> > > permission hints or O_CREAT.
> > > 
> > > binutils/
> > > 
> > > 	* rename.c (simple_copy): Don't use O_CREAT.
> > > 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:
> > > 	Attempt to retain permission bits.
> > 
> > Thanks, I'll fold this into a followup patch of mine that makes use of
> > the temp file descriptor in smart_rename rather than reopening the
> > file.  I don't believe there is a security issue in reopening the
> > file, but this way there is one less directory operation.  I'm also
> > going to make use of the target_stat we already have rather than
> > calling stat again.
> 
> I just tested tip and it looks in good shape.  I suppose this ought to go
> into 2.36 too since the simple_copy patch went in there too.

I thought we had reverted all the rename.c changes on the branch?

Anyway, the following needs to go on mainline, illustrating why it's
not a good idea to rush patches into a stable branch.  Applied.

	PR 27456
	* rename.c (simple_copy): Mark target_stat ATTRIBUTE_UNUSED.

diff --git a/binutils/rename.c b/binutils/rename.c
index f688f350d5..861c2b56d1 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -35,7 +35,8 @@
    Return 0 if ok, -1 if error.  */
 
 static int
-simple_copy (int fromfd, const char *to, struct stat *target_stat)
+simple_copy (int fromfd, const char *to,
+	     struct stat *target_stat ATTRIBUTE_UNUSED)
 {
   int tofd, nread;
   int saved;


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] binutils: Attempt to retain permissions on copying file

[Siddhesh Poyarekar]

On 2/25/21 5:17 AM, Alan Modra wrote:
> I thought we had reverted all the rename.c changes on the branch?
> 
> Anyway, the following needs to go on mainline, illustrating why it's
> not a good idea to rush patches into a stable branch.  Applied.

Got it, I'll keep that in mind for future changes.

Thanks,
Siddhesh

Re: [PATCH] binutils: Attempt to retain permissions on copying file

[Alan Modra]

I've pushed a series of the smart_rename patches to the branch in
order to correct the current failure to compile on MinGW.

-- 
Alan Modra
Australia Development Lab, IBM