commit 61d3db428176d9d0822e4e680305fe34285edff2

Author: Florian Weimer <fweimer@redhat.com>

Date:   Wed Aug 28 11:59:45 2019 +0200

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

    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.

    Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>

diff --git a/login/Makefile b/login/Makefile

index 81986ab6bd8560ea..82132c83fd799357 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 0000000000000000..47c25dc0658d3c60

--- /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 process 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 a736d3d25e005920..cbc53d06de280af9 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))

 	{