From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dog.birch.relay.mailchannels.net (dog.birch.relay.mailchannels.net [23.83.209.48]) by sourceware.org (Postfix) with ESMTPS id 657D93858C27 for ; Mon, 15 Feb 2021 05:17:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 657D93858C27 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=siddhesh@gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 5A168322C41 for ; Mon, 15 Feb 2021 05:17:51 +0000 (UTC) Received: from pdx1-sub0-mail-a3.g.dreamhost.com (100-98-118-114.trex.outbound.svc.cluster.local [100.98.118.114]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id D14E2322C21 for ; Mon, 15 Feb 2021 05:17:50 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a3.g.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384) by 100.98.118.114 (trex/6.1.1); Mon, 15 Feb 2021 05:17:51 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Stop-Unite: 601959be3a8e1c90_1613366271143_3099169550 X-MC-Loop-Signature: 1613366271143:3338711375 X-MC-Ingress-Time: 1613366271143 Received: from pdx1-sub0-mail-a3.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a3.g.dreamhost.com (Postfix) with ESMTP id 94F527EFD1 for ; Sun, 14 Feb 2021 21:17:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gotplt.org; h=subject:from :to:references:message-id:date:mime-version:in-reply-to :content-type:content-transfer-encoding; s=gotplt.org; bh=XVtsy7 FeJ0AqwDgS9LeXo/WvX34=; b=gIWxR8r0+adaI9ayqMo+q9cBfWe9z9J+tL2OTJ eIpYxrWb6HOdxn+ipALp/npPFmY26nMT0V8gWJVpGfBb6DXkWLKlwdmyMPvtZ5rU +9AiJZdVf6BjrFm3hixdc4PoQal7DQjpfFaFu3ZF23HA1HaVcVyei06tCD6uGVVO ZEw58= Received: from [192.168.1.111] (unknown [1.186.101.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a3.g.dreamhost.com (Postfix) with ESMTPSA id ADAAC8B59B for ; Sun, 14 Feb 2021 21:17:49 -0800 (PST) Subject: [PING][PATCH] binutils: Avoid renaming over existing files X-DH-BACKEND: pdx1-sub0-mail-a3 From: Siddhesh Poyarekar To: binutils@sourceware.org References: <20210208191749.233922-1-siddhesh@gotplt.org> Message-ID: <6066dd62-aa26-40a6-c4ff-67a3cd22d4d3@gotplt.org> Date: Mon, 15 Feb 2021 10:47:45 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <20210208191749.233922-1-siddhesh@gotplt.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3037.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Feb 2021 05:17:55 -0000 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; >