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>
This commit is contained in:
Florian Weimer 2019-08-28 11:59:45 +02:00
parent 3a9d025fdd
commit 61d3db4281
4 changed files with 201 additions and 11 deletions

View File

@ -1,3 +1,13 @@
2019-08-28 Florian Weimer <fweimer@redhat.com>
[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.
2019-08-28 Stefan Liebler <stli@linux.ibm.com> 2019-08-28 Stefan Liebler <stli@linux.ibm.com>
* posix/tst-regex.c (do_test): Use tst-regex.input as input file. * posix/tst-regex.c (do_test): Use tst-regex.input as input file.

View File

@ -43,7 +43,8 @@ endif
subdir-dirs = programs subdir-dirs = programs
vpath %.c 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. # Build the -lutil library with these extra functions.
extra-libs := libutil extra-libs := libutil
@ -71,3 +72,5 @@ endif
$(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force) $(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force)
$(make-target-directory) $(make-target-directory)
-$(INSTALL_PROGRAM) -m 4755 -o root $< $@ -$(INSTALL_PROGRAM) -m 4755 -o root $< $@
$(objpfx)tst-pututxline-lockfail: $(shared-thread-library)

View File

@ -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>

View File

@ -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 static int
internal_getut_r (const struct utmp *id, struct utmp *buffer, internal_getut_r (const struct utmp *id, bool *lock_failed)
bool *lock_failed)
{ {
int result = -1; int result = -1;
@ -206,7 +208,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
while (1) while (1)
{ {
/* Read the next entry. */ /* 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)) != sizeof (struct utmp))
{ {
__set_errno (ESRCH); __set_errno (ESRCH);
@ -215,7 +217,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
} }
file_offset += sizeof (struct utmp); file_offset += sizeof (struct utmp);
if (id->ut_type == buffer->ut_type) if (id->ut_type == last_entry.ut_type)
break; break;
} }
} }
@ -227,7 +229,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
while (1) while (1)
{ {
/* Read the next entry. */ /* 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)) != sizeof (struct utmp))
{ {
__set_errno (ESRCH); __set_errno (ESRCH);
@ -236,7 +238,7 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
} }
file_offset += sizeof (struct utmp); file_offset += sizeof (struct utmp);
if (__utmp_equal (buffer, id)) if (__utmp_equal (&last_entry, id))
break; 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 /* We don't have to distinguish whether we can lock the file or
whether there is no entry. */ whether there is no entry. */
bool lock_failed = false; bool lock_failed = false;
if (internal_getut_r (id, &last_entry, &lock_failed) < 0) if (internal_getut_r (id, &lock_failed) < 0)
{ {
*result = NULL; *result = NULL;
return -1; return -1;
@ -330,10 +332,9 @@ unlock_return:
struct utmp * struct utmp *
__libc_pututline (const struct utmp *data) __libc_pututline (const struct utmp *data)
{ {
if (!maybe_setutent ()) if (!maybe_setutent () || file_offset == -1l)
return NULL; return NULL;
struct utmp buffer;
struct utmp *pbuf; struct utmp *pbuf;
int found; int found;
@ -369,7 +370,7 @@ __libc_pututline (const struct utmp *data)
else else
{ {
bool lock_failed = false; 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)) if (__builtin_expect (lock_failed, false))
{ {