[PATCH] login: pututxline could fail to overwrite existing entries [BZ #24902]

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] login: pututxline could fail to overwrite existing entries [BZ #24902]

Florian Weimer-5
The internal_getut_r function updates the file_offset variable and
therefore must always update last_entry as well.

Previously, if pututxline could not upgrade the read lock to a
write lock, internal_getut_r would update file_offset only,
without updating last_entry, and a subsequent call would not
overwrite the existing utmpx entry at file_offset, instead
creating a new entry.  This has been observed to cause unbounded
file growth in high-load situations.

This commit removes the buffer argument to internal_getut_r and
updates the last_entry variable directly, along with file_offset.

Initially reported and fixed by Ondřej Lysoněk.

2019-08-21  Florian Weimer  <[hidden email]>

        [BZ #24902]
        * login/Makefile (tests): Add tst-pututxline-lockfail.
        (tst-pututxline-lockfail): Link with -lpthread.
        * login/utmp_file.c (internal_getut_r): Remove buffer argument.
        (__libc_getutid_r): Adjust.
        (__libc_pututline): Likewise.  Check for file_offset == -1.
        * login/tst-pututxline-lockfail.c: New file.

diff --git a/login/Makefile b/login/Makefile
index f9c4264087..93a3c8edf2 100644
--- a/login/Makefile
+++ b/login/Makefile
@@ -43,7 +43,8 @@ endif
 subdir-dirs = programs
 vpath %.c programs
 
-tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx
+tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx \
+  tst-pututxline-lockfail
 
 # Build the -lutil library with these extra functions.
 extra-libs      := libutil
@@ -71,3 +72,5 @@ endif
 $(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force)
  $(make-target-directory)
  -$(INSTALL_PROGRAM) -m 4755 -o root $< $@
+
+$(objpfx)tst-pututxline-lockfail: $(shared-thread-library)
diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
new file mode 100644
index 0000000000..c8fad13622
--- /dev/null
+++ b/login/tst-pututxline-lockfail.c
@@ -0,0 +1,176 @@
+/* Test the lock upgrade path in tst-pututxline.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+/* pututxline upgrades the read lock on the file to a write lock.
+   This test verifies that if the lock upgrade fails, the utmp
+   subsystem remains in a consistent state, so that pututxline can be
+   called again.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+#include <utmp.h>
+#include <utmpx.h>
+
+/* Path to the temporary utmp file.   */
+static char *path;
+
+/* Used to synchronize the subprocesses.  The barrier itself is
+   allocated in shared memory.  */
+static pthread_barrier_t *barrier;
+
+/* Use pututxline to write an entry for PID.  */
+static struct utmpx *
+write_entry (pid_t pid)
+{
+  struct utmpx ut =
+    {
+     .ut_type = LOGIN_PROCESS,
+     .ut_id = "1",
+     .ut_user = "root",
+     .ut_pid = pid,
+     .ut_line = "entry",
+     .ut_host = "localhost",
+    };
+  return pututxline (&ut);
+}
+
+/* Create the initial entry in a subprocess, so that the utmp
+   subsystem in the original rocess is not disturbed.  */
+static void
+subprocess_create_entry (void *closure)
+{
+  TEST_COMPARE (utmpname (path), 0);
+  TEST_VERIFY (write_entry (101) != NULL);
+}
+
+/* Acquire an advisory read lock on PATH.  */
+__attribute__ ((noreturn)) static void
+subprocess_lock_file (void)
+{
+  int fd = xopen (path, O_RDONLY, 0);
+
+  struct flock64 fl =
+    {
+     .l_type = F_RDLCK,
+     fl.l_whence = SEEK_SET,
+    };
+  TEST_COMPARE (fcntl64 (fd, F_SETLKW, &fl), 0);
+
+  /* Signal to the main process that the lock has been acquired.  */
+  xpthread_barrier_wait (barrier);
+
+  /* Wait for the unlock request from the main process.  */
+  xpthread_barrier_wait (barrier);
+
+  /* Implicitly unlock the file.  */
+  xclose (fd);
+
+  /* Overwrite the existing entry.  */
+  TEST_COMPARE (utmpname (path), 0);
+  errno = 0;
+  setutxent ();
+  TEST_COMPARE (errno, 0);
+  TEST_VERIFY (write_entry (102) != NULL);
+  errno = 0;
+  endutxent ();
+  TEST_COMPARE (errno, 0);
+
+  _exit (0);
+}
+
+static int
+do_test (void)
+{
+  xclose (create_temp_file ("tst-pututxline-lockfail-", &path));
+
+  {
+    pthread_barrierattr_t attr;
+    xpthread_barrierattr_init (&attr);
+    xpthread_barrierattr_setpshared (&attr, PTHREAD_SCOPE_PROCESS);
+    barrier = support_shared_allocate (sizeof (*barrier));
+    xpthread_barrier_init (barrier, &attr, 2);
+    xpthread_barrierattr_destroy (&attr);
+  }
+
+  /* Write the initial entry.  */
+  support_isolate_in_subprocess (subprocess_create_entry, NULL);
+
+  pid_t locker_pid = xfork ();
+  if (locker_pid == 0)
+    subprocess_lock_file ();
+
+  /* Wait for the file locking to complete.  */
+  xpthread_barrier_wait (barrier);
+
+  /* Try to add another entry.  This attempt will fail, with EINTR or
+     EAGAIN.  */
+  TEST_COMPARE (utmpname (path), 0);
+  TEST_VERIFY (write_entry (102) == NULL);
+  if (errno != EINTR)
+    TEST_COMPARE (errno, EAGAIN);
+
+  /* Signal the subprocess to overwrite the entry.  */
+  xpthread_barrier_wait (barrier);
+
+  /* Wait for write and unlock to complete.  */
+  {
+    int status;
+    xwaitpid (locker_pid, &status, 0);
+    TEST_COMPARE (status, 0);
+  }
+
+  /* The file is no longer locked, so this operation will succeed.  */
+  TEST_VERIFY (write_entry (103) != NULL);
+  errno = 0;
+  endutxent ();
+  TEST_COMPARE (errno, 0);
+
+  /* Check that there is just one entry with the expected contents.
+     If pututxline becomes desynchronized internally, the entry is not
+     overwritten (bug 24902).  */
+  errno = 0;
+  setutxent ();
+  TEST_COMPARE (errno, 0);
+  struct utmpx *ut = getutxent ();
+  TEST_VERIFY_EXIT (ut != NULL);
+  TEST_COMPARE (ut->ut_type, LOGIN_PROCESS);
+  TEST_COMPARE_STRING (ut->ut_id, "1");
+  TEST_COMPARE_STRING (ut->ut_user, "root");
+  TEST_COMPARE (ut->ut_pid, 103);
+  TEST_COMPARE_STRING (ut->ut_line, "entry");
+  TEST_COMPARE_STRING (ut->ut_host, "localhost");
+  TEST_VERIFY (getutxent () == NULL);
+  errno = 0;
+  endutxent ();
+  TEST_COMPARE (errno, 0);
+
+  xpthread_barrier_destroy (barrier);
+  support_shared_free (barrier);
+  free (path);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/login/utmp_file.c b/login/utmp_file.c
index 94753e0404..2d0548f6fa 100644
--- a/login/utmp_file.c
+++ b/login/utmp_file.c
@@ -185,9 +185,11 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result)
 }
 
 
+/* Search for *ID, updating last_entry and file_offset.  Return 0 on
+   success and -1 on failure.  If the locking operation failed, write
+   true to *LOCK_FAILED.  */
 static int
-internal_getut_r (const struct utmp *id, struct utmp *buffer,
-  bool *lock_failed)
+internal_getut_r (const struct utmp *id, bool *lock_failed)
 {
   int result = -1;
 
@@ -206,7 +208,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
       while (1)
  {
   /* Read the next entry.  */
-  if (__read_nocancel (file_fd, buffer, sizeof (struct utmp))
+  if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp))
       != sizeof (struct utmp))
     {
       __set_errno (ESRCH);
@@ -215,7 +217,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
     }
   file_offset += sizeof (struct utmp);
 
-  if (id->ut_type == buffer->ut_type)
+  if (id->ut_type == last_entry.ut_type)
     break;
  }
     }
