From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93045 invoked by alias); 17 Jan 2020 14:05:00 -0000 Mailing-List: contact libc-stable-help@sourceware.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: List-Archive: Sender: libc-stable-owner@sourceware.org Received: (qmail 92830 invoked by uid 89); 17 Jan 2020 14:04:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-18.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=cooperating, sk:PTHREAD X-Spam-Status: No, score=-18.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (207.211.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 Jan 2020 14:04:43 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579269881; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=A59R8feDDkUld1dGPaqYdEPCT7D9a8hshTpCsJI0+oU=; b=goud6Wv5jdoxrEKPZG/slWGsj0R3s+/q/oymGmYCx7sdwJrZdKGXn4oU8KRM70OtugtVN2 1IGwAF4rROV8ODmj9G3+cMWuGwH2LymHe32ekvcazIRIjRDnlHe1B/dnKBYlZwSVaqWOuM WepyyoPcPkHrAZ0/mAKZB9l8vEnkD2g= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-211-2GqYsvLIMl-LJlqeLYZzmQ-1; Fri, 17 Jan 2020 09:04:40 -0500 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 69D8AA0CC2 for ; Fri, 17 Jan 2020 14:04:39 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-117-165.ams2.redhat.com [10.36.117.165]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D853283865 for ; Fri, 17 Jan 2020 14:04:38 +0000 (UTC) Received: by oldenburg2.str.redhat.com (Postfix, from userid 1000) id 8A7128299EE2; Fri, 17 Jan 2020 15:04:37 +0100 (CET) Date: Wed, 01 Jan 2020 00:00:00 -0000 To: libc-stable@sourceware.org Subject: [2.30 COMMITTED] login: Acquire write lock early in pututline [BZ #24882] User-Agent: Heirloom mailx 12.5 7/5/10 MIME-Version: 1.0 Message-Id: <20200117140437.8A7128299EE2@oldenburg2.str.redhat.com> From: Florian Weimer X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-MC-Unique: 2GqYsvLIMl-LJlqeLYZzmQ-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00007.txt.bz2 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 Change-Id: I57e31ae30719e609a53505a0924dda101d46372e (cherry picked from commit be6b16d975683e6cca57852cd4cfe715b2a9d8b1) diff --git a/NEWS b/NEWS index ae6c815515..1352a339ab 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,7 @@ The following bugs are resolved with this release: [24867] malloc: Remove unwanted leading whitespace in malloc_info [24879] login: Disarm timer after utmp lock acquisition [24880] login: Use struct flock64 in utmp + [24882] login: Acquire write lock early in pututline [24986] alpha: new getegid, geteuid and getppid syscalls used unconditionally [24899] login: Add nonstring attributes to struct utmp, struct utmpx diff --git a/login/Makefile b/login/Makefile index 93a3c8edf2..4fd8195e73 100644 --- a/login/Makefile +++ b/login/Makefile @@ -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 0000000000..3f30dd1776 --- /dev/null +++ b/login/tst-pututxline-cache.c @@ -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 . */ + +/* 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* 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 diff --git a/login/utmp_file.c b/login/utmp_file.c index 74ec622e96..d1612ff2b5 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -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: +/* 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; }