[review] login: Acquire write lock early in pututline [BZ #24882]

Message ID	gerrit.1573147933000.I57e31ae30719e609a53505a0924dda101d46372e@gnutoolchain-gerrit.osci.io
State	New
Headers	show Return-Path: <libc-alpha-return-106774-incoming=patchwork.ozlabs.org@sourceware.org> DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:message-id:subject:references :reply-to:mime-version:content-transfer-encoding:content-type; q=dns; s=default; b=kcIP2wjnimeYLuORCZxwEFxy4/urtNZzLQWP6NNSc/N zhbov/5P6rJtPtsRhNXE/Pmtd9DMY9YRV49Ikwka0XmPlu6ZRwfKSOjthnkzJcVK qP4ZqZgCpkvWZMxAcNs+to78FnWXWm4BjVECu4Z8S3UCGYsvaCY1UtaWuaQZ9DMc = Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk Sender: libc-alpha-owner@sourceware.org Date: Thu, 7 Nov 2019 12:32:13 -0500 From: "Florian Weimer (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io> To: libc-alpha@sourceware.org Cc: Florian Weimer <fweimer@redhat.com> Message-ID: <gerrit.1573147933000.I57e31ae30719e609a53505a0924dda101d46372e@gnutoolchain-gerrit.osci.io> Auto-Submitted: auto-generated Subject: [review] login: Acquire write lock early in pututline [BZ #24882] References: <gerrit.1573147933000.I57e31ae30719e609a53505a0924dda101d46372e@gnutoolchain-gerrit.osci.io> Reply-To: fweimer@redhat.com, fweimer@redhat.com, libc-alpha@sourceware.org MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Content-Type: text/plain; charset=UTF-8
Series	[review] login: Acquire write lock early in pututline [BZ #24882] \| expand [review] login: Acquire write lock early in pututline [BZ #24882]

diff --git a/login/Makefile b/login/Makefile index 0183db1..6a0182b 100644 --- a/login/Makefile +++ b/login/Makefile @@ -44,7 +44,7 @@ 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 @@ -$(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 0000000..f66ee17 --- /dev/null +++ b/login/tst-pututxline-cache.c @@ -0,0 +1,184 @@ +/* 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/>. */ + +#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 e627233..917c4c5 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -186,19 +186,11 @@ /* 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 @@ { __set_errno (ESRCH); file_offset = -1l; - goto unlock_return; + return -1; } file_offset += sizeof (struct utmp); @@ -234,7 +226,7 @@ { __set_errno (ESRCH); file_offset = -1l; - goto unlock_return; + return -1; } file_offset += sizeof (struct utmp); @@ -243,14 +235,25 @@ } } - result = 0; - -unlock_return: - file_unlock (file_fd); - - return result; + return 0; } +/* 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. */ @@ -279,7 +282,6 @@ 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 @@ return NULL; struct utmp *pbuf; - int found; if (! file_writable) { @@ -358,7 +359,12 @@ 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 @@ || 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 @@ { /* 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; }

[review] login: Acquire write lock early in pututline [BZ #24882]

Commit Message

Comments

Patch