@@ -227,7 +229,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
       while (1)
  {
   /* Read the next entry.  */
-  if (__read_nocancel (file_fd, buffer, sizeof (struct utmp))
+  if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp))
       != sizeof (struct utmp))
     {
       __set_errno (ESRCH);
@@ -236,7 +238,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
     }
   file_offset += sizeof (struct utmp);
 
-  if (__utmp_equal (buffer, id))
+  if (__utmp_equal (&last_entry, id))
     break;
  }
     }
@@ -265,7 +267,7 @@ __libc_getutid_r (const struct utmp *id, struct utmp *buffer,
   /* We don't have to distinguish whether we can lock the file or
      whether there is no entry.  */
   bool lock_failed = false;
-  if (internal_getut_r (id, &last_entry, &lock_failed) < 0)
+  if (internal_getut_r (id, &lock_failed) < 0)
     {
       *result = NULL;
       return -1;
@@ -330,10 +332,9 @@ unlock_return:
 struct utmp *
 __libc_pututline (const struct utmp *data)
 {
-  if (!maybe_setutent ())
+  if (!maybe_setutent () || file_offset == -1l)
     return NULL;
 
-  struct utmp buffer;
   struct utmp *pbuf;
   int found;
 
@@ -369,7 +370,7 @@ __libc_pututline (const struct utmp *data)
   else
     {
       bool lock_failed = false;
-      found = internal_getut_r (data, &buffer, &lock_failed);
+      found = internal_getut_r (data, &lock_failed);
 
       if (__builtin_expect (lock_failed, false))
  {
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] login: pututxline could fail to overwrite existing entries [BZ #24902]

Gabriel F. T. Gomes-4
On Wed, Aug 21 2019, Florian Weimer wrote:
>
> Previously, if pututxline could not upgrade the read lock to a
> write lock, internal_getut_r would update file_offset only,
> without updating last_entry, and a subsequent call would not
> overwrite the existing utmpx entry at file_offset, instead
> creating a new entry.  This has been observed to cause unbounded
> file growth in high-load situations.

Looks good to me with the typo below fixed.  Thanks.

Reviewed-by: Gabriel F. T. Gomes <[hidden email]>

> +/* Create the initial entry in a subprocess, so that the utmp
> +   subsystem in the original rocess is not disturbed.  */
                                ~~~~~~
Typo: process?

> +__attribute__ ((noreturn)) static void
> +subprocess_lock_file (void)
> +{
> +  int fd = xopen (path, O_RDONLY, 0);
> +
> +  struct flock64 fl =
> +    {
> +     .l_type = F_RDLCK,
> +     fl.l_whence = SEEK_SET,
> +    };
> +  TEST_COMPARE (fcntl64 (fd, F_SETLKW, &fl), 0);
> +
> +  /* Signal to the main process that the lock has been acquired.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Wait for the unlock request from the main process.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Implicitly unlock the file.  */
> +  xclose (fd);
> +
> +  /* Overwrite the existing entry.  */
> +  TEST_COMPARE (utmpname (path), 0);
> +  errno = 0;
> +  setutxent ();
> +  TEST_COMPARE (errno, 0);
> +  TEST_VERIFY (write_entry (102) != NULL);

OK.  This will replace the only entry in the utmp file.

(that's my first contact with the user management database file and
functions, so I'll not 100% sure I got it right).

> +static int
> +do_test (void)
> +{
> +  xclose (create_temp_file ("tst-pututxline-lockfail-", &path));
> +
> +  {
> +    pthread_barrierattr_t attr;
> +    xpthread_barrierattr_init (&attr);
> +    xpthread_barrierattr_setpshared (&attr, PTHREAD_SCOPE_PROCESS);
> +    barrier = support_shared_allocate (sizeof (*barrier));
> +    xpthread_barrier_init (barrier, &attr, 2);
> +    xpthread_barrierattr_destroy (&attr);
> +  }
> +
> +  /* Write the initial entry.  */
> +  support_isolate_in_subprocess (subprocess_create_entry, NULL);
> +
> +  pid_t locker_pid = xfork ();
> +  if (locker_pid == 0)
> +    subprocess_lock_file ();
> +
> +  /* Wait for the file locking to complete.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Try to add another entry.  This attempt will fail, with EINTR or
> +     EAGAIN.  */
> +  TEST_COMPARE (utmpname (path), 0);
> +  TEST_VERIFY (write_entry (102) == NULL);
> +  if (errno != EINTR)
> +    TEST_COMPARE (errno, EAGAIN);

OK.  This will try to update the current entry, leaving file_offset and
last_entry in an inconsistent state (the bug this patch fixes).

> +  /* Signal the subprocess to overwrite the entry.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Wait for write and unlock to complete.  */
> +  {
> +    int status;
> +    xwaitpid (locker_pid, &status, 0);
> +    TEST_COMPARE (status, 0);
> +  }
> +
> +  /* The file is no longer locked, so this operation will succeed.  */
> +  TEST_VERIFY (write_entry (103) != NULL);
> +  errno = 0;
> +  endutxent ();
> +  TEST_COMPARE (errno, 0);
> +
> +  /* Check that there is just one entry with the expected contents.
> +     If pututxline becomes desynchronized internally, the entry is not
> +     overwritten (bug 24902).  */
> +  errno = 0;
> +  setutxent ();
> +  TEST_COMPARE (errno, 0);
> +  struct utmpx *ut = getutxent ();
> +  TEST_VERIFY_EXIT (ut != NULL);
> +  TEST_COMPARE (ut->ut_type, LOGIN_PROCESS);
> +  TEST_COMPARE_STRING (ut->ut_id, "1");
> +  TEST_COMPARE_STRING (ut->ut_user, "root");
> +  TEST_COMPARE (ut->ut_pid, 103);

Without the fix, this assert fails, because getutxent finds the
not-overwritten entry with pid == 102.  After the fix, it works
correctly (note: I tested the patch with and without the actual fix).

> +  TEST_COMPARE_STRING (ut->ut_line, "entry");
> +  TEST_COMPARE_STRING (ut->ut_host, "localhost");
> +  TEST_VERIFY (getutxent () == NULL);

Without the fix, this assert fails, because there is an extra entry in
the database file.