From patchwork Wed Aug 21 13:39:21 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 34224 Received: (qmail 40297 invoked by alias); 21 Aug 2019 13:39:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 40289 invoked by uid 89); 21 Aug 2019 13:39:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=lesser X-HELO: mx1.redhat.com From: Florian Weimer To: libc-alpha@sourceware.org Subject: [PATCH] login: pututxline could fail to overwrite existing entries [BZ #24902] Date: Wed, 21 Aug 2019 15:39:21 +0200 Message-ID: <87a7c2hc3a.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 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. 2019-08-21 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. Reviewed-by: Gabriel F. T. Gomes 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..c8fad13622 --- /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 rocess 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)) {