Message ID | mvm5ztmpb3p.fsf@suse.de |
---|---|
State | New, archived |
Headers |
Received: (qmail 57161 invoked by alias); 14 Feb 2019 16:51:26 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 57152 invoked by uid 89); 14 Feb 2019 16:51:25 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=auxiliary X-HELO: mx1.suse.de From: Andreas Schwab <schwab@suse.de> To: libc-alpha@sourceware.org Subject: [PATCH] Avoid concurrency problem in ldconfig (bug 23973) X-Yow: Talking Pinhead Blues: Oh, I LOST my ``HELLO KITTY'' DOLL and I get BAD reception on channel TWENTY-SIX!! Th'HOSTESS FACTORY is closin' down and I just heard ZASU PITTS has been DEAD for YEARS.. (sniff) My PLATFORM SHOE collection was CHEWED up by th'dog, ALEXANDER HAIG won't let me take a SHOWER 'til Easter.. (snurf) So I went to the kitchen, but WALNUT PANELING whup me upside mah HAID!! (on no, no, no.. Heh, heh) Date: Thu, 14 Feb 2019 17:51:22 +0100 Message-ID: <mvm5ztmpb3p.fsf@suse.de> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.91 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain |
Commit Message
Andreas Schwab
Feb. 14, 2019, 4:51 p.m. UTC
Use a unique name for the temporary file when updating the ld.so cache, so that two concurrent runs of ldconfig don't write to the same file. * elf/cache.c (save_cache): Use unique temporary name. (save_aux_cache): Likewise. --- elf/cache.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Comments
* Andreas Schwab: > Use a unique name for the temporary file when updating the ld.so cache, so > that two concurrent runs of ldconfig don't write to the same file. > > * elf/cache.c (save_cache): Use unique temporary name. > (save_aux_cache): Likewise. The downside of this change is that if ldconfig is interrupted, the temporary file never goes away. Ideally, we would use O_TMPFILE if supported by the (file) system, but that can get quite involved. > diff --git a/elf/cache.c b/elf/cache.c > index b8e9e6ccc3..ec7d94b0bc 100644 > --- a/elf/cache.c > +++ b/elf/cache.c > @@ -427,12 +427,12 @@ save_cache (const char *cache_name) > /* Write out the cache. */ > > /* Write cache first to a temporary file and rename it later. */ > - char *temp_name = xmalloc (strlen (cache_name) + 2); > - sprintf (temp_name, "%s~", cache_name); > + char *temp_name; > + if (asprintf (&temp_name, "%s.XXXXXX", cache_name) < 0) > + error (EXIT_FAILURE, errno, _("Can't allocate temporary name for cache file")); > > /* Create file. */ > - int fd = open (temp_name, O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW, > - S_IRUSR|S_IWUSR); > + int fd = mkostemp (temp_name, O_NOFOLLOW); I think you can use mkstemp because O_NOFOLLOW is implied by its use of O_EXCL. > + int fd = mkostemp (temp_name, O_NOFOLLOW); Likewise. Thanks, Florian
On Thu, Apr 18, 2019 at 02:01:55PM +0200, Florian Weimer wrote: > * Andreas Schwab: > > > Use a unique name for the temporary file when updating the ld.so cache, so > > that two concurrent runs of ldconfig don't write to the same file. > > > > * elf/cache.c (save_cache): Use unique temporary name. > > (save_aux_cache): Likewise. > > The downside of this change is that if ldconfig is interrupted, the > temporary file never goes away. > > Ideally, we would use O_TMPFILE if supported by the (file) system, but > that can get quite involved. Just saw this fly by so sorry if I miss the the necessary context: If there doesn't need to be a file on disk what about using memfd_create() on kernels that support it? > > > diff --git a/elf/cache.c b/elf/cache.c > > index b8e9e6ccc3..ec7d94b0bc 100644 > > --- a/elf/cache.c > > +++ b/elf/cache.c > > @@ -427,12 +427,12 @@ save_cache (const char *cache_name) > > /* Write out the cache. */ > > > > /* Write cache first to a temporary file and rename it later. */ > > - char *temp_name = xmalloc (strlen (cache_name) + 2); > > - sprintf (temp_name, "%s~", cache_name); > > + char *temp_name; > > + if (asprintf (&temp_name, "%s.XXXXXX", cache_name) < 0) > > + error (EXIT_FAILURE, errno, _("Can't allocate temporary name for cache file")); > > > > /* Create file. */ > > - int fd = open (temp_name, O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW, > > - S_IRUSR|S_IWUSR); > > + int fd = mkostemp (temp_name, O_NOFOLLOW); > > I think you can use mkstemp because O_NOFOLLOW is implied by its use of > O_EXCL. > > > + int fd = mkostemp (temp_name, O_NOFOLLOW); > > Likewise. > > Thanks, > Florian
* Christian Brauner: > On Thu, Apr 18, 2019 at 02:01:55PM +0200, Florian Weimer wrote: >> * Andreas Schwab: >> >> > Use a unique name for the temporary file when updating the ld.so cache, so >> > that two concurrent runs of ldconfig don't write to the same file. >> > >> > * elf/cache.c (save_cache): Use unique temporary name. >> > (save_aux_cache): Likewise. >> >> The downside of this change is that if ldconfig is interrupted, the >> temporary file never goes away. >> >> Ideally, we would use O_TMPFILE if supported by the (file) system, but >> that can get quite involved. > > Just saw this fly by so sorry if I miss the the necessary context: If > there doesn't need to be a file on disk what about using memfd_create() > on kernels that support it? The file needs to be on disk because it's used as part of the atomic file replacement pattern. Thanks, Florian
On Apr 18 2019, Florian Weimer <fweimer@redhat.com> wrote: > * Andreas Schwab: > >> Use a unique name for the temporary file when updating the ld.so cache, so >> that two concurrent runs of ldconfig don't write to the same file. >> >> * elf/cache.c (save_cache): Use unique temporary name. >> (save_aux_cache): Likewise. > > The downside of this change is that if ldconfig is interrupted, the > temporary file never goes away. > > Ideally, we would use O_TMPFILE if supported by the (file) system, but > that can get quite involved. That shortens the window, but won't close it, since we need a name to pass to rename in any case. >> diff --git a/elf/cache.c b/elf/cache.c >> index b8e9e6ccc3..ec7d94b0bc 100644 >> --- a/elf/cache.c >> +++ b/elf/cache.c >> @@ -427,12 +427,12 @@ save_cache (const char *cache_name) >> /* Write out the cache. */ >> >> /* Write cache first to a temporary file and rename it later. */ >> - char *temp_name = xmalloc (strlen (cache_name) + 2); >> - sprintf (temp_name, "%s~", cache_name); >> + char *temp_name; >> + if (asprintf (&temp_name, "%s.XXXXXX", cache_name) < 0) >> + error (EXIT_FAILURE, errno, _("Can't allocate temporary name for cache file")); >> >> /* Create file. */ >> - int fd = open (temp_name, O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW, >> - S_IRUSR|S_IWUSR); >> + int fd = mkostemp (temp_name, O_NOFOLLOW); > > I think you can use mkstemp because O_NOFOLLOW is implied by its use of > O_EXCL. Yes, O_NOFOLLOW is useless anyway, since mk[o]stemp is about creating a new file. Andreas.
* Andreas Schwab: > On Apr 18 2019, Florian Weimer <fweimer@redhat.com> wrote: > >> * Andreas Schwab: >> >>> Use a unique name for the temporary file when updating the ld.so cache, so >>> that two concurrent runs of ldconfig don't write to the same file. >>> >>> * elf/cache.c (save_cache): Use unique temporary name. >>> (save_aux_cache): Likewise. >> >> The downside of this change is that if ldconfig is interrupted, the >> temporary file never goes away. >> >> Ideally, we would use O_TMPFILE if supported by the (file) system, but >> that can get quite involved. > > That shortens the window, but won't close it, since we need a name to > pass to rename in any case. The difference is that with a fixed name, the next ldconfig run will use the same name and rename, cleaning up. With a random, unique name, that automatic cleanup is gone. Thanks, Florian
On Apr 18 2019, Florian Weimer <fweimer@redhat.com> wrote: > * Andreas Schwab: > >> On Apr 18 2019, Florian Weimer <fweimer@redhat.com> wrote: >> >>> * Andreas Schwab: >>> >>>> Use a unique name for the temporary file when updating the ld.so cache, so >>>> that two concurrent runs of ldconfig don't write to the same file. >>>> >>>> * elf/cache.c (save_cache): Use unique temporary name. >>>> (save_aux_cache): Likewise. >>> >>> The downside of this change is that if ldconfig is interrupted, the >>> temporary file never goes away. >>> >>> Ideally, we would use O_TMPFILE if supported by the (file) system, but >>> that can get quite involved. >> >> That shortens the window, but won't close it, since we need a name to >> pass to rename in any case. > > The difference is that with a fixed name, the next ldconfig run will use > the same name and rename, cleaning up. With a random, unique name, that > automatic cleanup is gone. O_TMPFILE won't change that. Andreas.
* Andreas Schwab: > On Apr 18 2019, Florian Weimer <fweimer@redhat.com> wrote: > >> * Andreas Schwab: >> >>> On Apr 18 2019, Florian Weimer <fweimer@redhat.com> wrote: >>> >>>> * Andreas Schwab: >>>> >>>>> Use a unique name for the temporary file when updating the ld.so cache, so >>>>> that two concurrent runs of ldconfig don't write to the same file. >>>>> >>>>> * elf/cache.c (save_cache): Use unique temporary name. >>>>> (save_aux_cache): Likewise. >>>> >>>> The downside of this change is that if ldconfig is interrupted, the >>>> temporary file never goes away. >>>> >>>> Ideally, we would use O_TMPFILE if supported by the (file) system, but >>>> that can get quite involved. >>> >>> That shortens the window, but won't close it, since we need a name to >>> pass to rename in any case. >> >> The difference is that with a fixed name, the next ldconfig run will use >> the same name and rename, cleaning up. With a random, unique name, that >> automatic cleanup is gone. > > O_TMPFILE won't change that. Hmm. I thought it would, but it does not. It's possible to avoid writing to the fixed name though, like this: #include <err.h> #include <errno.h> #include <fcntl.h> #include <libgen.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <time.h> #include <unistd.h> int main (int argc, char **argv) { if (argc != 2) { fprintf (stderr, "usage: %s PATH\n", argv[0]); exit (1); } const char *path = argv[1]; char *dir_storage = strdup (path); if (dir_storage == NULL) err (1, "strdup"); char *dir = dirname (dir_storage); int fd = open64 (dir, O_TMPFILE | O_RDWR, 0700); if (fd < 0) err (1, "open (\"%s\"., O_TMPFILE | O_RDWR)", dir); time_t t = time (NULL); const char *data = asctime (gmtime (&t)); if (write (fd, data, strlen (data)) != strlen (data)) errx (1, "write failure"); if (fsync (fd) != 0) err (1, "fsync"); char *proc_fd_path; if (asprintf (&proc_fd_path, "/proc/self/fd/%d", fd) < 0) err (1, "asprintf"); if (linkat (AT_FDCWD, proc_fd_path, AT_FDCWD, path, AT_SYMLINK_FOLLOW) == 0) { /* Nothing to do. */ } else { char *tmp_path; if (asprintf (&tmp_path, "%s~", path) < 0) err (1, "asprintf"); while (true) if (linkat (AT_FDCWD, proc_fd_path, AT_FDCWD, tmp_path, AT_SYMLINK_FOLLOW) == 0) break; else { if (errno == EEXIST) { if (unlink (tmp_path) != 0) if (errno != ENOENT) err (1, "unlink (\"%s\")", tmp_path); continue; } else err (1, "linkat"); } if (renameat2 (AT_FDCWD, tmp_path, AT_FDCWD, path, RENAME_EXCHANGE) != 0) err (1, "renameat2"); if (unlink (tmp_path) != 0) if (errno != ENOENT) err (1, "unlink (\"%s\")", tmp_path); free (tmp_path); } free (proc_fd_path); if (close (fd) != 0) err (1, "close"); free (dir); return 0; } This is unfortunately way more complicated than it should be. Thanks, Florian
On Apr 18 2019, Florian Weimer <fweimer@redhat.com> wrote:
> This is unfortunately way more complicated than it should be.
And you need to keep the fallback when O_TMPFILE isn't supported.
Andreas.
diff --git a/elf/cache.c b/elf/cache.c index b8e9e6ccc3..ec7d94b0bc 100644 --- a/elf/cache.c +++ b/elf/cache.c @@ -427,12 +427,12 @@ save_cache (const char *cache_name) /* Write out the cache. */ /* Write cache first to a temporary file and rename it later. */ - char *temp_name = xmalloc (strlen (cache_name) + 2); - sprintf (temp_name, "%s~", cache_name); + char *temp_name; + if (asprintf (&temp_name, "%s.XXXXXX", cache_name) < 0) + error (EXIT_FAILURE, errno, _("Can't allocate temporary name for cache file")); /* Create file. */ - int fd = open (temp_name, O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW, - S_IRUSR|S_IWUSR); + int fd = mkostemp (temp_name, O_NOFOLLOW); if (fd < 0) error (EXIT_FAILURE, errno, _("Can't create temporary cache file %s"), temp_name); @@ -481,6 +481,7 @@ save_cache (const char *cache_name) free (file_entries_new); free (file_entries); free (strings); + free (temp_name); while (entries) { @@ -804,8 +805,9 @@ save_aux_cache (const char *aux_cache_name) /* Write out auxiliary cache file. */ /* Write auxiliary cache first to a temporary file and rename it later. */ - char *temp_name = xmalloc (strlen (aux_cache_name) + 2); - sprintf (temp_name, "%s~", aux_cache_name); + char *temp_name; + if (asprintf (&temp_name, "%s.XXXXXX", aux_cache_name) < 0) + goto out_fail2; /* Check that directory exists and create if needed. */ char *dir = strdupa (aux_cache_name); @@ -819,8 +821,7 @@ save_aux_cache (const char *aux_cache_name) } /* Create file. */ - int fd = open (temp_name, O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW, - S_IRUSR|S_IWUSR); + int fd = mkostemp (temp_name, O_NOFOLLOW); if (fd < 0) goto out_fail; @@ -840,5 +841,6 @@ save_aux_cache (const char *aux_cache_name) out_fail: /* Free allocated memory. */ free (temp_name); +out_fail2: free (file_entries); }