Avoid concurrency problem in ldconfig (bug 23973)

Message ID mvm5ztmpb3p.fsf@suse.de
State New
Headers show
Series
  • Avoid concurrency problem in ldconfig (bug 23973)
Related show

Commit Message

Andreas Schwab Feb. 14, 2019, 4:51 p.m.
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(-)

-- 
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."

Comments

Florian Weimer April 18, 2019, 12:01 p.m. | #1
* 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
Christian Brauner April 18, 2019, 12:09 p.m. | #2
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
Florian Weimer April 18, 2019, 12:11 p.m. | #3
* 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
Andreas Schwab April 18, 2019, 12:59 p.m. | #4
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."
Florian Weimer April 18, 2019, 2:47 p.m. | #5
* 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
Andreas Schwab April 18, 2019, 3:16 p.m. | #6
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."
Florian Weimer April 18, 2019, 3:52 p.m. | #7
* 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
Andreas Schwab April 18, 2019, 4:04 p.m. | #8
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."

Patch

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);
 }