[PING][PATCH] binutils: Avoid renaming over existing files
Siddhesh Poyarekar
siddhesh@gotplt.org
Mon Feb 15 05:17:45 GMT 2021
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;
>
More information about the Binutils
mailing list