login: Fix updwtmp, updwtmx unlocking

  Commit 5a3afa9738f3dbbaf8c0a35665318c1af782111b (login: Replace
macro-based control flow with function calls in utmp) introduced
a regression because after it, __libc_updwtmp attempts to unlock
the wrong file descriptor.

2019-08-14  Florian Weimer  <fweimer@redhat.com>

	* login/utmp_file.c (__libc_updwtmp): Unlock the right file
	descriptor.
	* login/Makefile (tests): Add tst-updwtmpx.
	* login/tst-updwtmpx.c: New file.
  

On 8/14/19 7:33 AM, Florian Weimer wrote:
> Commit 5a3afa9738f3dbbaf8c0a35665318c1af782111b (login: Replace
> macro-based control flow with function calls in utmp) introduced
> a regression because after it, __libc_updwtmp attempts to unlock
> the wrong file descriptor.

This makes sense, and it was an easy mistake to make because many
other functions use the same pattern to unlock file_fd.

OK for master if you add a block comment describing the test to the
testsuite.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2019-08-14  Florian Weimer  <fweimer@redhat.com>
> 
> 	* login/utmp_file.c (__libc_updwtmp): Unlock the right file
> 	descriptor.
> 	* login/Makefile (tests): Add tst-updwtmpx.
> 	* login/tst-updwtmpx.c: New file.
> 
> diff --git a/login/Makefile b/login/Makefile
> index 92535f0aec..f9c4264087 100644
> --- a/login/Makefile
> +++ b/login/Makefile
> @@ -43,7 +43,7 @@ endif
>   subdir-dirs = programs
>   vpath %.c programs
>   
> -tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin
> +tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx

OK.

>   
>   # Build the -lutil library with these extra functions.
>   extra-libs      := libutil
> diff --git a/login/tst-updwtmpx.c b/login/tst-updwtmpx.c
> new file mode 100644
> index 0000000000..4eb4a3fbcd
> --- /dev/null
> +++ b/login/tst-updwtmpx.c
> @@ -0,0 +1,108 @@
> +/* Basic test coverage for updwtmpx.

OK. Because we broke __libc_updwtmp.

> +   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 <support/check.h>
> +#include <support/descriptors.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +#include <utmpx.h>
> +

Needs a one paragraph description of the intent of the test.

> +static int
> +do_test (void)
> +{
> +  /* Two entries filled with an arbitrary bit pattern.  */
> +  struct utmpx entries[2];

OK.

> +  unsigned char pad;
> +  {
> +    unsigned char *p = (unsigned char *) &entries[0];
> +    for (size_t i = 0; i < sizeof (entries); ++i)
> +      {
> +        p[i] = i;
> +      }
> +    /* Make sure that the first and second entry and the padding are
> +       different.  */
> +    p[sizeof (struct utmpx)] = p[0] + 1;
> +    pad = p[0] + 2;
> +  }

OK.

> +
> +  char *path;
> +  int fd = create_temp_file ("tst-updwtmpx-", &path);

OK.

> +
> +  /* Used to check that updwtmpx does not leave an open file
> +     descriptor around.  */
> +  struct support_descriptors *descriptors = support_descriptors_list ();

OK.

> +
> +  /* updwtmpx is expected to remove misalignment.  Optionally insert
> +     one byte of misalignment at the start and in the middle (after
> +     the first entry).  */

OK.

> +  for (int misaligned_start = 0; misaligned_start < 2; ++misaligned_start)
> +    for (int misaligned_middle = 0; misaligned_middle < 2; ++misaligned_middle)
> +      {
> +        if (test_verbose > 0)
> +          printf ("info: misaligned_start=%d misaligned_middle=%d\n",
> +                  misaligned_start, misaligned_middle);
> +
> +        xftruncate (fd, 0);
> +        TEST_COMPARE (pwrite64 (fd, &pad, misaligned_start, 0),
> +                      misaligned_start);
> +
> +        /* Write first entry and check it.  */
> +        errno = 0;
> +        updwtmpx (path, &entries[0]);
> +        TEST_COMPARE (errno, 0);
> +        support_descriptors_check (descriptors);
> +        TEST_COMPARE (xlseek (fd, 0, SEEK_END), sizeof (struct utmpx));
> +        struct utmpx buffer;
> +        TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), 0),
> +                      sizeof (buffer));
> +        TEST_COMPARE_BLOB (&entries[0], sizeof (entries[0]),
> +                           &buffer, sizeof (buffer));

OK.

> +
> +        /* Middle mis-alignmet.  */
> +        TEST_COMPARE (pwrite64 (fd, &pad, misaligned_middle,
> +                                sizeof (struct utmpx)), misaligned_middle);
> +
> +        /* Write second entry and check both entries.  */
> +        errno = 0;
> +        updwtmpx (path, &entries[1]);
> +        TEST_COMPARE (errno, 0);
> +        support_descriptors_check (descriptors);
> +        TEST_COMPARE (xlseek (fd, 0, SEEK_END), 2 * sizeof (struct utmpx));
> +        TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), 0),
> +                      sizeof (buffer));
> +        TEST_COMPARE_BLOB (&entries[0], sizeof (entries[0]),
> +                           &buffer, sizeof (buffer));
> +        TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), sizeof (buffer)),
> +                      sizeof (buffer));
> +        TEST_COMPARE_BLOB (&entries[1], sizeof (entries[1]),
> +                           &buffer, sizeof (buffer));

OK.

> +      }
> +
> +  support_descriptors_free (descriptors);
> +  free (path);
> +  xclose (fd);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/login/utmp_file.c b/login/utmp_file.c
> index 7bd6034af4..5e4e66d1d0 100644
> --- a/login/utmp_file.c
> +++ b/login/utmp_file.c
> @@ -503,7 +503,7 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
>     result = 0;
>   
>   unlock_return:
> -  file_unlock (file_fd);
> +  file_unlock (fd);

OK. Reviewed the surrounding code, and verified that __libc_updwtmp uses 'fd' because
it's what was locked earlier in the function, and that file_fd should not be used
(which is only for setutent/getutent/endutent/getutline... and others which don't
open their own file).

>     file_lock_restore (&fl);
>   
>     /* Close WTMP file.  */
>