310 lines
9.5 KiB
Diff
310 lines
9.5 KiB
Diff
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))
|
|
{
|