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

["Carlos O'Donell (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

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.



-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I57e31ae30719e609a53505a0924dda101d46372e
Gerrit-Change-Number: 523
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-CC: Carlos O'Donell <carlos@redhat.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 17:52:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment