[PING][PATCH] binutils: Avoid renaming over existing files
Michael Matz
matz@suse.de
Mon Feb 15 15:59:30 GMT 2021
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;
> >
>
More information about the Binutils
mailing list