diff --git a/ChangeLog b/ChangeLog index 85d3e195a4..becd26f7d2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2019-08-28 Florian Weimer + + [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-15 Florian Weimer [BZ #24880] diff --git a/NEWS b/NEWS index 8952a8a77d..b0ab63a83b 100644 --- a/NEWS +++ b/NEWS @@ -25,6 +25,7 @@ The following bugs are resolved with this release: [24880] login: Use struct flock64 in utmp [24986] alpha: new getegid, geteuid and getppid syscalls used unconditionally + [24902] login: pututxline could fail to overwrite existing entries [25189] Don't use a custom wrapper macro around __has_include [25203] libio: Disable vtable validation for pre-2.1 interposed handles [25204] Ignore LD_PREFER_MAP_32BIT_EXEC for SUID programs 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..47c25dc065 --- /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 . */ + +/* 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* 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 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)) {