* [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
@ 2019-02-14 16:51 Andreas Schwab
2019-04-18 12:07 ` Florian Weimer
0 siblings, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2019-02-14 16:51 UTC (permalink / raw)
To: libc-alpha
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(-)
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);
}
--
2.20.1
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-02-14 16:51 [PATCH] Avoid concurrency problem in ldconfig (bug 23973) Andreas Schwab
@ 2019-04-18 12:07 ` Florian Weimer
2019-04-18 12:11 ` Christian Brauner
2019-04-18 13:17 ` Andreas Schwab
0 siblings, 2 replies; 10+ messages in thread
From: Florian Weimer @ 2019-04-18 12:07 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-04-18 12:07 ` Florian Weimer
@ 2019-04-18 12:11 ` Christian Brauner
2019-04-18 12:38 ` Florian Weimer
2019-04-18 13:17 ` Andreas Schwab
1 sibling, 1 reply; 10+ messages in thread
From: Christian Brauner @ 2019-04-18 12:11 UTC (permalink / raw)
To: Florian Weimer; +Cc: Andreas Schwab, libc-alpha
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-04-18 12:11 ` Christian Brauner
@ 2019-04-18 12:38 ` Florian Weimer
0 siblings, 0 replies; 10+ messages in thread
From: Florian Weimer @ 2019-04-18 12:38 UTC (permalink / raw)
To: Christian Brauner; +Cc: Andreas Schwab, libc-alpha
* 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-04-18 12:07 ` Florian Weimer
2019-04-18 12:11 ` Christian Brauner
@ 2019-04-18 13:17 ` Andreas Schwab
2019-04-18 14:48 ` Florian Weimer
1 sibling, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2019-04-18 13:17 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-04-18 13:17 ` Andreas Schwab
@ 2019-04-18 14:48 ` Florian Weimer
2019-04-18 15:20 ` Andreas Schwab
0 siblings, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2019-04-18 14:48 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-04-18 14:48 ` Florian Weimer
@ 2019-04-18 15:20 ` Andreas Schwab
2019-04-18 16:04 ` Florian Weimer
0 siblings, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2019-04-18 15:20 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-04-18 15:20 ` Andreas Schwab
@ 2019-04-18 16:04 ` Florian Weimer
2019-04-18 16:07 ` Andreas Schwab
0 siblings, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2019-04-18 16:04 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
2019-04-18 16:04 ` Florian Weimer
@ 2019-04-18 16:07 ` Andreas Schwab
0 siblings, 0 replies; 10+ messages in thread
From: Andreas Schwab @ 2019-04-18 16:07 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Avoid concurrency problem in ldconfig (bug 23973)
@ 2019-01-21 15:30 Andreas Schwab
0 siblings, 0 replies; 10+ messages in thread
From: Andreas Schwab @ 2019-01-21 15:30 UTC (permalink / raw)
To: libc-alpha
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(-)
diff --git a/elf/cache.c b/elf/cache.c
index b8e9e6ccc32e..ec7d94b0bc30 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);
}
--
2.20.1
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-18 16:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-14 16:51 [PATCH] Avoid concurrency problem in ldconfig (bug 23973) Andreas Schwab
2019-04-18 12:07 ` Florian Weimer
2019-04-18 12:11 ` Christian Brauner
2019-04-18 12:38 ` Florian Weimer
2019-04-18 13:17 ` Andreas Schwab
2019-04-18 14:48 ` Florian Weimer
2019-04-18 15:20 ` Andreas Schwab
2019-04-18 16:04 ` Florian Weimer
2019-04-18 16:07 ` Andreas Schwab
-- strict thread matches above, loose matches on Subject: below --
2019-01-21 15:30 Andreas Schwab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).