Message ID | gerrit.1573147933000.I57e31ae30719e609a53505a0924dda101d46372e@gnutoolchain-gerrit.osci.io |
---|---|
State | New |
Headers | show |
Series | [review] login: Acquire write lock early in pututline [BZ #24882] | expand |
Florian Weimer has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523 ...................................................................... Patch Set 1: This is a re-submission of: <https://sourceware.org/ml/libc-alpha/2019-08/msg00228.html> I think we still need it because of the fairness issue.
Carlos O'Donell has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523 ...................................................................... Patch Set 1: (7 comments) I like the design, particularly taking the write lock early, and then holding it while you do all the validation. What's missing is a more detailed explanation of the test and what the intent of the test is. Please add some more comments there and repost v2. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/Makefile File login/Makefile: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/Makefile@47 PS1, Line 47: 42 | 43 | subdir-dirs = programs 44 | vpath %.c programs 45 | 46 | tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx \ 47 > tst-pututxline-lockfail tst-pututxline-cache 48 | 49 | # Build the -lutil library with these extra functions. 50 | extra-libs := libutil 51 | extra-libs-others := $(extra-libs) 52 | OK. New normal test. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/Makefile@77 PS1, Line 77: 72 | $(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force) 73 | $(make-target-directory) 74 | -$(INSTALL_PROGRAM) -m 4755 -o root $< $@ 75 | 76 | $(objpfx)tst-pututxline-lockfail: $(shared-thread-library) 77 > $(objpfx)tst-pututxline-cache: $(shared-thread-library) OK. Test depends on pthread. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/tst-pututxline-cache.c File login/tst-pututxline-cache.c: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/tst-pututxline-cache.c@30 PS1, Line 30: 25 | #include <support/temp_file.h> 26 | #include <support/xthread.h> 27 | #include <support/xunistd.h> 28 | #include <utmp.h> 29 | #include <utmpx.h> 30 > 31 | /* Set to the path of the utmp file. */ 32 | static char *utmp_file; 33 | 34 | /* Used to synchronize the subprocesses. The barrier itself is 35 | allocated in shared memory. */ This test needs a big comment explaining what the test is doing and why what it is doing is correct, and what failure mode it is testing. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c File login/utmp_file.c: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@192 PS1, Line 192: 187 | 188 | /* Search for *ID, updating last_entry and file_offset. Return 0 on 189 | success and -1 on failure. Does not perform locking; for that see 190 | internal_getut_r below. */ 191 | static int 192 > internal_getut_nolock (const struct utmp *id) 193 | { 194 | if (id->ut_type == RUN_LVL || id->ut_type == BOOT_TIME 195 | || id->ut_type == OLD_TIME || id->ut_type == NEW_TIME) 196 | { 197 | /* Search for next entry with type RUN_LVL, BOOT_TIME, OK. Locking is removed from this function. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@245 PS1, Line 245: 240 | 241 | /* Search for *ID, updating last_entry and file_offset. Return 0 on 242 | success and -1 on failure. If the locking operation failed, write 243 | true to *LOCK_FAILED. */ 244 | static int 245 > internal_getut_r (const struct utmp *id, bool *lock_failed) 246 | { 247 | if (try_file_lock (file_fd, F_RDLCK)) 248 | { 249 | *lock_failed = true; 250 | return -1; OK. Locking refactored to here. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@364 PS1, Line 364: 335 | __libc_pututline (const struct utmp *data) | ... 359 | file_writable = true; 360 | } 361 | 362 | /* Exclude other writers before validating the cache. */ 363 | if (try_file_lock (file_fd, F_WRLCK)) 364 > return NULL; 365 | 366 | /* Find the correct place to insert the data. */ 367 | bool found = false; 368 | if (file_offset > 0 369 | && ((last_entry.ut_type == data->ut_type OK. We take the write lock right away before validation. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@396 PS1, Line 396: 335 | __libc_pututline (const struct utmp *data) | ... 391 | else 392 | found = __utmp_equal (&last_entry, data); 393 | } 394 | 395 | if (!found) 396 > found = internal_getut_nolock (data) >= 0; 397 | 398 | if (!found) 399 | { 400 | /* We append the next entry. */ 401 | file_offset = __lseek64 (file_fd, 0, SEEK_END); OK. Use the _nolock version because we already hold the lock.
Florian Weimer has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523
......................................................................
Patch Set 2:
(1 comment)
Not exactly a big comment, but there is now a birds-eye overview.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/tst-pututxline-cache.c
File login/tst-pututxline-cache.c:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/tst-pututxline-cache.c@30
PS1, Line 30:
25 | #include <support/temp_file.h>
26 | #include <support/xthread.h>
27 | #include <support/xunistd.h>
28 | #include <utmp.h>
29 | #include <utmpx.h>
30 >
31 | /* Set to the path of the utmp file. */
32 | static char *utmp_file;
33 |
34 | /* Used to synchronize the subprocesses. The barrier itself is
35 | allocated in shared memory. */
> This test needs a big comment explaining what the test is doing and why what it is doing is correct, […]
Done
Carlos O'Donell has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523 ...................................................................... Patch Set 2: Code-Review+2 (7 comments) I really like the refactoring for internal_getut_nolock, it makes the locking organization much clearer than passing down found and manipulting that indirect state about the lock status. Overall the code is clearer and makes more sense. The test case doesn't cover the case you are are fixing, but testing that is much harder. If empirically taking the write lock earlier, and avoiding the lock promotion is better then I'm OK with that. There is only some added latency and contention, but it resolves swbz#24882, which is a correctness issue. This looks good to me for master. Reviewed-by: Carlos O'Donell <carlos@redhat.com> https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/Makefile File login/Makefile: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/Makefile@47 PS1, Line 47: 42 | 43 | subdir-dirs = programs 44 | vpath %.c programs 45 | 46 | tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx \ 47 > tst-pututxline-lockfail tst-pututxline-cache 48 | 49 | # Build the -lutil library with these extra functions. 50 | extra-libs := libutil 51 | extra-libs-others := $(extra-libs) 52 | > OK. New normal test. Done https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/Makefile@77 PS1, Line 77: 72 | $(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force) 73 | $(make-target-directory) 74 | -$(INSTALL_PROGRAM) -m 4755 -o root $< $@ 75 | 76 | $(objpfx)tst-pututxline-lockfail: $(shared-thread-library) 77 > $(objpfx)tst-pututxline-cache: $(shared-thread-library) > OK. Test depends on pthread. Done https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/2/login/tst-pututxline-cache.c File login/tst-pututxline-cache.c: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/2/login/tst-pututxline-cache.c@19 PS2, Line 19: 14 | 15 | You should have received a copy of the GNU Lesser General Public 16 | License along with the GNU C Library; see the file COPYING.LIB. If 17 | not, see <http://www.gnu.org/licenses/>. */ 18 | 19 > /* This test writes an entry to the utmpx file, reads it (so that it 20 > is cached) in process1, and overwrites the same entry in process2 21 > with something that does not match the search criteria. At this 22 > point, the cache of the first process is stale, and when process1 23 > attempts to write a new record which would have gone to the same 24 > place (as indicated by the cache), it needs to realize that it has 25 > to pick a different slot because the old slot is now used for 26 > something else. */ 27 | 28 | #include <errno.h> 29 | #include <stdlib.h> 30 | #include <string.h> 31 | #include <support/check.h> OK. Comment looks good to me, and covers test case intent. https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c File login/utmp_file.c: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@192 PS1, Line 192: 187 | 188 | /* Search for *ID, updating last_entry and file_offset. Return 0 on 189 | success and -1 on failure. Does not perform locking; for that see 190 | internal_getut_r below. */ 191 | static int 192 > internal_getut_nolock (const struct utmp *id) 193 | { 194 | if (id->ut_type == RUN_LVL || id->ut_type == BOOT_TIME 195 | || id->ut_type == OLD_TIME || id->ut_type == NEW_TIME) 196 | { 197 | /* Search for next entry with type RUN_LVL, BOOT_TIME, > OK. Locking is removed from this function. Done https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@245 PS1, Line 245: 240 | 241 | /* Search for *ID, updating last_entry and file_offset. Return 0 on 242 | success and -1 on failure. If the locking operation failed, write 243 | true to *LOCK_FAILED. */ 244 | static int 245 > internal_getut_r (const struct utmp *id, bool *lock_failed) 246 | { 247 | if (try_file_lock (file_fd, F_RDLCK)) 248 | { 249 | *lock_failed = true; 250 | return -1; > OK. Locking refactored to here. Done https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@364 PS1, Line 364: 335 | __libc_pututline (const struct utmp *data) | ... 359 | file_writable = true; 360 | } 361 | 362 | /* Exclude other writers before validating the cache. */ 363 | if (try_file_lock (file_fd, F_WRLCK)) 364 > return NULL; 365 | 366 | /* Find the correct place to insert the data. */ 367 | bool found = false; 368 | if (file_offset > 0 369 | && ((last_entry.ut_type == data->ut_type > OK. We take the write lock right away before validation. Done https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523/1/login/utmp_file.c@396 PS1, Line 396: 335 | __libc_pututline (const struct utmp *data) | ... 391 | else 392 | found = __utmp_equal (&last_entry, data); 393 | } 394 | 395 | if (!found) 396 > found = internal_getut_nolock (data) >= 0; 397 | 398 | if (!found) 399 | { 400 | /* We append the next entry. */ 401 | file_offset = __lseek64 (file_fd, 0, SEEK_END); > OK. Use the _nolock version because we already hold the lock. Done
Carlos O'Donell has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/523 ...................................................................... Patch Set 3: Code-Review+2
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; }