[review v2] 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 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



-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I57e31ae30719e609a53505a0924dda101d46372e
Gerrit-Change-Number: 523
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 21:44:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: comment