| commit be6b16d975683e6cca57852cd4cfe715b2a9d8b1 |
| Author: Florian Weimer <fweimer@redhat.com> |
| Date: Thu Nov 7 18:15:18 2019 +0100 |
| |
| login: Acquire write lock early in pututline [BZ #24882] |
| |
| It has been reported that due to lack of fairness in POSIX file |
| locking, the current reader-to-writer lock upgrade can result in |
| lack of forward progress. Acquiring the write lock directly |
| hopefully avoids this issue if there are only writers. |
| |
| This also fixes bug 24882 due to the cache revalidation in |
| __libc_pututline. |
| |
| Reviewed-by: Carlos O'Donell <carlos@redhat.com> |
| Change-Id: I57e31ae30719e609a53505a0924dda101d46372e |
| |
| diff --git a/login/Makefile b/login/Makefile |
| index 82132c83fd799357..030cf489b2e037d4 100644 |
| |
| |
| @@ -44,7 +44,7 @@ subdir-dirs = programs |
| vpath %.c programs |
| |
| tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx \ |
| - tst-pututxline-lockfail |
| + tst-pututxline-lockfail tst-pututxline-cache |
| |
| # Build the -lutil library with these extra functions. |
| extra-libs := libutil |
| @@ -74,3 +74,4 @@ $(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force) |
| -$(INSTALL_PROGRAM) -m 4755 -o root $< $@ |
| |
| $(objpfx)tst-pututxline-lockfail: $(shared-thread-library) |
| +$(objpfx)tst-pututxline-cache: $(shared-thread-library) |
| diff --git a/login/tst-pututxline-cache.c b/login/tst-pututxline-cache.c |
| new file mode 100644 |
| index 0000000000000000..3f30dd1776711769 |
| |
| |
| @@ -0,0 +1,193 @@ |
| +/* Test case for cache invalidation after concurrent write (bug 24882). |
| + 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/>. */ |
| + |
| +/* This test writes an entry to the utmpx file, reads it (so that it |
| + is cached) in process1, and overwrites the same entry in process2 |
| + with something that does not match the search criteria. At this |
| + point, the cache of the first process is stale, and when process1 |
| + attempts to write a new record which would have gone to the same |
| + place (as indicated by the cache), it needs to realize that it has |
| + to pick a different slot because the old slot is now used for |
| + something else. */ |
| + |
| +#include <errno.h> |
| +#include <stdlib.h> |
| +#include <string.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 <utmp.h> |
| +#include <utmpx.h> |
| + |
| +/* Set to the path of the utmp file. */ |
| +static char *utmp_file; |
| + |
| +/* Used to synchronize the subprocesses. The barrier itself is |
| + allocated in shared memory. */ |
| +static pthread_barrier_t *barrier; |
| + |
| +/* setutxent with error checking. */ |
| +static void |
| +xsetutxent (void) |
| +{ |
| + errno = 0; |
| + setutxent (); |
| + TEST_COMPARE (errno, 0); |
| +} |
| + |
| +/* getutxent with error checking. */ |
| +static struct utmpx * |
| +xgetutxent (void) |
| +{ |
| + errno = 0; |
| + struct utmpx *result = getutxent (); |
| + if (result == NULL) |
| + FAIL_EXIT1 ("getutxent: %m"); |
| + return result; |
| +} |
| + |
| +static void |
| +put_entry (const char *id, pid_t pid, const char *user, const char *line) |
| +{ |
| + struct utmpx ut = |
| + { |
| + .ut_type = LOGIN_PROCESS, |
| + .ut_pid = pid, |
| + .ut_host = "localhost", |
| + }; |
| + strcpy (ut.ut_id, id); |
| + strncpy (ut.ut_user, user, sizeof (ut.ut_user)); |
| + strncpy (ut.ut_line, line, sizeof (ut.ut_line)); |
| + TEST_VERIFY (pututxline (&ut) != NULL); |
| +} |
| + |
| +/* Use two cooperating subprocesses to avoid issues related to |
| + unlock-on-close semantics of POSIX advisory locks. */ |
| + |
| +static __attribute__ ((noreturn)) void |
| +process1 (void) |
| +{ |
| + TEST_COMPARE (utmpname (utmp_file), 0); |
| + |
| + /* Create an entry. */ |
| + xsetutxent (); |
| + put_entry ("1", 101, "root", "process1"); |
| + |
| + /* Retrieve the entry. This will fill the internal cache. */ |
| + { |
| + errno = 0; |
| + setutxent (); |
| + TEST_COMPARE (errno, 0); |
| + struct utmpx ut = |
| + { |
| + .ut_type = LOGIN_PROCESS, |
| + .ut_line = "process1", |
| + }; |
| + struct utmpx *result = getutxline (&ut); |
| + if (result == NULL) |
| + FAIL_EXIT1 ("getutxline (\"process1\"): %m"); |
| + TEST_COMPARE (result->ut_pid, 101); |
| + } |
| + |
| + /* Signal the other process to overwrite the entry. */ |
| + xpthread_barrier_wait (barrier); |
| + |
| + /* Wait for the other process to complete the write operation. */ |
| + xpthread_barrier_wait (barrier); |
| + |
| + /* Add another entry. Note: This time, there is no setutxent call. */ |
| + put_entry ("1", 103, "root", "process1"); |
| + |
| + _exit (0); |
| +} |
| + |
| +static void |
| +process2 (void *closure) |
| +{ |
| + /* Wait for the first process to write its entry. */ |
| + xpthread_barrier_wait (barrier); |
| + |
| + /* Truncate the file. The glibc interface does not support |
| + re-purposing records, but an external expiration mechanism may |
| + trigger this. */ |
| + TEST_COMPARE (truncate64 (utmp_file, 0), 0); |
| + |
| + /* Write the replacement entry. */ |
| + TEST_COMPARE (utmpname (utmp_file), 0); |
| + xsetutxent (); |
| + put_entry ("2", 102, "user", "process2"); |
| + |
| + /* Signal the other process that the entry has been replaced. */ |
| + xpthread_barrier_wait (barrier); |
| +} |
| + |
| +static int |
| +do_test (void) |
| +{ |
| + xclose (create_temp_file ("tst-tumpx-cache-write-", &utmp_file)); |
| + { |
| + 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); |
| + } |
| + |
| + /* Run both subprocesses in parallel. */ |
| + { |
| + pid_t pid1 = xfork (); |
| + if (pid1 == 0) |
| + process1 (); |
| + support_isolate_in_subprocess (process2, NULL); |
| + int status; |
| + xwaitpid (pid1, &status, 0); |
| + TEST_COMPARE (status, 0); |
| + } |
| + |
| + /* Check that the utmpx database contains the expected records. */ |
| + { |
| + TEST_COMPARE (utmpname (utmp_file), 0); |
| + xsetutxent (); |
| + |
| + struct utmpx *ut = xgetutxent (); |
| + TEST_COMPARE_STRING (ut->ut_id, "2"); |
| + TEST_COMPARE (ut->ut_pid, 102); |
| + TEST_COMPARE_STRING (ut->ut_user, "user"); |
| + TEST_COMPARE_STRING (ut->ut_line, "process2"); |
| + |
| + ut = xgetutxent (); |
| + TEST_COMPARE_STRING (ut->ut_id, "1"); |
| + TEST_COMPARE (ut->ut_pid, 103); |
| + TEST_COMPARE_STRING (ut->ut_user, "root"); |
| + TEST_COMPARE_STRING (ut->ut_line, "process1"); |
| + |
| + if (getutxent () != NULL) |
| + FAIL_EXIT1 ("additional utmpx entry"); |
| + } |
| + |
| + xpthread_barrier_destroy (barrier); |
| + support_shared_free (barrier); |
| + free (utmp_file); |
| + |
| + return 0; |
| +} |
| + |
| +#include <support/test-driver.c> |
| diff --git a/login/utmp_file.c b/login/utmp_file.c |
| index 9ad80364682bae92..6bba120db9cc574e 100644 |
| |
| |
| @@ -186,19 +186,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. */ |
| + success and -1 on failure. Does not perform locking; for that see |
| + internal_getut_r below. */ |
| static int |
| -internal_getut_r (const struct utmp *id, bool *lock_failed) |
| +internal_getut_nolock (const struct utmp *id) |
| { |
| - int result = -1; |
| - |
| - if (try_file_lock (file_fd, F_RDLCK)) |
| - { |
| - *lock_failed = true; |
| - return -1; |
| - } |
| - |
| if (id->ut_type == RUN_LVL || id->ut_type == BOOT_TIME |
| || id->ut_type == OLD_TIME || id->ut_type == NEW_TIME) |
| { |
| @@ -213,7 +205,7 @@ internal_getut_r (const struct utmp *id, bool *lock_failed) |
| { |
| __set_errno (ESRCH); |
| file_offset = -1l; |
| - goto unlock_return; |
| + return -1; |
| } |
| file_offset += sizeof (struct utmp); |
| |
| @@ -234,7 +226,7 @@ internal_getut_r (const struct utmp *id, bool *lock_failed) |
| { |
| __set_errno (ESRCH); |
| file_offset = -1l; |
| - goto unlock_return; |
| + return -1; |
| } |
| file_offset += sizeof (struct utmp); |
| |
| @@ -243,15 +235,26 @@ internal_getut_r (const struct utmp *id, bool *lock_failed) |
| } |
| } |
| |
| - result = 0; |
| + return 0; |
| +} |
| |
| -unlock_return: |
| - file_unlock (file_fd); |
| +/* 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, bool *lock_failed) |
| +{ |
| + if (try_file_lock (file_fd, F_RDLCK)) |
| + { |
| + *lock_failed = true; |
| + return -1; |
| + } |
| |
| + int result = internal_getut_nolock (id); |
| + file_unlock (file_fd); |
| return result; |
| } |
| |
| - |
| /* For implementing this function we don't use the getutent_r function |
| because we can avoid the reposition on every new entry this way. */ |
| int |
| @@ -279,7 +282,6 @@ __libc_getutid_r (const struct utmp *id, struct utmp *buffer, |
| return 0; |
| } |
| |
| - |
| /* For implementing this function we don't use the getutent_r function |
| because we can avoid the reposition on every new entry this way. */ |
| int |
| @@ -336,7 +338,6 @@ __libc_pututline (const struct utmp *data) |
| return NULL; |
| |
| struct utmp *pbuf; |
| - int found; |
| |
| if (! file_writable) |
| { |
| @@ -358,7 +359,12 @@ __libc_pututline (const struct utmp *data) |
| file_writable = true; |
| } |
| |
| + /* Exclude other writers before validating the cache. */ |
| + if (try_file_lock (file_fd, F_WRLCK)) |
| + return NULL; |
| + |
| /* Find the correct place to insert the data. */ |
| + bool found = false; |
| if (file_offset > 0 |
| && ((last_entry.ut_type == data->ut_type |
| && (last_entry.ut_type == RUN_LVL |
| @@ -366,23 +372,30 @@ __libc_pututline (const struct utmp *data) |
| || last_entry.ut_type == OLD_TIME |
| || last_entry.ut_type == NEW_TIME)) |
| || __utmp_equal (&last_entry, data))) |
| - found = 1; |
| - else |
| { |
| - bool lock_failed = false; |
| - found = internal_getut_r (data, &lock_failed); |
| - |
| - if (__builtin_expect (lock_failed, false)) |
| + if (__lseek64 (file_fd, file_offset, SEEK_SET) < 0) |
| { |
| - __set_errno (EAGAIN); |
| + file_unlock (file_fd); |
| return NULL; |
| } |
| + if (__read_nocancel (file_fd, &last_entry, sizeof (last_entry)) |
| + != sizeof (last_entry)) |
| + { |
| + if (__lseek64 (file_fd, file_offset, SEEK_SET) < 0) |
| + { |
| + file_unlock (file_fd); |
| + return NULL; |
| + } |
| + found = false; |
| + } |
| + else |
| + found = __utmp_equal (&last_entry, data); |
| } |
| |
| - if (try_file_lock (file_fd, F_WRLCK)) |
| - return NULL; |
| + if (!found) |
| + found = internal_getut_nolock (data) >= 0; |
| |
| - if (found < 0) |
| + if (!found) |
| { |
| /* We append the next entry. */ |
| file_offset = __lseek64 (file_fd, 0, SEEK_END); |
| @@ -411,7 +424,7 @@ __libc_pututline (const struct utmp *data) |
| { |
| /* If we appended a new record this is only partially written. |
| Remove it. */ |
| - if (found < 0) |
| + if (!found) |
| (void) __ftruncate64 (file_fd, file_offset); |
| pbuf = NULL; |
| } |