Avoid concurrency problem in ldconfig (bug 23973)

  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(-)
  

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